fix: Fix bug that could cause renders to clobber the undo stack - #10227
Open
gonfunko wants to merge 1 commit into
Open
fix: Fix bug that could cause renders to clobber the undo stack#10227gonfunko wants to merge 1 commit into
gonfunko wants to merge 1 commit into
Conversation
This file contains hidden or 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
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.
The basics
The details
Resolves
Fixes #8129
Proposed Changes
This PR fixes a bug that could result in the undo/redo stack state becoming invalid when an undo or redo triggered operations that (a) fired events and (b) performed a non-immediate render.
In the particular scenario described in the bug, undoing the collapse disabled recording undo events, called
setCollapsed(false)on the various blocks, which as a side effect queued a render of each of them, and then re-enabled recording undo events once the undo was done. However, because the renders were queued but not immediately executed, the render happened after the undo infrastructure had re-enabled recording undo events. The render then calledbumpNeighbours()on each rendered block, which fired move events, and since undo recording was enabled at that point,Workspace.fireChangeListener()cleared the redo stack. Thus, redoing the collapse became impossible.The render infrastructure already had handling for this problem with regard to event grouping – when a render is queued, the current event group is recorded, and the same group is set when the block in question bumps its neighbours. The fix was just to extend this to enabling/disabling recording undo events, so that if a render of a block is queued while recording undo events is disabled, the post-render bump of that block's neighbours will not record undo events, even if recording undo events has been turned back on after the render was queued.
This change was LLM-assisted. I heavily modified the generated tests, and reviewed the actual implementation and believe that the root cause and the fix make sense.