Update the behavior of the cached undo/redo stack #51644
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
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Size Change: +714 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
Will this also unblock #37171? |
|
@Mamaduka It will not solve it but will provide us a way to do it yes. |
paulopmt1
approved these changes
Jun 26, 2023
There was a problem hiding this comment.
Tested blocks, images, templates, and text edition and everything worked as expected.
it's possible to have calls to editEntityRecords updating non-transient properties like "meta" or "title"
That's great to hear! I'm planning to tackle this one in my next 20% time to see if I really understand undo/redo now 😅
| By default all calls to `editEntityRecord` are considered "non-cached" unless the `isCached` option is passed as true. Example: | ||
|
|
||
| ```js | ||
| wp.data.dispatch( 'core' ).editEntityRecord( 'postType', 'post', 1, { title: 'Hello World' }, { isCached: true } ); |
There was a problem hiding this comment.
Thanks for that example!
ellatrix
reviewed
Jul 3, 2023
| @@ -275,7 +275,7 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) { | |||
| const edits = { blocks: newBlocks, selection }; | |||
| registry.batch( () => { | |||
| updateFootnotes( edits.blocks ); | |||
| editEntityRecord( kind, name, id, edits ); | |||
| editEntityRecord( kind, name, id, edits, { isCached: false } ); | |||
There was a problem hiding this comment.
@youknowriad I rebase this PR to try it with footnotes, but undo still doesn't seem batched. Do we need to update the editEntityRecord call in useEntityProp as well?
There was a problem hiding this comment.
That change is not correct. For this PR to only create one undo level for footnotes, you need one of the editEntityRecord calls (either footnotes one or actual change) to be isChached: true (isCached: false is the default)
There was a problem hiding this comment.
Oh, sorry. I just rebased with the previous change.
ellatrix
reviewed
Jul 4, 2023
There was a problem hiding this comment.
If you reorder two paragraph blocks with a footnote in each, undo will now revert the change as a whole correctly, but only after you undo, redo, and undo again. Somehow on the first time it messes up. 🤔
If a page already has footnotes, and you add another one, undoing the addition right away will only remove the footnote from the block. But somehow when you press redo, then undo again, it will undo the entire change. So same problem happening here.
But if you start off with no footnotes, and a footnotes block needs to be inserted, the first undo seems to do nothing, and consecutive redo/undo does it in two steps. Not sure why, since everything is batched:
gutenberg/packages/block-library/src/footnotes/format.js
Lines 46 to 100 in 1be8fa3
|
I'm happy to solve the footnotes undo as well, but just wanted to note that this initial PR was not meant to solve the footnotes behavior. It just gives us the tools to do so. I can include in the PR though. |
|
@youknowriad Sorry, didn't meant to derail your PR :D |
|
No worries @ellatrix in the last commit I went ahead and fixed footnotes undo/redo. There's one small remaining issue when we insert the first initial footnote, there's one undo level for the footnote addition and one for the block addition. That's because |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
@ellatrix I've just realized that you were already batching these two actions but it was still resulting in two onChange calls. The reason was a bug in nested |
youknowriad
added
the
[Type] Bug
ellatrix
reviewed
Jul 5, 2023
| selection, | ||
| content: ( { blocks: blocksForSerialization = [] } ) => | ||
| __unstableSerializeAndClean( blocksForSerialization ), | ||
| ...updateFootnotes( newBlocks ), |
There was a problem hiding this comment.
Oh interesting, I didn't know there was a key for meta.
ellatrix
approved these changes
Jul 5, 2023
tellthemachines
pushed a commit
that referenced
this pull request
Jul 7, 2023
Co-authored-by: Ella van Durpe <ella@vandurpe.com>
|
I just cherry-picked this PR to the update/first-pre-beta-4 branch to get it included in the next release: ed76244 |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
added a commit
that referenced
this pull request
Jul 7, 2023
* add hint to show template part move (#52395) * Page Content Focus: Ignore page content within a Query Loop block (#52351) * Block Editor: Pass context and className props to editor.BlockEdit filter * Page Content Focus: Ignore content blocks that are within a Query Loop * Patterns: stop endless snackbars appearing (#52012) * Patterns: Distinguish between theme patterns and template parts in category list (#52382) * Allow opt out of auto-creation of Navigation fallback (#52319) * Update welcome guide copy (#52282) * Patterns: Update pattern copy (#52340) * Post Title: The changes should be reflected when previewing a post (#52369) * Update fixed block toolbar (#52123) * update the icons for expanding and collapsing the fixed block toolbar * hide the tools selector item in document tools for fixed toolbar preference * reveal undo, redo and list view buttons * tweaks for show icon labels and hide zoom out for top toolbar option * improve the responsiveness of the fixed block toolbar * remove the overflow rule - bad experiment * update top toolbar test with the new label for buttons * update the toolbar tests to account for moving the collapse button * Drop PHP 5.6 CI jobs (#52345) * Remove PHP 5.6 PHPUnit CI job * Raise version in phpcs / WPCS * Patterns: Add handling of sync status to the wp-admin patterns list page (#52346) * Exit template focus when opening the W menu (#52235) * wrap buttons (#52249) * Update the behavior of the cached undo/redo stack (#51644) Co-authored-by: Ella van Durpe <ella@vandurpe.com> * Adjust top position (#52248) * Patterns: add a hint about the rename of reusable blocks to menu and inserter (#51771) Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> * Site Editor: update headings hierarchy in the 'Manage all' screens (#52271) * This commit: - updates heading levels on the template and template part pages - passes a `level` prop to Header from Page * update h2 size * Rolling back custom sizes * Rolling back unnecessary classNames There was a rogue space in trunk. Let's let it live * Check randomizer experiment is enabled before rendering button (#52306) * Hide parent selector when parent is 'disabled' or 'contentOnly' (#52264) * Fix incorrect aria-describedby attributes for theme patterns (#52263) * Patterns: rename sync_status and move to top level field on rest return instead of a meta field (#52146) * Fix default block dimensions visibility (#52256) * core/heading * core/details * core/list * core/table * core/video * core/verse * core/social-links * core/site-title * core/site-tagline * core/site-logo * core/post-time-to-read * core/gallery * core/code * core/categories * core/audio * core/archives * Patterns: Display all custom template part areas in sidebar nav (#52355) * Revert phpcs back to PHP 5.6 (#52384) Reverts phpcs PHP compatibility version back to 5.6. * Check if experiment enabled fr this time (#52315) * Navigation: Remove one preloaded endpoint (#52115) * default to showing status (#52226) * Command palette: rename (#52153) * Revise use of “command menu” to “command palette”. Dropping "global" where it was used as well. * Find “command center” and replace with “command palette” * Image block and behaviors: Fix some warnings (#52109) * Fix first warning * Fix second warning - dividing a NaN * Turn off DFM for style book and style editing (#52117) * Add confirmation step when deleting a Template (#52236) * [Command Palette]: Remove suggestion for deleting templates/parts (#52168) * Update stepper styling in Home template details panel (#51972) * Update stepper styling * Remove !important * [Edit Post]: Add toggle fullscreen mode and list view commands (#52184) * Style Book: Show tabs and make blocks clickable when entering edit mode from the Styles menu (#52222) * Style Book: Show tabs and make blocks clickable when entering edit mode from the Styles menu * Move lines * !important (#52025) * Navigation in Site View: Readd the edit button (#52111) * Fix UnitControl crashing on regex control characters. Units are now escaped using `escapeRegExp` before concatenating them into a regular expression. Fixes #52211. --------- Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net> * Patterns: rename wp_block sync_status postmeta to wp_pattern_sync_status (#52232) --------- Co-authored-by: Kai Hao <kai@kaihao.dev> * Site Editor Frame: Ignore Spotlight in view mode (#52262) * Guide: Place focus on the guide's container instead of its first tabbable (#52300) * Guide: Place focus on the guide's container instead of its first tabbable * Update CHANGELOG * Post editor: Require confirmation before removing Footnotes (#52277) * Post editor: Require confirmation before removing Footnotes Context: #52176 * BlockRemovalWarningModal: Limit width to 40rem * Explain that footnotes are preserved. Add warning to Site Editor * Fix react-dropdown-menu version to avoid breaking change from one of … (#52356) * Fix react-dropdown-menu version to avoid breaking change from one if its dependencies. * Changelog update * move changelog entry to the right place * Update package-lock --------- Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: Robert Anderson <robert@noisysocks.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Tonya Mork <tonya.mork@automattic.com> Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: Ella van Durpe <ella@vandurpe.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net> Co-authored-by: Carlos Bravo <37012961+c4rl0sbr4v0@users.noreply.github.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Timothy Jacobs <timothy@ironbounddesigns.com> Co-authored-by: Kai Hao <kai@kaihao.dev> Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
|
@youknowriad, I'm experimenting with this updated behavior/new flag. I wanted to check if the following behavior is expected or if I miss understanding the feature. Gist for the caseI've got three edits and I want to "cache" the second edit; the first two should be stored in the history stack. Expected undo stack
What I see now
Example codeconst edits = [ 'one', 'two', 'three' ];
const postId = wp.data.select( 'core/editor' ).getCurrentPostId();
edits.forEach(( edit, index ) => {
// Cache the second edit.
const isCached = index === 1;
wp.data.dispatch( 'core' ).editEntityRecord( 'postType', 'post', postId, { title: edit }, { isCached } );
console.log( { edit, isCached } );
}); |
|
I'm not entirely sure what you want to achieve but here's the behavior: 1- When So we end up with I'd say it's arguable whether we should merge 2 with 1 or 3 here but in my changes, I just left what we had for a long time because that was a proven behavior. |
|
This was just a simplified example. I was experimenting with the "coalescing edits" implementation in the Thanks for the explanation! |
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.
What?
Until now, any change to what we call "transient properties" (like blocks, selection) was considered as a change that doesn't create an undo/redo step. This PR makes the assumption that this heuristic is wrong. IMO any change to "blocks" for instance should actually create an undo/redo step.
That said, we do have some smart behavior in places where we want sometimes to cache changes and only create an undo step after some time, for instance when typing quickly in a RichText component. For these cases, the
isCachedflag is the solution proposed by this PR.Also, this PR enables a new behavior to unblock #51201
Basically, with the new flag, it's possible to have calls to
editEntityRecordsupdating non-transient properties like "meta" or "title" and cache these changes and only create an undo/redo once for these changes potential consecutive changes.Testing Instructions
1- Open the post editor
2- Try doing a few different things
3- Make sure that undo/redo still behaves similarly to trunk.
(We also have e2e tests to validate that)