Add abstract layer for displaying elements

[gkresic]

Instead of depending on AcceptsOneWidget and its AcceptsOneWidget.setWidget(IsWidget) mechanism, introduce abstract ActivityDisplay<V> which can be implemented to provide displaying methods for any widgetset.

Also, provide two simple ActivityDisplay implementations (WidgetActivityDisplay and ElementalActivityDisplay) for most common scenarios.

Specialized test cases are not needed, because ActivityManagerTest.MyDisplay is updated to directly implement ActivityDisplay<IsWidget> so this abstraction is already covered by tests.

[tbroyer]

Looks like it's never assigned, maybe it doesn't really matter if passing the same display should be a no-op or not (ActivityManager doesn't really care, all it cares about apparently is whether we go from null to non-null, or from non-null to null), but it's a breaking change.

This should be assigned in setDisplay, and tested for non-regression wrt c.g.g.activity.

[gkresic]

Looks like it's never assigned, maybe it doesn't really matter if passing the same display should be a no-op or not (ActivityManager doesn't really care, all it cares about apparently is whether we go from null to non-null, or from non-null to null), but it's a breaking change.

Indeed, I didn't see that. I saw it was comparing it (instance compare), so I took the safe route and decided to provide same wrapper for same delegate. If my 2 cents are worth anything, I wouldn't care about instancing new wrapper for each call since it results in much more readable code.

This should be assigned in setDisplay, and tested for non-regression wrt c.g.g.activity.

Yes, that was intention, but I forgot and since ActivityManager really doesn't care (like you pointed), it didn't matter in tests.

I'll initialize it, but will leave a note for possible simplification (with performance penalty, though).

[tbroyer]

Couldn't it be a local variable?

[gkresic]

Well, no, at least if we want to return same wrapper for each display. Note that it's initialized inside if, only if display is different from previous one. Otherwise, its left as an existing wrapper used in prior call.

[tbroyer]

Should have an @Override

[gkresic]

Correct.

[tbroyer]

Because that's what ActivityManager will call, it means the start(AcceptsOneWidget,EventBus) will receive a different AcceptsOnePanel each time it's started, even if it's actually the same underlying AcceptsOnePanel we passed to the ActivityManager. That panel will have been wrapped twice here: once to convert it to an ActivityDisplay in WidgetActivityManager, and then to converted it back to an AcceptsOneWidget here.
Maybe using an actual wrapper class and testing it here with instanceof would be better?

class AcceptsOneWidgetActivityDisplayWrapper {
  final AcceptsOneWidget panel;

  AcceptsOneWidgetActivityDisplayWrapper(AcceptsOneWidget panel) {
    this.panel = panel;
  }
}

AcceptsOneWidget panel;
if (display instanceof AcceptsOneWidgetActivityDisplayWrapper) {
  panel = ((AcceptsOneWidgetActivityDisplayWrapper) panel).panel;
} else {
  panel = (AcceptsOneWidget) display::show
}
start(panel, eventBus);

Unless we a) those multiple wrappers aren't a problem in practice (in terms of performance and/or code size) and b) don't care about the breaking change (any activity testing either the type of the panel or whether it's the same as in the previous start will break when migrated to this lib)

[gkresic]

Like I said, I would choose simplicity over microoptimization any time, but like you pointed out, this may actually break activities if they compare panels (didn't occur to me they may), so I agree this is safer. Will do.

[gkresic]

Looks like activities never had an opportunity to compare panels they got, since ActivityManager wraps every display/panel with new ProtectedDisplay every time Activity.start is invoked (see ActivityManager.tryStart).

So, (double) wrapping seems inevitable.

[tbroyer]

And on the other hand, that simplifies the code a lot, as setDisplay(AcceptsOnWidget panel) can then just call setDisplay(panel == null ? null : panel::setWidget) (i.e. you can remove the widgetDisplay and wrapperDisplay)

[tbroyer]

Should this extend or wrap?

• by extending, this allows any ActivityDisplay<IsWidget>, but breaks setDisplay(null) that now requires a explicit cast to choose an overload
• by wrapping, it cannot be used interchangeably with an ActivityManager<IsWidget> (similar to the CompatEventBus in gwt-event-compat), but ensures backwards compatibility

[gkresic]

Like you pointed out, either one is bad. Javadoc could alleviate issue with required cast of nulls, but it's far from optimal. What is your suggestion?

[tbroyer]

I would wrap, because I don't see a need for casting to ActivityManager<IsWidget>. And I would have a constructor that takes the ActivityManager<IsWidget> to be wrapped, and another constructor that would instantiate one, for ease of use. If one needs both an ActivityManager<IsWidget> and a WidgetActivityManager, they could then create the former and wrap it in the latter (similar to CompatEventBus)

[gkresic]

Wrapped.

I exposed all public methods from wrapped ActivityManager<IsWidget> together with wrapped object itself (via (getDelegateManager) and explained relationship to ActivityManager<IsWidget> in docs.