We're missing a third Context that can be included in view subclasses

[sgrif]

As it stands it turns out it's quite a pain to use many of the Macroid DSLs from inside of a view subclass. There's a couple of pain points that have come out over the last day or two for me.

Things require ActivityContext when they don't appear to need it.

Specifically the layout building DSL. I think this is a trivial one to resolve, and just have it take an AppContext instead, but I wanted to get your opinion on it.

Unable to use things which require AppContext when I only have an implicit ActivityContext.

This one might be a little bit less straightforward to resolve. Here's a specific case I ran into, for reference. Because I'd like to use the layout building DSL inside of my View, I've changed the signature to class MyView(implicit ctx: ActivityContext) extends View(ctx.get). However, inside of my layout code, I would like to set a height of a child in DP. The DisplayUnits implicit requires an AppContext. I should be able to use ActivityContext anywhere I need an AppContext.

Is there anywhere in the API that actually requires an ActivityContext specifically? Does it make sense to have a common parent trait that both implement and is the thing required by the APIs internally?

Should we have a more general trait that can be included for things with contexts?

Specifically wondering if it makes sense to look for the structural type getContext: Context, and define the implicit conversion for that type in the Contexts trait.

The layout DSL doesn't play nicely with implicits

The macro currently assumes that the first argument is the context, which doesn't work for the following signatures which one might want to use if they're using Macroid, since they all are technically a 0 argument constructor with 2 argument lists.

class MyView(implicit ctx: Context) extends View(ctx) {
  implicit val appContext = AppContext(ctx)
}

class MyView(implicit ctx: ActivityContext) extends View(ctx.get)

class MyView(implicit ctx: ActivityContext, app: AppContext) extends View(ctx.get)

In order for my View class to be useable by Macroid, I need to take an android.content.Context as my first constructor argument. In order to use Macroid inside of it, I'll need an implicit ActivityContext and potentially an implicit AppContext. That leads to this being the signature I have to write, which feels horribly redundant

class MyView(context: Context)(implicit ctx: ActivityContext, app: AppContext)
extends View(context)

I'd be happy to do the legwork on any changes to resolve these if you'd like, but the problems right now are high level enough that I figured it'd warrant a discussion before any pull requests.

[stanch]

Things require ActivityContext when they don't appear to need it

I’m pretty sure that creating widgets with the application context doesn’t work very well. It makes sense, because for GUI you need a GUI context, which pertains to the Activity, but not necessarily the Application. One exception here is a Service with the SYSTEM_ALERT_WINDOW permission (see a related thread on the mailing list).

If you have tried using just the application context and succeeded, I would be interested in your findings :)

Unable to use things which require AppContext when I only have an implicit ActivityContext

This thing also annoys me, but at the time I haven’t found a good way to unify the two. I wish Android separated the different Contexts from the start to emphasize the hidden restrictions and side-effects.

Should we have a more general trait that can be included for things with contexts?

The problem here is that if you have a general type like AnyContext, which is (implicitly) assignable from both AppContext and ActivityContext, and you have both AppContext and ActivityContext in the implicit scope, then the assignment is ambiguous. Unless you mean something else :)

The layout DSL doesn't play nicely with implicits

This one should be straightforward to fix in the macro, good point.

[sgrif]

Is there an example of where the AppContext being the actual application context instead of the activity is important? Would it make sense to have ActivityContext extend from AppContext?

[sgrif]

Oh and btw this line made me see the wisdom in having ActivityContext be a thing you can specify.

ctx.get.getTheme.resolveAttribute(styleAttr, output, true)

[stanch]

Is there an example of where the AppContext being the actual application context instead of the activity is important? Would it make sense to have ActivityContext extend from AppContext?

Generally, passing an AppContext (when applicable) is better than passing an ActivityContext, because it will never die on you or leak memory.

[sgrif]

I've been thinking about this a little bit more. Since widget creation requires an ActivityContext, and we're pretty sure that in general attempting to do it outside of that context is a bad idea, would we be able to define a Contexts[View] which assumes that getContext is an Activity (unsafely, at runtime, but I don't think there's another way)?

[stanch]

What if it’s a Service (see above)?

[dant3]

