From b0a3292e3f4791b7b9f79dc7268ecb628dae4f6b Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Wed, 24 May 2023 15:31:21 +0100 Subject: [PATCH] Fix multi-entity multi-property undo redo --- packages/core-data/src/actions.js | 10 +- packages/core-data/src/reducer.js | 231 +++++++++++++------------ packages/core-data/src/selectors.ts | 18 +- packages/core-data/src/test/reducer.js | 78 ++++----- 4 files changed, 174 insertions(+), 163 deletions(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index ffae417a83cd13..2f4389ad699f1b 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -411,9 +411,8 @@ export const undo = return; } dispatch( { - type: 'EDIT_ENTITY_RECORD', - ...undoEdit, - meta: { isUndo: true }, + type: 'UNDO', + stackedEdits: undoEdit, } ); }; @@ -429,9 +428,8 @@ export const redo = return; } dispatch( { - type: 'EDIT_ENTITY_RECORD', - ...redoEdit, - meta: { isRedo: true }, + type: 'REDO', + stackedEdits: redoEdit, } ); }; diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index f04d543919b8c8..6363157393159d 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -183,6 +183,26 @@ export function themeGlobalStyleVariations( state = {}, action ) { return state; } +const withMultiEntityRecordEdits = ( reducer ) => ( state, action ) => { + if ( action.type === 'UNDO' || action.type === 'REDO' ) { + const { stackedEdits } = action; + + let newState = state; + stackedEdits.forEach( ( { kind, name, recordId, edits } ) => { + newState = reducer( newState, { + type: 'EDIT_ENTITY_RECORD', + kind, + name, + recordId, + edits, + } ); + } ); + return newState; + } + + return reducer( state, action ); +}; + /** * Higher Order Reducer for a given entity config. It supports: * @@ -196,6 +216,8 @@ export function themeGlobalStyleVariations( state = {}, action ) { */ function entity( entityConfig ) { return compose( [ + withMultiEntityRecordEdits, + // Limit to matching action type so we don't attempt to replace action on // an unhandled action. ifMatchingAction( @@ -411,8 +433,9 @@ export const entities = ( state = {}, action ) => { /** * @typedef {Object} UndoStateMeta * - * @property {number} offset Where in the undo stack we are. - * @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. */ /** @typedef {Array & UndoStateMeta} UndoState */ @@ -422,10 +445,7 @@ export const entities = ( state = {}, action ) => { * * @todo Given how we use this we might want to make a custom class for it. */ -const UNDO_INITIAL_STATE = Object.assign( [], { offset: 0 } ); - -/** @type {Object} */ -let lastEditAction; +const UNDO_INITIAL_STATE = { list: [], offset: 0 }; /** * Reducer keeping track of entity edit undo history. @@ -436,107 +456,103 @@ let lastEditAction; * @return {UndoState} Updated state. */ export function undo( state = UNDO_INITIAL_STATE, action ) { + const omitPendingRedos = ( currentState ) => { + return { + ...currentState, + list: currentState.list.slice( + 0, + currentState.offset || undefined + ), + offset: 0, + }; + }; + + const appendCachedEditsToLastUndo = ( currentState ) => { + if ( ! currentState.cache ) { + return currentState; + } + + let nextState = { + ...currentState, + list: [ ...currentState.list ], + }; + nextState = omitPendingRedos( nextState ); + let previousUndoState = nextState.list.pop(); + currentState.cache.forEach( ( edit ) => { + previousUndoState = appendEditsToStack( previousUndoState, edit ); + } ); + nextState.list.push( previousUndoState ); + + return { + ...nextState, + cache: undefined, + }; + }; + + const appendEditsToStack = ( + stack = [], + { kind, name, recordId, edits } + ) => { + const existingEditIndex = stack?.findIndex( + ( { kind: k, name: n, recordId: r } ) => { + return k === kind && n === name && r === recordId; + } + ); + + const nextStack = stack.filter( + ( _, index ) => index !== existingEditIndex + ); + nextStack.push( { + kind, + name, + recordId, + edits: { + ...( existingEditIndex !== -1 + ? stack[ existingEditIndex ].edits + : {} ), + ...edits, + }, + } ); + return nextStack; + }; + switch ( action.type ) { - case 'EDIT_ENTITY_RECORD': case 'CREATE_UNDO_LEVEL': - let isCreateUndoLevel = action.type === 'CREATE_UNDO_LEVEL'; - const isUndoOrRedo = - ! isCreateUndoLevel && - ( action.meta.isUndo || action.meta.isRedo ); - if ( isCreateUndoLevel ) { - action = lastEditAction; - } else if ( ! isUndoOrRedo ) { - // Don't lose the last edit cache if the new one only has transient edits. - // Transient edits don't create new levels so updating the cache would make - // us skip an edit later when creating levels explicitly. - if ( - Object.keys( action.edits ).some( - ( key ) => ! action.transientEdits[ key ] - ) - ) { - lastEditAction = action; - } else { - lastEditAction = { - ...action, - edits: { - ...( lastEditAction && lastEditAction.edits ), - ...action.edits, - }, - }; - } - } + return appendCachedEditsToLastUndo( state ); - /** @type {UndoState} */ - let nextState; - - if ( isUndoOrRedo ) { - // @ts-ignore we might consider using Object.assign({}, state) - nextState = [ ...state ]; - nextState.offset = - state.offset + ( action.meta.isUndo ? -1 : 1 ); - - if ( state.flattenedUndo ) { - // The first undo in a sequence of undos might happen while we have - // flattened undos in state. If this is the case, we want execution - // to continue as if we were creating an explicit undo level. This - // will result in an extra undo level being appended with the flattened - // undo values. - // We also have to take into account if the `lastEditAction` had opted out - // of being tracked in undo history, like the action that persists the latest - // content right before saving. In that case we have to update the `lastEditAction` - // to avoid returning early before applying the existing flattened undos. - isCreateUndoLevel = true; - if ( ! lastEditAction.meta.undo ) { - lastEditAction.meta.undo = { - edits: {}, - }; - } - action = lastEditAction; - } else { - return nextState; - } - } + case 'UNDO': + case 'REDO': { + const nextState = appendCachedEditsToLastUndo( state ); + return { + ...nextState, + offset: state.offset + ( action.type === 'UNDO' ? -1 : 1 ), + }; + } - if ( ! action.meta.undo ) { - return state; - } + case 'EDIT_ENTITY_RECORD': + const isCachedChange = Object.keys( action.edits ).every( + ( key ) => action.transientEdits[ key ] + ); - // Transient edits don't create an undo level, but are - // reachable in the next meaningful edit to which they - // are merged. They are defined in the entity's config. - if ( - ! isCreateUndoLevel && - ! Object.keys( action.edits ).some( - ( key ) => ! action.transientEdits[ key ] - ) - ) { - // @ts-ignore we might consider using Object.assign({}, state) - nextState = [ ...state ]; - nextState.flattenedUndo = { - ...state.flattenedUndo, - ...action.edits, + if ( isCachedChange ) { + return { + ...state, + cache: appendEditsToStack( state.cache, action ), }; - nextState.offset = state.offset; - return nextState; } - // Clear potential redos, because this only supports linear history. - nextState = - // @ts-ignore this needs additional cleanup, probably involving code-level changes - nextState || state.slice( 0, state.offset || undefined ); - nextState.offset = nextState.offset || 0; - nextState.pop(); - if ( ! isCreateUndoLevel ) { - nextState.push( { - kind: action.meta.undo.kind, - name: action.meta.undo.name, - recordId: action.meta.undo.recordId, - edits: { - ...state.flattenedUndo, - ...action.meta.undo.edits, - }, - } ); - } + let nextState = omitPendingRedos( state ); + nextState = appendCachedEditsToLastUndo( nextState ); + nextState = { ...nextState, list: [ ...nextState.list ] }; + const previousUndoState = nextState.list.pop(); + nextState.list.push( + appendEditsToStack( previousUndoState, { + kind: action.kind, + name: action.name, + recordId: action.recordId, + edits: action.meta.undo.edits, + } ) + ); // When an edit is a function it's an optimization to avoid running some expensive operation. // We can't rely on the function references being the same so we opt out of comparing them here. const comparisonUndoEdits = Object.values( @@ -546,15 +562,16 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { ( edit ) => typeof edit !== 'function' ); if ( ! isShallowEqual( comparisonUndoEdits, comparisonEdits ) ) { - nextState.push( { - kind: action.kind, - name: action.name, - recordId: action.recordId, - edits: isCreateUndoLevel - ? { ...state.flattenedUndo, ...action.edits } - : action.edits, - } ); + nextState.list.push( [ + { + kind: action.kind, + name: action.name, + recordId: action.recordId, + edits: action.edits, + }, + ] ); } + return nextState; } diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 7513d918109673..2959d827cc12f8 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -73,8 +73,8 @@ interface EntityConfig { kind: string; } -interface UndoState extends Array< Object > { - flattenedUndo: unknown; +interface UndoState { + list: Array< Object >; offset: number; } @@ -889,7 +889,9 @@ function getCurrentUndoOffset( state: State ): number { * @return The edit. */ export function getUndoEdit( state: State ): Optional< any > { - return state.undo[ state.undo.length - 2 + getCurrentUndoOffset( state ) ]; + return state.undo.list[ + state.undo.list.length - 2 + getCurrentUndoOffset( state ) + ]; } /** @@ -901,7 +903,9 @@ export function getUndoEdit( state: State ): Optional< any > { * @return The edit. */ export function getRedoEdit( state: State ): Optional< any > { - return state.undo[ state.undo.length + getCurrentUndoOffset( state ) ]; + return state.undo.list[ + state.undo.list.length + getCurrentUndoOffset( state ) + ]; } /** @@ -1142,11 +1146,7 @@ export const hasFetchedAutosaves = createRegistrySelector( export const getReferenceByDistinctEdits = createSelector( // This unused state argument is listed here for the documentation generating tool (docgen). ( state: State ) => [], - ( state: State ) => [ - state.undo.length, - state.undo.offset, - state.undo.flattenedUndo, - ] + ( state: State ) => [ state.undo.list.length, state.undo.offset ] ); /** diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index 63caf5fb83b177..0f43266dcc5df6 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -173,16 +173,13 @@ describe( 'undo', () => { // We need to "apply" the undo level here and build // the action to move the offset. lastEdits = - undoState[ - undoState.length + + undoState.list[ + undoState.list.length + undoState.offset - ( args[ 0 ] === 'isUndo' ? 2 : 0 ) - ].edits; + ][ 0 ].edits; action = { - type: 'EDIT_ENTITY_RECORD', - meta: { - [ args[ 0 ] ]: true, - }, + type: args[ 0 ] === 'isUndo' ? 'UNDO' : 'REDO', }; } else if ( args[ 0 ] === 'isCreate' ) { action = { type: 'CREATE_UNDO_LEVEL' }; @@ -194,8 +191,7 @@ describe( 'undo', () => { beforeEach( () => { lastEdits = {}; undoState = undefined; - expectedUndoState = []; - expectedUndoState.offset = 0; + expectedUndoState = { list: [], offset: 0 }; } ); it( 'initializes', () => { @@ -208,19 +204,19 @@ describe( 'undo', () => { // Check that the first edit creates an undo level for the current state and // one for the new one. undoState = createNextUndoState( { value: 1 } ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that the second and third edits just create an undo level for // themselves. undoState = createNextUndoState( { value: 2 } ); - expectedUndoState.push( createEditActionPart( { value: 2 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 2 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.push( createEditActionPart( { value: 3 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -229,11 +225,11 @@ describe( 'undo', () => { undoState = createNextUndoState( { value: 1 } ); undoState = createNextUndoState( { value: 2 } ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1 } ), - createEditActionPart( { value: 2 } ), - createEditActionPart( { value: 3 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1 } ) ], + [ createEditActionPart( { value: 2 } ) ], + [ createEditActionPart( { value: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); @@ -255,17 +251,18 @@ describe( 'undo', () => { // Check that another edit will go on top when there // is no undo level offset. undoState = createNextUndoState( { value: 4 } ); - expectedUndoState.push( createEditActionPart( { value: 4 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 4 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that undoing and editing will slice of // all the levels after the current one. undoState = createNextUndoState( 'isUndo' ); undoState = createNextUndoState( 'isUndo' ); + undoState = createNextUndoState( { value: 5 } ); - expectedUndoState.pop(); - expectedUndoState.pop(); - expectedUndoState.push( createEditActionPart( { value: 5 } ) ); + expectedUndoState.list.pop(); + expectedUndoState.list.pop(); + expectedUndoState.list.push( [ createEditActionPart( { value: 5 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -277,10 +274,10 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1, transientValue: 2 } ), - createEditActionPart( { value: 3 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1, transientValue: 2 } ) ], + [ createEditActionPart( { value: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -292,9 +289,9 @@ describe( 'undo', () => { // transient edits. undoState = createNextUndoState( { value: 1 } ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); @@ -305,18 +302,17 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState[ - expectedUndoState.length - 1 - ].edits.transientValue = 2; + expectedUndoState.list[ + expectedUndoState.list.length - 1 + ][ 0 ].edits.transientValue = 2; expect( undoState ).toEqual( expectedUndoState ); - // Check that undo levels are created with the latest action, - // even if undone. + // Check that create after undo does nothing. undoState = createNextUndoState( { value: 3 } ); undoState = createNextUndoState( 'isUndo' ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.pop(); - expectedUndoState.push( createEditActionPart( { value: 3 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 3 } ) ] ); + expectedUndoState.offset = -1; expect( undoState ).toEqual( expectedUndoState ); } ); @@ -328,9 +324,9 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( 'isUndo' ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1, transientValue: 2 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1, transientValue: 2 } ) ] ); expectedUndoState.offset--; expect( undoState ).toEqual( expectedUndoState ); @@ -341,7 +337,7 @@ describe( 'undo', () => { undoState = createNextUndoState(); undoState = createNextUndoState( { value } ); undoState = createNextUndoState( { value: () => {} } ); - expectedUndoState.push( createEditActionPart( { value } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); } );