Move selection logic into a dedicated model #3866
Merged
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
This fixes two bugs where client selections were being cleared due to other clients moving tokens, and where the selection history was having redundant entries while missing others. The bulk of the change is to pull the selection logic out of `ZoneRenderer` and into a new `SelectionModel`. The `SelectionModel` is solely responsible for maintain selections and the selection undo history. A new `SelectionChanged` event allows components to react to selection changes without having to be explicitly notified.
Phergus
approved these changes
Mar 16, 2023
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @kwvanderlinde)
Phergus
approved these changes
Mar 16, 2023
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
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
Addresses #3759
Description of the Change
The primary motivation here is fixing the linked bug. There was also another bug discovered in the meantime where selection history was not being saved at the right times, resulting in both missing and duplicated entries in the history.
Secondary to that, I took the opportunity to clean up
ZoneRendererwith regards to selection. The current selection and selection history are now managed by theSelectionModelclass (there is one perZoneRenderer). In addition to querying the current selection,SelectionModelalso provides these bulk operations:replaceSelection(): deselect all tokens and then select the provided tokens.addTokensToSelection(): keep the current selection and add more tokens to it.removeTokensFromSelection(): keep the current selection but deselect the provided tokens.undoSelection(): revert selection to a previous state.These operations guarantee that history is modified consistently. They also communicate selection changes via a new
SelectionChangedevent sent through the event bus.The new event means we no longer need
ZoneRenderer.updateAfterSelection(), which needed to be called in ~30 places to keep the selection panel, impersonation panel and HTML views up-to-date. Instead this is now done automatically with each of the components being directly notified of the selection change.Possible Drawbacks
As usual, it's possible I messed up somewhere that I didn't test.
Documentation Notes
N/A, just a minor bug fix.
Release Notes
This change is