You can legally create a View with Application, or Service context. android.permission.SYSTEM_ALERT_WINDOW permission is only required for [WindowManager.addView(View,LayoutParams)](http://developer.android.com/reference/android/view/ViewManager.html#addView%28android.view.View, android.view.ViewGroup.LayoutParams%29) call. So if you rely on a fact that Context is Activity, this is your internal app logic, and is not some sort of general case.

[stanch]

I think we should use something like this instead of the current contexts:

case class ContextWrapper(originalContext: Option[WeakReference[Context]], appContext: Context) {
  def get = originalContext.flatMap(_.get) getOrElse appContext
}

object ContextWrapper {
  def apply(activity: Activity): ContextWrapper =
    ContextWrapper(Some(WeakReference(activity)), activity.getApplicationContext)

  def apply(app: Application): ContextWrapper =
    ContextWrapper(None, app)

  def apply(service: Service): ContextWrapper =
    ContextWrapper(Some(WeakReference(service)), service.getApplicationContext)

  // + a method for FragmentApi...
}

So the user of this class can choose between wrapper.get (which is not very trustworthy, because it will default to Application if the original context died) and wrapper.appContext (which is trustworthy, but limited to Application).

Thoughts?

[dant3]

So the user of this class can choose between wrapper.get (which is not very trustworthy, because it will default to Application if the original context died) and wrapper.appContext (which is trustworthy, but limited to Application).

The name feels too much general given it is not trustworthy. In some cases getting app context makes big difference, for example then starting an Activity, different set of flags can be required. Name like getAny feels more safe in that sense. As long as there are clear methods to get one context or another, and just an easy way to get any of them for places there you don't actually care, I like it.

[stanch]

@dant3 That’s a fair point. What about this?

class ContextWrapper(originalContextRef: Option[WeakReference[Context]], appContext: Context) {
  def original: Option[Context] = ...
  def app: Context = appContext
  def bestAvailable: Context = original getOrElse app
}

[stanch]

Now that I think of it, originalContextRef does not have to be an Option. We can always supply it, even if it’s the same as appContext. And it can be made available publicly as a WeakReference for more clarity:

case class ContextWrapper(original: WeakReference[Context], application: Context) {
  def getOriginal: Context = original.get.get
  def bestAvailable: Context = original.get getOrElse app

  def originalIsActivity = original.get.exists(_.isInstanceOf[Activity])
  def originalIsService  = original.get.exists(_.isInstanceOf[Service])
}

object ContextWrapper {
  ...
  def apply(app: Application): ContextWrapper =
      ContextWrapper(WeakReference(app), app)
  ...
}

[dant3]

@dant3 That’s a fair point. What about this?

Now this is fair & clean.

And it can be made available publicly as a WeakReference for more clarity:

But getOriginal returns a Context now, not WeakReference[Context], not Option[Context]? Feels like NPE point, given that you store a WeakReference[Context].

[dant3]

  def originalIsActivity = original.get.exists(_.isInstanceOf[Activity])
  def originalIsService  = original.get.exists(_.isInstanceOf[Service])

Maybe there should be a subclasses, with getOriginal: Option[Activity] and so on?

[stanch]

But getOriginal returns a Context now, not WeakReference[Context], not Option[Context]? Feels like NPE point, given that you store a WeakReference[Context].

Right, but that’s the same for Option#get, Try#get, etc. And for the current ActivityContext#get, so it’s a drop-in replacement. You can always use ContextWrapper#original instead if you want to handle failure. What do you think?

Maybe there should be a subclasses, with getOriginal: Option[Activity] and so on?

sealed trait ContextWrapper[C <: Context] {
  def original: WeakReference[C]
  def application: Context

  def getOriginal: C = original.get.get
  def bestAvailable: Context = original.get getOrElse application

  def isActivity: Boolean
  def isService: Boolean
  def isApplication: Boolean
}

case class ActivityContextWrapper(original: WeakReference[Activity], application: Context)
  extends ContextWrapper[Activity] {
  def isActivity = true
  def isService = false
  def isApplication = false
}

case class ServiceContextWrapper(original: WeakReference[Service], application: Context)
  extends ContextWrapper[Service] {
  def isActivity = false
  def isService = true
  def isApplication = false
}

case class ApplicationContextWrapper(original: WeakReference[Application], application: Context)
  extends ContextWrapper[Application] {
  def isActivity = false
  def isService = false
  def isApplication = true
}

object ContextWrapper {
  ...
  def apply(activity: Activity): ContextWrapper =
      ActivityContextWrapper(WeakReference(activity), activity.getApplicationContext)
  ...
}

[stanch]

Hey @sgrif, I believe my recent commit addressed your points 1 through 3, so we have one left:

• Things require ActivityContext when they don’t appear to need it.
• Unable to use things which require AppContext when I only have an implicit ActivityContext.
• Should we have a more general trait that can be included for things with contexts?
• The layout DSL doesn’t play nicely with implicits