From daab8235c7c7e2c12d6251d0aa7698586c0a000e Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Feb 2018 11:24:02 +0100 Subject: [PATCH] Overwrite undo + editable signal --- blocks/rich-text/index.js | 77 ++++++++------- blocks/rich-text/provider.js | 1 + editor/components/provider/index.js | 4 +- editor/store/actions.js | 4 + editor/store/reducer.js | 25 ++++- editor/store/selectors.js | 3 +- editor/store/test/reducer.js | 66 ++++++++++++- editor/utils/with-history/index.js | 74 +++++++++++--- editor/utils/with-history/test/index.js | 122 ++++++++++++++---------- 9 files changed, 271 insertions(+), 105 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index a256fc21aa53f4..f154d0b53844bd 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -13,7 +13,6 @@ import { find, defer, noop, - throttle, } from 'lodash'; import { nodeListToReact } from 'dom-react'; import 'element-closest'; @@ -93,16 +92,17 @@ export class RichText extends Component { this.getSettings = this.getSettings.bind( this ); this.onSetup = this.onSetup.bind( this ); this.onChange = this.onChange.bind( this ); - this.throttledOnChange = throttle( this.onChange.bind( this ), 500 ); this.onNewBlock = this.onNewBlock.bind( this ); this.onNodeChange = this.onNodeChange.bind( this ); this.onKeyDown = this.onKeyDown.bind( this ); this.onKeyUp = this.onKeyUp.bind( this ); this.changeFormats = this.changeFormats.bind( this ); this.onSelectionChange = this.onSelectionChange.bind( this ); - this.maybePropagateUndo = this.maybePropagateUndo.bind( this ); + this.onPropagateUndo = this.onPropagateUndo.bind( this ); this.onPastePreProcess = this.onPastePreProcess.bind( this ); this.onPaste = this.onPaste.bind( this ); + this.onAddUndo = this.onAddUndo.bind( this ); + this.onCreateUndoLevel = this.onCreateUndoLevel.bind( this ); this.state = { formats: {}, @@ -142,16 +142,16 @@ export class RichText extends Component { } ); editor.on( 'init', this.onInit ); - editor.on( 'focusout', this.onChange ); editor.on( 'NewBlock', this.onNewBlock ); editor.on( 'nodechange', this.onNodeChange ); editor.on( 'keydown', this.onKeyDown ); editor.on( 'keyup', this.onKeyUp ); editor.on( 'selectionChange', this.onSelectionChange ); - editor.on( 'BeforeExecCommand', this.maybePropagateUndo ); + editor.on( 'BeforeExecCommand', this.onPropagateUndo ); editor.on( 'PastePreProcess', this.onPastePreProcess, true /* Add before core handlers */ ); editor.on( 'paste', this.onPaste, true /* Add before core handlers */ ); - editor.on( 'input', this.throttledOnChange ); + editor.on( 'input', this.onChange ); + editor.on( 'addundo', this.onAddUndo ); patterns.apply( this, [ editor ] ); @@ -223,23 +223,15 @@ export class RichText extends Component { /** * Handles an undo event from tinyMCE. * - * When user attempts Undo when empty Undo stack, propagate undo - * action to context handler. The compromise here is that: TinyMCE - * handles Undo until change, at which point `editor.save` resets - * history. If no history exists, let context handler have a turn. - * Defer in case an immediate undo causes TinyMCE to be destroyed, - * if other undo behaviors test presence of an input field. - * - * @param {UndoEvent} event The undo event as triggered by tinyMCE. + * @param {UndoEvent} event The undo event as triggered by TinyMCE. */ - maybePropagateUndo( event ) { + onPropagateUndo( event ) { const { onUndo } = this.context; - if ( onUndo && event.command === 'Undo' && ! this.editor.undoManager.hasUndo() ) { - defer( onUndo ); + const { command } = event; - // We could return false here to stop other TinyMCE event handlers - // from running, but we assume TinyMCE won't do anything on an - // empty undo stack anyways. + if ( onUndo && ( command === 'Undo' || command === 'Redo' ) ) { + defer( onUndo ); + event.preventDefault(); } } @@ -373,12 +365,22 @@ export class RichText extends Component { * Handles any case where the content of the tinyMCE instance has changed. */ onChange() { - if ( ! this.editor.isDirty() ) { + this.savedContent = this.getContent(); + this.props.onChange( this.savedContent ); + } + + onAddUndo( { lastLevel } ) { + if ( ! lastLevel ) { return; } - this.savedContent = this.state.empty ? [] : this.getContent(); - this.props.onChange( this.savedContent ); - this.editor.save(); + + this.onCreateUndoLevel(); + } + + onCreateUndoLevel() { + // Always ensure the content is up-to-date. + this.onChange(); + this.context.onCreateUndoLevel(); } /** @@ -506,6 +508,8 @@ export class RichText extends Component { return; } + this.onCreateUndoLevel(); + const forward = event.keyCode === DELETE; if ( this.props.onMerge ) { @@ -543,6 +547,7 @@ export class RichText extends Component { } event.preventDefault(); + this.onCreateUndoLevel(); const childNodes = Array.from( rootNode.childNodes ); const index = dom.nodeIndex( selectedNode ); @@ -555,6 +560,7 @@ export class RichText extends Component { this.props.onSplit( beforeElement, afterElement ); } else { event.preventDefault(); + this.onCreateUndoLevel(); if ( event.shiftKey || ! this.props.onSplit ) { this.editor.execCommand( 'InsertLineBreak', false, event ); @@ -683,28 +689,20 @@ export class RichText extends Component { this.setState( { formats, focusPosition, selectedNodeId: this.state.selectedNodeId + 1 } ); } - updateContent() { - const bookmark = this.editor.selection.getBookmark( 2, true ); - this.savedContent = this.props.value; - this.setContent( this.savedContent ); - this.editor.selection.moveToBookmark( bookmark ); - - // Saving the editor on updates avoid unecessary onChanges calls - // These calls can make the focus jump - this.editor.save(); - } - setContent( content = '' ) { this.editor.setContent( renderToString( content ) ); } getContent() { + if ( this.state.empty ) { + return []; + } + return nodeListToReact( this.editor.getBody().childNodes || [], createTinyMCEElement ); } componentWillUnmount() { this.onChange(); - this.throttledOnChange.cancel(); } componentDidUpdate( prevProps ) { @@ -715,7 +713,11 @@ export class RichText extends Component { this.props.value !== prevProps.value && this.props.value !== this.savedContent ) { - this.updateContent(); + const bookmark = this.editor.selection.getBookmark( 2, true ); + + this.savedContent = this.props.value; + this.setContent( this.savedContent ); + this.editor.selection.moveToBookmark( bookmark ); } } componentWillReceiveProps( nextProps ) { @@ -848,6 +850,7 @@ export class RichText extends Component { RichText.contextTypes = { onUndo: noop, canUserUseUnfilteredHTML: noop, + onCreateUndoLevel: noop, }; RichText.defaultProps = { diff --git a/blocks/rich-text/provider.js b/blocks/rich-text/provider.js index d8d608f5bb17ca..ede5951e8614a0 100644 --- a/blocks/rich-text/provider.js +++ b/blocks/rich-text/provider.js @@ -29,6 +29,7 @@ class RichTextProvider extends Component { RichTextProvider.childContextTypes = { onUndo: noop, + onCreateUndoLevel: noop, }; export default RichTextProvider; diff --git a/editor/components/provider/index.js b/editor/components/provider/index.js index 906092835d2c74..a0f556b3194281 100644 --- a/editor/components/provider/index.js +++ b/editor/components/provider/index.js @@ -19,7 +19,7 @@ import { /** * Internal Dependencies */ -import { setupEditor, undo, initializeMetaBoxState } from '../../store/actions'; +import { setupEditor, undo, createUndoLevel, initializeMetaBoxState } from '../../store/actions'; import store from '../../store'; /** @@ -105,10 +105,12 @@ class EditorProvider extends Component { // RichText provider: // // - context.onUndo + // - context.onCreateUndoLevel [ RichTextProvider, bindActionCreators( { onUndo: undo, + onCreateUndoLevel: createUndoLevel, }, this.store.dispatch ), ], diff --git a/editor/store/actions.js b/editor/store/actions.js index 806ed66f17ffe0..cf303bc36aee7c 100644 --- a/editor/store/actions.js +++ b/editor/store/actions.js @@ -294,6 +294,10 @@ export function undo() { return { type: 'UNDO' }; } +export function createUndoLevel() { + return { type: 'CREATE_UNDO_LEVEL' }; +} + /** * Returns an action object used in signalling that the blocks * corresponding to the specified UID set are to be removed. diff --git a/editor/store/reducer.js b/editor/store/reducer.js index e0dd31667c2745..5f0509b2a1a50c 100644 --- a/editor/store/reducer.js +++ b/editor/store/reducer.js @@ -14,6 +14,9 @@ import { mapValues, findIndex, reject, + includes, + keys, + isEqual, } from 'lodash'; /** @@ -114,7 +117,27 @@ export const editor = flow( [ combineReducers, // Track undo history, starting at editor initialization. - partialRight( withHistory, { resetTypes: [ 'SETUP_NEW_POST', 'SETUP_EDITOR' ] } ), + partialRight( withHistory, { + resetTypes: [ 'SETUP_NEW_POST', 'SETUP_EDITOR' ], + shouldOverwriteState( action, previousAction ) { + if ( ! includes( [ 'UPDATE_BLOCK_ATTRIBUTES', 'EDIT_POST', 'RESET_POST' ], action.type ) ) { + return false; + } + + if ( + previousAction && + action.type === 'UPDATE_BLOCK_ATTRIBUTES' && + action.type === previousAction.type + ) { + const attributes = keys( action.attributes ); + const previousAttributes = keys( previousAction.attributes ); + + return action.uid === previousAction.uid && isEqual( attributes, previousAttributes ); + } + + return true; + }, + } ), // Track whether changes exist, resetting at each post save. Relies on // editor initialization firing post reset as an effect. diff --git a/editor/store/selectors.js b/editor/store/selectors.js index 4b563f8bd6fcff..ae56e1d794eef4 100644 --- a/editor/store/selectors.js +++ b/editor/store/selectors.js @@ -94,7 +94,8 @@ export function isSavingMetaBoxes( state ) { * @return {boolean} Whether undo history exists. */ export function hasEditorUndo( state ) { - return state.editor.past.length > 0; + const { past, present } = state.editor; + return past.length > 1 || last( past ) !== present; } /** diff --git a/editor/store/test/reducer.js b/editor/store/test/reducer.js index 1d0041de79059c..02d5a5bf4e2d17 100644 --- a/editor/store/test/reducer.js +++ b/editor/store/test/reducer.js @@ -72,7 +72,7 @@ describe( 'state', () => { it( 'should return history (empty edits, blocksByUid, blockOrder), dirty flag by default', () => { const state = editor( undefined, {} ); - expect( state.past ).toEqual( [] ); + expect( state.past ).toEqual( [ state.present ] ); expect( state.future ).toEqual( [] ); expect( state.present.edits ).toEqual( {} ); expect( state.present.blocksByUid ).toEqual( {} ); @@ -819,6 +819,70 @@ describe( 'state', () => { expect( state.present.blocksByUid ).toBe( state.present.blocksByUid ); } ); } ); + + describe( 'withHistory', () => { + it( 'should overwrite present history if updating same attributes', () => { + let state; + + state = editor( state, { + type: 'RESET_BLOCKS', + blocks: [ { + uid: 'kumquat', + attributes: {}, + innerBlocks: [], + } ], + } ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + test: 1, + }, + } ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + test: 2, + }, + } ); + + expect( state.past ).toHaveLength( 2 ); + } ); + + it( 'should not overwrite present history if updating same attributes', () => { + let state; + + state = editor( state, { + type: 'RESET_BLOCKS', + blocks: [ { + uid: 'kumquat', + attributes: {}, + innerBlocks: [], + } ], + } ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + test: 1, + }, + } ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + other: 1, + }, + } ); + + expect( state.past ).toHaveLength( 3 ); + } ); + } ); } ); describe( 'currentPost()', () => { diff --git a/editor/utils/with-history/index.js b/editor/utils/with-history/index.js index f1af9e1f20573b..76aff66bc587b9 100644 --- a/editor/utils/with-history/index.js +++ b/editor/utils/with-history/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { includes } from 'lodash'; +import { includes, first, last, drop, dropRight } from 'lodash'; /** * Reducer enhancer which transforms the result of the original reducer into an @@ -14,46 +14,82 @@ import { includes } from 'lodash'; * @return {Function} Enhanced reducer. */ export default function withHistory( reducer, options = {} ) { + const initialPresent = reducer( undefined, {} ); + const initialState = { - past: [], - present: reducer( undefined, {} ), + // Past already contains record of present since changes are buffered in present. + past: [ initialPresent ], + present: initialPresent, future: [], }; + let lastAction; + return ( state = initialState, action ) => { const { past, present, future } = state; + const previousAction = lastAction; + const { + resetTypes = [], + shouldOverwriteState = () => false, + } = options; + + lastAction = action; switch ( action.type ) { case 'UNDO': - // Can't undo if no past - if ( ! past.length ) { - break; + // If there are changes in buffer, push buffer to the future. + if ( last( past ) !== present ) { + return { + past, + present: last( past ), + future: [ present, ...future ], + }; + } + + // Can't undo if no past. + // If the present "buffer" is the same as the last record, + // There is no further past. + if ( past.length < 2 ) { + return state; } + const newPast = dropRight( past ); + return { - past: past.slice( 0, past.length - 1 ), - present: past[ past.length - 1 ], + past: newPast, + present: last( newPast ), future: [ present, ...future ], }; - case 'REDO': - // Can't redo if no future + // Can't redo if no future. if ( ! future.length ) { - break; + return state; + } + + return { + past: [ ...past, first( future ) ], + present: first( future ), + future: drop( future ), + }; + + case 'CREATE_UNDO_LEVEL': + // Already has this level. + if ( last( past ) === present ) { + return state; } return { past: [ ...past, present ], - present: future[ 0 ], - future: future.slice( 1 ), + present, + future: [], }; } const nextPresent = reducer( present, action ); - if ( includes( options.resetTypes, action.type ) ) { + if ( includes( resetTypes, action.type ) ) { return { - past: [], + past: [ nextPresent ], present: nextPresent, future: [], }; @@ -63,6 +99,14 @@ export default function withHistory( reducer, options = {} ) { return state; } + if ( shouldOverwriteState( action, previousAction ) ) { + return { + past, + present: nextPresent, + future, + }; + } + return { past: [ ...past, present ], present: nextPresent, diff --git a/editor/utils/with-history/test/index.js b/editor/utils/with-history/test/index.js index d59dcfc39847f2..4e84092af718e4 100644 --- a/editor/utils/with-history/test/index.js +++ b/editor/utils/with-history/test/index.js @@ -1,121 +1,145 @@ +/** + * External dependencies + */ +import { last } from 'lodash'; + /** * Internal dependencies */ import withHistory from '../'; describe( 'withHistory', () => { - const counter = ( state = 0, { type } ) => ( - type === 'INCREMENT' ? state + 1 : state - ); + const counter = ( state = { count: 0 }, { type } ) => { + if ( type === 'INCREMENT' ) { + return { count: state.count + 1 }; + } + + if ( type === 'RESET' ) { + return { count: 0 }; + } + + return state; + }; + + const reducer = withHistory( counter, { + shouldOverwriteState: ( { type } ) => type === 'INCREMENT', + resetTypes: [ 'RESET_HISTORY' ], + } ); it( 'should return a new reducer', () => { - const reducer = withHistory( counter ); + const state = reducer( undefined, {} ); - expect( typeof reducer ).toBe( 'function' ); - expect( reducer( undefined, {} ) ).toEqual( { - past: [], - present: 0, + expect( state ).toEqual( { + past: [ { count: 0 } ], + present: { count: 0 }, future: [], } ); - } ); - it( 'should track history', () => { - const reducer = withHistory( counter ); + expect( last( state.past ) ).toBe( state.present ); + } ); + it( 'should track changes in present', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ 0 ], - present: 1, + past: [ { count: 0 } ], + present: { count: 1 }, future: [], } ); } ); - it( 'should perform undo', () => { - const reducer = withHistory( counter ); - + it( 'should create undo level if buffer is available', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'UNDO' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ { count: 0 }, { count: 1 } ], + present: { count: 1 }, + future: [], } ); - } ); - it( 'should not perform undo on empty past', () => { - const reducer = withHistory( counter ); + expect( state ).toBe( reducer( state, { type: 'CREATE_UNDO_LEVEL' } ) ); + } ); + it( 'should perform undo of buffer', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ { count: 0 } ], + present: { count: 0 }, + future: [ { count: 1 } ], } ); - } ); - it( 'should perform redo', () => { - const reducer = withHistory( counter ); + expect( last( state.past ) ).toBe( state.present ); + expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); + } ); + it( 'should perform undo of last level', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'UNDO' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ { count: 0 } ], + present: { count: 0 }, + future: [ { count: 1 } ], } ); - } ); - it( 'should not perform redo on empty future', () => { - const reducer = withHistory( counter ); + expect( last( state.past ) ).toBe( state.present ); + expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); + } ); + it( 'should perform redo', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); + state = reducer( state, { type: 'UNDO' } ); state = reducer( state, { type: 'REDO' } ); expect( state ).toEqual( { - past: [ 0 ], - present: 1, + past: [ { count: 0 }, { count: 1 } ], + present: { count: 1 }, future: [], } ); + + expect( last( state.past ) ).toBe( state.present ); + expect( state ).toBe( reducer( state, { type: 'REDO' } ) ); } ); it( 'should reset history by options.resetTypes', () => { - const reducer = withHistory( counter, { resetTypes: [ 'RESET_HISTORY' ] } ); - let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'RESET_HISTORY' } ); - state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ 1, 2 ], - present: 3, + past: [ { count: 1 } ], + present: { count: 1 }, future: [], } ); } ); - it( 'should return same reference if state has not changed', () => { - const reducer = withHistory( counter ); - const original = reducer( undefined, {} ); - const state = reducer( original, {} ); + it( 'should create history record for non buffer types', () => { + let state; + state = reducer( undefined, {} ); + state = reducer( state, { type: 'INCREMENT' } ); + state = reducer( state, { type: 'RESET' } ); - expect( state ).toBe( original ); + expect( state ).toEqual( { + past: [ { count: 0 }, { count: 1 } ], + present: { count: 0 }, + future: [], + } ); } ); } );