Replace Observable and ModelChangeListener with EventBus #3749
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of directly listening to `ObservableList`, we instead send out events for subscribers to consume. - ChatMessageAdded - PlayerConnected - PlayerDisconnected - ServerStopped This also means `MapTool` no longer needs to maintain a list of messages since the panel can directly listen for and maintain the list. `ObservableList` is no longer necessary and has been replaced with `List` where used.
The use of the `Observable`/`Observer` pattern was unnecessary here since the objects are created together anyways, and the observer was very simple.
With this change, subscribers no longer need to subscribe to a particular model object but can generically listen for event types that come from anywhere. In particular, this frees several components from having to ensure that their current zone is the one they are actually listening to. For any components that relied on a particular zone, these include a check that the event's zone matches the one in the component. For the most part, this change does not change behaviour beyond handling events in the new way. There are a few exceptions where components unconditionally responded to every model event whether or not it was actually applicable. These components are now more constrained in what they respond to: - `UndoPerZone` no longer subscribes to anything as it did nothing but log all of a zone's events. - `DrawPanelTreeModel` only subscribes to the `DrawableChanged` event. - `TokenPanelTreeModel` only subscribes to the various `Token*` events. - `ZoneRenderer` only subscribes to the events that it already handled explicitly. But it no longer calls `updateTokenTree()` and `repaintDebouncer.dispatch()` for all the other possible events. All the events that were fired this way were in the `Zone.Event` enum (which no longer exists). Here is the mapping from old to new: - `Zone.Event.TOKEN_ADDED`: `TokenAdded` - `Zone.Event.TOKEN_REMOVED`: `TokenRemoved` - `Zone.Event.TOKEN_CHANGED`: `TokenChanged` - `Zone.Event.GRID_CHANGED`: `GridChanged` - `Zone.Event.DRAWABLE_ADDED`: `DrawableAdded` - `Zone.Event.DRAWABLE_REMOVED`: `DrawableRemoved` - `Zone.Event.FOG_CHANGED`: `FogChanged` - `Zone.Event.LABEL_ADDED`: `LabelAdded` - `Zone.Event.LABEL_REMOVED`: `LabelRemoved` - `Zone.Event.LABEL_CHANGED`: `LabelChanged` - `Zone.Event.TOPOLOGY_CHANGED`: `TopologyChanged` - `Zone.Event.INITIATIVE_LIST_CHANGED`: `InitiativeListChanged` - `Zone.Event.BOARD_CHANGED`: `BoardChanged` - `Zone.Event.TOKEN_EDITED`: `TokenEdited` - `Zone.Event.TOKEN_MACRO_CHANGED`: `TokenMacroChanged` - `Zone.Event.TOKEN_PANEL_CHANGED`: `TokenPanelChanged` There were some existing events for EventBus that have been replaced with easier-to-use alternatives that are consistent with the other events: - `TokensAddedEvent`: `TokensAdded` - `TokensRemovedEvent`: `TokensRemoved` - `TokensChangedEvent`: `TokensChanged` In conjunction with that, the `TokenEventBusBridge` is no more. There was some important functionality in there to post token events in response to corresponding zone events, but that is now handled by posting the token events at the some point that the zone events are posted. This functionality was also added to `ServeMessageHandler` for good measure. Finally, since `BaseModel` only offered functionality for model events, it has been removed and existing uses (`Zone`, `Token`) no longer have a base class. Of course, `ModelChangeListener` is also gone.
Phergus
approved these changes
Nov 25, 2022
There was a problem hiding this comment.
Reviewed 52 of 52 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @kwvanderlinde)
|
@kwvanderlinde need a spotless check |
|
Formatting is fine (or so spotless says on my end). The JAR download just failed on Github:
Reran and everything checked out. |
kwvanderlinde
deleted the
refactor/1827-replace-observables-and-model-listeners
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
Relates to #1827
Description of the Change
This change moves the remaining from legacy event models to
EventBus. The two legacy event models are:Observable/ObserverBaseModel/ModelChangeListenerOne use of
Observable/Observerwas removed as being unnecessary (ChatTyperObserver), while the remaining uses are now covered by these new events:ChatMessageAddedPlayerConnectedPlayerDisconnectedServerStoppedWith that gone, our custom
ObservableListor the relatedPlayerListModelcould be deleted in favour of plain lists.The remainder and bulk of the change is in removing
ModelChangeListener, which amounts to replacing eachZone.Eventtag with a corresponding event class. There were some existing event classes (TokensAddedEvent,TokensRemovedEvent,TokensChangedEvent) thatTokenEventBusBridgetranslated to, but these have been replaced with new ones that are easier to build and consume, and which are consistent with how the other events works.With all the model events replaced, we could remove these types:
ModelChangeListenerBaseModelTokenEventBusBridgePossible Drawbacks
If done correctly, should be nothing. It just requires a bunch of testing
Documentation Notes
N/A (refactoring)
Release Notes
N/A
This change is