Replace EventDispatcher with an off the shelf solution

[cwisniew]

Is your feature request related to a problem? Please describe.
The EventDispatcher class should be replaced with an off the shelf event dispatcher.

Describe the solution you'd like
Replace EventDispatcher with Google EventBus.

Additional context
This required replacing current

• MapTool.ZoneEvent
• MapTool.PreferencesEvent

Used in

• ClientMethodHandler
• MapTool
• MapToolFrame
• PreferenceDialog
• CommandPanel
• HTMLFrameFactory
• ImpersonatePanel
• SelectionPanel
• WebAppInitiative

MapTool.setCampaign() should be changed to dispatch a new campaign event, replacing the below code

    AssetManager.updateRepositoryList();
    MapTool.getFrame().getCampaignPanel().reset();
    MapTool.getFrame().getGmPanel().reset();
    // overlay vanishes after campaign change
    MapTool.getFrame().getOverlayPanel().removeAllOverlays();
    UserDefinedMacroFunctions.getInstance().loadCampaignLibFunctions();

Eventually this can be expanded to include other events things like advancing initiative etc.

[cwisniew]

Should also replace
ZoneRenderer.updateAfterSelection which currently does

  public void updateAfterSelection() {
    MapTool.getFrame().getSelectionPanel().reset();
    MapTool.getFrame().getImpersonatePanel().resetIfNotImpersonating();
    HTMLFrameFactory.selectedListChanged();
    repaintDebouncer.dispatch();
  }

ModelChangeEvent which is fired off by

• Grid
• Zone

Used by

• UndoPerZone
• DrawPanelTreeModel
• HTMLFrameFactory
• AbstractMacroPanel
• ImpersonatePanel
• SelectionPanel
• InitiativePanel
• TokenPanel
• TokenPanelTreeModel
• ZoneMiniMapPanel
• ZoneRenderer
• ZoneView
• BaseModel
• Grid
• ModelChangeListener
• Zone
• WebAppInitiative
• WebTokenInfo

(yeah its a lot)

[Merudo]

Related: the classes CommandPanel and PlayerListModel implement the deprecated Observer interface, which should be replaced by the EventDispatcher.

[cwisniew]

Related: the classes CommandPanel and PlayerListModel implement the deprecated Observer interface, which should be replaced by the EventDispatcher.

MapToolFrame too

[bubblobill]

Why this particular Off the Shelf solution?

[cwisniew]

Why this particular Off the Shelf solution?

Because its simple(ish), works well, doesn't require external configuration to set up, there is enough documentation on it, and will be enough for out needs.

If you have an alternate suggestion that fits the above, I am willing to consider it.

[bubblobill]

http://reactivex.io?

[cwisniew]

@bubblobill looking at that it doesn't really seem to fit what we need.
For example I don't want to worry about subscribing to a new token or zone each time one is added, I also don't want to worry about unsubscribing when they are removed.
EventBus works well for this as when a new Token is added everything listing to Token changes will automatically listening to any changes to the new Token.

From the reading I have done you can use RxJava to have a such an EventBus like set up but its more complicated and you then don't really gain much for the extra complication.

[nmeier]

Is this a chance to rework how updates go through the full stack?

There's so much explicit dependency coded into the classes, e.g.

ZoneRenderer.modelChanged>MapTool.updateTokenTree, which updates two components explicitly

  /** Update tokenPanelTreeModel and the initiativePanel. */
  public void updateTokenTree() {
    if (tokenPanelTreeModel != null) {
      tokenPanelTreeModel.update();
    }
    if (initiativePanel != null) {
      initiativePanel.update();
    }
  }

and ServerMethodHandler calling methods straight into the UI, then broadcasting

  public void exposePCArea(GUID zoneGUID) {
    ZoneRenderer renderer = MapTool.getFrame().getZoneRenderer(zoneGUID);
    FogUtil.exposePCArea(renderer);
    server
        .getConnection()
        .broadcastCallMethod(
            ClientCommand.COMMAND.exposePCArea.name(), RPCContext.getCurrent().parameters);
  }

One would have to really untangle all that explicit dependency for a cleaner, clearly separated area of responsibility approach. That would also open up a future for a headless server which seems impossible now.

