fix: filter undone event groups before moving them to the redo stack #7069
Conversation
ewpatton
changed the title
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
ewpatton
force-pushed
the
hotfix/undo-event-merge
branch
from
b922d0d to
33c1ab0
Compare
ewpatton
changed the title
There was a problem hiding this comment.
This LGTM :D
My plan is just to accept this as is since it fixes the bug, but I think John's other idea is also a good one:
I would also suggest testing to see if the output of filtering/merging contains any non-null events. If not, then the undo/redo behavior should be repeated, trying the next group of events, until something actually changes, otherwise it feels weird to press undo and nothing happens.
So if you want to add that while you're in the area I wanted to give you an opportunity to!
|
@BeksOmega Let me think about this a bit and get back to you. |
ewpatton
force-pushed
the
hotfix/undo-event-merge
branch
3 times, most recently
from
3bcd167 to
9529d45
Compare
|
@BeksOmega So what I decided would be the best approach is to filter any events at the top of the undo stack after firing events, possibly removing null events, so that the undo stack is always a minimal set of changes. Note that this implementation makes the assumption that event groups cannot be interleaved in the undo stack. |
There was a problem hiding this comment.
Approach LGTM! Not sure if we still need the previous change or not. Once I get clarification on that I'll get this merged =) Thank you again for the fix!
Wrt event groups, they can be interleaved, but we've never handled that for undo and redo (we just stop if we hit a group that's different than the current one, even if the "breaking group" is in the middle of a group). So I see no reason to start handling it now.
core/workspace.ts
Outdated
| @@ -634,12 +634,12 @@ export class Workspace implements IASTNodeLocation { | |||
| if (!event) continue; | |||
| events.push(event); | |||
| } | |||
| events = eventUtils.filter(events, redo); | |||
There was a problem hiding this comment.
Is this change still necessary now that we are filtering the undo stack itself?
There was a problem hiding this comment.
It may still be semantically correct to filter events being processed in the undo stack before pushing them to the redo stack. Otherwise, Blockly ends up dropping events from the queue that happen to still be on the stack, which ultimately leads to what happened in the initial issue since there is an event that never gets cleaned up from the stack.
There was a problem hiding this comment.
So is this just a precautionary thing? My reading is that they're getting filtered before being run + pushed to the undo stack anyway because they're being filtered before they're even pushed to the undo stack.
There was a problem hiding this comment.
Hmm. That is a fair point. I can push an update with only the second commit. If there continue to be occasional issues then we can always reapply it.
Change-Id: I21eb3dfaaf5089d373b0f7e810da657c9b541b8b
ewpatton
force-pushed
the
hotfix/undo-event-merge
branch
from
9529d45 to
a03bc2d
Compare
BeksOmega
changed the title
Use of the filter function in Workspace.prototype.undo has caused problems with repeated undo/redo (see issue google#7026), the originally-chosen fix for which was the addition (in PR google#7069) of code to fireNow to post-filter the .undoStack_ and .redoStack_ of any workspace that had just been involved in dispatching events. This apparently resolved the issue but added considerable additional complexity and made it difficlut to reason about how events are processed for undo/redo. Instead, since this filtering typically does nothing (at least nothing desirable), simply don't re-filter events on the undo stack before replaying them.
Use of the filter function in Workspace.prototype.undo has caused problems with repeated undo/redo (see issue google#7026), the originally-chosen fix for which was the addition (in PR google#7069) of code to fireNow to post-filter the .undoStack_ and .redoStack_ of any workspace that had just been involved in dispatching events. This apparently resolved the issue but added considerable additional complexity and made it difficlut to reason about how events are processed for undo/redo. Instead, since this filtering typically does nothing (at least nothing desirable), simply don't re-filter events on the undo stack before replaying them.
Use of the filter function in Workspace.prototype.undo has caused problems with repeated undo/redo (see issue google#7026), the originally-chosen fix for which was the addition (in PR google#7069) of code to fireNow to post-filter the .undoStack_ and .redoStack_ of any workspace that had just been involved in dispatching events. This apparently resolved the issue but added considerable additional complexity and made it difficult to reason about how events are processed for undo/redo. Instead, since this filtering typically does nothing (at least nothing desirable), simply don't re-filter events on the undo stack before replaying them.
Use of the filter function in Workspace.prototype.undo has caused problems with repeated undo/redo (see issue google#7026), the originally-chosen fix for which was the addition (in PR google#7069) of code to fireNow to post-filter the .undoStack_ and .redoStack_ of any workspace that had just been involved in dispatching events. This apparently resolved the issue but added considerable additional complexity and made it difficult to reason about how events are processed for undo/redo. Instead, since this filtering typically does nothing (at least nothing desirable), simply don't re-filter events on the undo stack before replaying them.
Use of the filter function in Workspace.prototype.undo has caused problems with repeated undo/redo (see issue #7026), the originally-chosen fix for which was the addition (in PR #7069) of code to fireNow to post-filter the .undoStack_ and .redoStack_ of any workspace that had just been involved in dispatching events. This apparently resolved the issue but added considerable additional complexity and made it difficult to reason about how events are processed for undo/redo. Instead, since this filtering typically does nothing (at least nothing desirable), simply don't re-filter events on the undo stack before replaying them.
The basics
npm run formatandnpm run lintThe details
Resolves
Fixes #7026
Proposed Changes
This PR filters events in a group prior to undoing them.
Behavior Before Change
Events being undone were not filtered before being put onto the redo stack. If there were multiple events that needed to be merged, this can result in inconsistent changes on the undo and redo stacks.
Behavior After Change
During undo, events within a group are passed through the Event.utils.filter function so that any sequence of events that result in a no-op will be removed from the undo/redo stack.
Reason for Changes
The issue was appropriately described in #7026.
Test Coverage
I ran
npm testand all tests passed. However, we may want to add more tests to evaluate the specific sequence of events that lead to #7026.Documentation
Additional Information