Fix multi-entity multi-property undo redo #50911
Conversation
youknowriad
force-pushed
the
fix/undo-redo
branch
from
017af93 to
b0a3292
Compare
|
Size Change: -17.7 kB (-1%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0b1bfff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5110632790
|
youknowriad
added
[Type] Bug
youknowriad
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
youknowriad
force-pushed
the
fix/undo-redo
branch
from
e2f3e17 to
18733ea
Compare
| expect( undoState ).toEqual( expectedUndoState ); | ||
|
|
||
| // Check that undo levels are created with the latest action, | ||
| // even if undone. | ||
| // Check that create after undo does nothing. |
There was a problem hiding this comment.
This is the only place where I disagree with the previous expectations/behavior.
| @@ -406,14 +407,14 @@ export const editEntityRecord = | |||
| export const undo = | |||
| () => | |||
| ( { select, dispatch } ) => { | |||
| const undoEdit = select.getUndoEdit(); | |||
| // Todo: we shouldn't have to pass "root" here. | |||
| const undoEdit = select( ( state ) => getUndoEdits( state.root ) ); | |||
There was a problem hiding this comment.
@adamziel @jsnajdr I've learned today that:
- you can call private selectors like that in "thunks"
- you can't (or at least I didn't see any way to do it) call
unlock( select ).somePrivateSelector()in "thunks". - The first approach is buggy because I had to pass
state.rootinstead of juststate(state.rootshould be just an internal thing, the store author shouldn't even know it exists).
Fixing that bug is going to be a breaking change. Maybe we can still "proxy" .root and deprecate it somehow though. I'm not fixing this in this unrelated PR but just wanted to let you know about this.
There was a problem hiding this comment.
Thunks themselves are a private implementation detail of the store. The user of the store doesn't see how a particular action is implemented.
Therefore, the thunk should be able to see the private selectors and actions without having to unlock anything. The select object passed into a thunk should be already "unlocked", it should expose all the private selectors and let you call select.getUndoEdits() directly.
The same applies to private actions and the dispatch object.
Note that the ability to pass functions to select and dispatch, i.e., calling them as select( state => state.foo ) or dispatch( { type: 'BAR' } ), is also unique to thunks -- see how the thunkArgs properties are constructed with Object.assign here. This allows the thunk to access the state directly and dispatch actions to the reducer directly.
If select doesn't expose private selectors, we need to fix the thunkArgs construction: instead of getSelectors() we need to get the "unlocked" selectors.
The first approach is buggy because I had to pass
state.rootinstead of juststate
It seems like this was intentional because we're passing store.__unstableOriginalGetState() to select() here. If we didn't want that, we could very easily pass store.getState(). Or was this an oversight that I and @adamziel didn't catch 2 years ago?
There was a problem hiding this comment.
If select doesn't expose private selectors, we need to fix the thunkArgs construction: instead of getSelectors() we need to get the "unlocked" selectors.
Yes, I tried without unlocking and it doesn't work, these things are not available.
It seems like this was intentional because we're passing store.__unstableOriginalGetState() to select() here. If we didn't want that, we could very easily pass store.getState(). Or was this an oversight that I and @adamziel didn't catch 2 years ago?
I'd argue that it's oversight because all selectors receive the "state" and not the "original state" with metadata (which is an internal thing added by the data module).
|
Reading your description of the undo stack data structure, I'm wondering if we shouldn't be storing diffs? Like: Then each edit creates one undo stack record, and doesn't need to amend the penultimate one. Going through the stack backward or forward would be applying the diffs in the desired direction. It's like having a classic diff: @@ property2
- value21
+ value22and then applying it either with The currently implemented format -- and this PR is only bugfixing it, not fundamentally changing it -- basically stores snapshots at each step. Only the snapshots are not complete, but compressed: they store only the modified properties. After modifying |
|
@jsnajdr It is possible indeed. But I think it's probably a change that is too big IMO. Worth noting that editing the penultimate edit was being done before too but in a buggy way. |
|
@jsnajdr I'll probably explore it separately to see what we get. |
| * | ||
| * @return The edit. | ||
| */ | ||
| export function getUndoEdits( state: State ): Optional< any > { |
There was a problem hiding this comment.
We know the return value is an array so Optional< unknown[] > should be better.
| pageUtils, | ||
| } ) => { | ||
| await page.getByRole( 'textbox', { name: 'Add title' } ).type( 'a' ); // First step. | ||
| await page.keyboard.press( 'Backspace' ); // Second step. |
There was a problem hiding this comment.
Could we also add a test for this for blocks? The title case is a bit different? Maybe we need some logic in RichText that treats Backspace changes differently from input, which normally only marks changes as persistent after 1s.
There was a problem hiding this comment.
Can you give me an example of test you have in mind. The issue fixed by this PR is hardly reproducible with blocks in trunk.
| @@ -436,107 +456,110 @@ let lastEditAction; | |||
| * @return {UndoState} Updated state. | |||
| */ | |||
| export function undo( state = UNDO_INITIAL_STATE, action ) { | |||
| const omitPendingRedos = ( currentState ) => { | |||
There was a problem hiding this comment.
Nit: why not use named functions for easier debugging?
There was a problem hiding this comment.
Function declarations in const f = (...) => syntax are equivalent to named functions. I don't remember when this started, but it's been some years. See for yourself, even in Safari:
(() => {
const foo = () => 42
const bar = foo
function logFunctionName(fn) {
console.log(fn.name)
}
logFunctionName(bar);
})() // logs 'foo'(But I agree that we should name our callbacks when possible.)
There was a problem hiding this comment.
Ah cool! So all these anon functions are anon because they're not assigned to a var?
| * @property {Object} [flattenedUndo] Flattened form of undo stack. | ||
| * @property {number} list The undo stack. | ||
| * @property {number} offset Where in the undo stack we are. | ||
| * @property {Object} cache Cache of unpersisted transient edits. |
There was a problem hiding this comment.
Sounds like that's just unpersisted edits, not really cache?
There was a problem hiding this comment.
It's a cache of "edits" that were not added to the "undo" stack yet.
| } | ||
| case 'UNDO': | ||
| case 'REDO': { | ||
| const nextState = appendCachedEditsToLastUndo( state ); |
There was a problem hiding this comment.
Why do we persist unpersisted changes on redo? Normally it should no longer be possible to redo once there are more changes made?
There was a problem hiding this comment.
In theory it's not going to do anything at all because when you call "undo", the "unpersisted changes" are persisted.
So whenever you have a "redo", there's probably no cached changes. The code is here for the "undo" case. (to share the same code) but I can add a check if needed.
|
@jsnajdr Here's the alternative exploration (on top of this branch) #51002 I'll be waiting for the tests to pass on that PR to merge into the current one. |
|
I've merged the other PR into this one and updated the PR description accordingly. We're now using the alternative "diff" format as suggested by @jsnajdr |
|
This is ready to ship. Any approvals/reviews? |
closes #12075
It's been sometime I didn't dig into the undo/redo reducer and while taking a deep look into it, I noticed a fundamental issue in how the stack is recorded and applied when undoing/redoing.
How Undo works ?
To understand the issue and the need for this PR to solve it, we first need to understand how the undo/redo works in Gutenberg.
In trunk, for any change done in the editor, a "set of edits" is added to a list of undos. Each edit, contain the following information: The identifier of the modified entity/record + the set of edits to apply when going (either back or forth) to that state. So the list resembles something like this:
We also keep track of a "pointer" to a particular "step" in this list. By default the pointer is always pointing at the last step (current step) and when calling "undo" it goes back by 1 step (and opposite for redo).
Each time this step is modified (undo or redo called), we retrieve the set of "edits" corresponding to that step and apply it.
How do these steps gets recorded
This is a bit complex (as there are edge cases) but for the regular case, the behavior is the following: Let's stay initially the "undo list" contained the following content:
The user in the UI edits property1 with value12, the list becomes:
The user in the UI edits property2 with value22 (let's also assume that the previous value of value2 was value21 but this was not in the undo list, it's just the saved value), the list becomes
Notice that for the second item (penultimate item) of the list, we actually updated the edits to store the previous value of property2. The reason we do this is because when "undoing" to that step, we need to restore the previous value, so we need to "edit" value2 to set the previous value.
And here's the bug, now let's say with used undo twice, we went to the initial step (property 1 is value1), and now we "redo" to move to the pointer to step2. What will happen now is that the edits of step2 will be applied, so
property2 set to value21but if you remember properly, in step2, property1 need to be set to value12 and it didn't happen. (This is the bug recorded in #12075)So what this tells us is that for each "step" in the undo list, we need to record both:
Multi-entity bug
Worth nothing that in trunk, the undo/redo suffer also from the "multi-entity" diff problem. Each diff can contain multiple properties (especially when using transient edits) but in theory "multi-entity" diffs are possible. So each "diff" should be a list of diffs instead.
Solution
One solution to this is to update the "diff" format to use a (from, to) tupples instead, so for the example above, we should have the following undo stack:
And when undoing, we use the pointer to retrieve the current diff and edit the entities using the "from" values. (Restore the from values), while for redos, we'd use the "to" values.
Now, this solves navigating up and down the list and we'll get the expected result (and works both for multi property and multi entities).
This is the main change in this PR
getUndoEdit/getRedoEdit, this PR contains a breaking change right now. Ideally these selectors should have been private, I'll find a way to solve this by using new private selectors and trying to return something similar in format for these two selectors and probably deprecate them.There are some extra simplifications that are possible to our undo/redo APIs but are not included in this PR.
Todo
Testing instructions