[cwisniew]

Is this a chance to rework how updates go through the full stack?

There's so much explicit dependency coded into the classes, e.g.

ZoneRenderer.modelChanged>MapTool.updateTokenTree, which updates two components explicitly

  /** Update tokenPanelTreeModel and the initiativePanel. */
  public void updateTokenTree() {
    if (tokenPanelTreeModel != null) {
      tokenPanelTreeModel.update();
    }
    if (initiativePanel != null) {
      initiativePanel.update();
    }
  }

At the moment, the initiative panel already listens for Zone Activations and Token Change events via ModelChangeListener. I believe the Author sprinkled in the updateTokenTree() every change they found wasn't updating the tree properly (and I know some of the not updated properly were due to threading issues). Hopefully once I have addressed the above the whole MapToolFrame.updateTokenTree() method can be deleted.

Really it would be nice to redo all of the InitiativePanel but where will I find the time :)

and ServerMethodHandler calling methods straight into the UI, then broadcasting

  public void exposePCArea(GUID zoneGUID) {
    ZoneRenderer renderer = MapTool.getFrame().getZoneRenderer(zoneGUID);
    FogUtil.exposePCArea(renderer);
    server
        .getConnection()
        .broadcastCallMethod(
            ClientCommand.COMMAND.exposePCArea.name(), RPCContext.getCurrent().parameters);
  }

One would have to really untangle all that explicit dependency for a cleaner, clearly separated area of responsibility approach. That would also open up a future for a headless server which seems impossible now.

Eventually yes, but for the first part just the above change though is big enough. Once that is done the next bit would be untangling things more in how the clients/server/UI interact.

[kwvanderlinde]

I can take a look at replacing the Observable/Observer stuff as well, maybe even take a crack at detangling updateAfterSelection().

One more thing in addition to what's already been mentioned here: it might be beneficial to also use an event bus instead of ModelChangeListener. A few perks I would expect to see:

1. Events would be typed instead of being a plain tag + an untyped argument.
2. Components (e.g., Selected panel) can generally listen for applicable events without having to make sure it's listening to the current zone. And things that affect tokens could be responded to without having to subscribe to each token in a zone (e.g., when a token light changes, ZoneRenderer/ZoneView could flush its lighting/vision caches).
3. Model types don't have to maintain a list of listeners.

[Phergus]

You've got my vote.

[cwisniew]

It's got my vote too :)
This was the intention to move everything that makes sense to the EventBus for the reasons you listed above, such as type safety and decoupling. I was replacing things as I changed them and using them for new items. I am certainly not going to complain if others want to help with refactoring :)

[kwvanderlinde]

Awesome, glad we're all thinking the same thing!

[kwvanderlinde]

Just discovered this little nugget: google/guava#1630 (comment)

I'm closing this issue because we are no longer going to be making changes to EventBus other than important bug fixes. The reasons we're discouraging it and some alternatives are listed in the EventBus javadoc.

[thelsing]

So recommendation seem so to be to use DI with rxjava. Last time I looked at java DI frameworks I dagger2 looked best to me.
IMO we have quite a bit other places in the code where DI would be nice.

[kwvanderlinde]

I do believe the event bus model is still the most appealing for us, particularly for this reason articulated above:

For example I don't want to worry about subscribing to a new token or zone each time one is added, I also don't want to worry about unsubscribing when they are removed.
EventBus works well for this as when a new Token is added everything listing to Token changes will automatically listening to any changes to the new Token.

It's just that the Guava devs have decided the model isn't so good and would rather use something else.

Whatever we move to in the future, we should still be aiming to use the event bus model inside MapTool. For now, Guava's EventBus can fill that role, we just probably don't want to rely on that particular implementation too heavily considering the project's status.

[thelsing]

I agree that some kind of bus seems to suit us. But we could also do this with rxjava as written in here or here. (Stolen from guava eventbus javadoc).

I also can live with keeping guava eventbus. We could also use otto or greenrobots eventbus.

Edit: otto is abandoned too.

[kwvanderlinde]

I'll just close this off as there should not be any change in behaviour. Bugs can be opened as they surface.