Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try a generic undo handler #4959

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 5 additions & 24 deletions blocks/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ export default class RichText extends Component {
editor.on( 'BeforeExecCommand', this.maybePropagateUndo );
editor.on( 'PastePreProcess', this.onPastePreProcess, true /* Add before core handlers */ );
editor.on( 'paste', this.onPaste, true /* Add before core handlers */ );
editor.on( 'input', this.onChange );

patterns.apply( this, [ editor ] );

Expand Down Expand Up @@ -358,21 +359,12 @@ export default class RichText extends Component {
}
}

fireChange() {
this.savedContent = this.getContent();
this.editor.save();
this.props.onChange( this.state.empty ? [] : this.savedContent );
}

/**
* Handles any case where the content of the tinyMCE instance has changed.
*/
onChange() {
// Note that due to efficiency, speed and low cost requirements isDirty may
// not reflect reality for a brief period immediately after a change.
if ( this.editor.isDirty() ) {
this.fireChange();
}
this.savedContent = this.state.empty ? [] : this.getContent();
this.props.onChange( this.savedContent );
}

/**
Expand Down Expand Up @@ -500,8 +492,6 @@ export default class RichText extends Component {
return;
}

this.fireChange();

const forward = event.keyCode === DELETE;

if ( this.props.onMerge ) {
Expand Down Expand Up @@ -680,19 +670,10 @@ export default class RichText extends Component {
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 ) {
if ( ! content ) {
content = '';
}

content = renderToString( content );
this.editor.setContent( content );
setContent( content = '' ) {
this.editor.setContent( renderToString( content ) );
}

getContent() {
Expand Down
76 changes: 75 additions & 1 deletion editor/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ import {
mapValues,
findIndex,
reject,
keys,
isString,
isArray,
isPlainObject,
every,
} from 'lodash';
import diff from 'fast-diff';

/**
* WordPress dependencies
Expand Down Expand Up @@ -94,6 +100,55 @@ function getFlattenedBlocks( blocks ) {
return flattenedBlocks;
}

/**
* Check whether two values are considered equivalent
* Two values are considered equivalent if the only
* possible difference between these two values is a character in a string.
*
* @param {*} value1
* @param {*} value2
*
* @return {boolean} Whether the values are equivalent.
*/
function areValuesEquivalent( value1, value2 ) {
if ( isString( value1 ) && isString( value2 ) ) {
const diffResult = diff( value1, value2 ).filter( ( [ type ] ) => type !== 0 );
if (
diffResult.length === 0 ||
( diffResult.length === 1 && diffResult[ 0 ][ 1 ].match( /\S/ ) !== null )
) {
return true;
}

// For some reason comparing text adding a char after a space is creating two diffs,
// let's run the diff again for more accurate results
if ( diffResult.length === 2 ) {
const diffResult2 = diff( diffResult[ 0 ][ 1 ], diffResult[ 1 ][ 1 ] ).filter( ( [ type ] ) => type !== 0 );
return diffResult2.length === 0 ||
( diffResult2.length === 1 && diffResult2[ 0 ][ 1 ].match( /\S/ ) !== null );
}

return false;
}

if ( isArray( value1 ) && isArray( value2 ) && value1.length === value2.length ) {
return every( value1, ( subvalue, key ) => areValuesEquivalent( subvalue, value2[ key ] ) );
}

if ( isPlainObject( value1 ) && isPlainObject( value2 ) ) {
const keys1 = keys( value1 );
const keys2 = keys( value2 );
return (
keys1.length === keys2.length &&
every( keys1, ( key, index ) =>
keys2[ index ] === key && areValuesEquivalent( value1[ key ], value2[ key ] )
)
);
}

return value1 === value2;
}

/**
* Undoable reducer returning the editor post state, including blocks parsed
* from current HTML markup.
Expand All @@ -114,7 +169,26 @@ 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: ( previous, next ) => {
if ( previous.type !== next.type || previous.type !== 'UPDATE_BLOCK_ATTRIBUTES' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could generalise this more? Say e.g. someone changes the post format to "link", but then immediate changes it back to "standard", I think we should avoid to create an undo level as well. Similarly we could have input fields outside block scope, which would still add records for each character change?

return false;
}

const previousAttributes = keys( previous.attributes );
const nextAttributes = keys( next.attributes );
if (
previousAttributes.length !== 1 ||
nextAttributes.length !== 1 ||
previousAttributes[ 0 ] !== nextAttributes[ 0 ]
) {
return false;
}

return areValuesEquivalent( previous.attributes[ previousAttributes[ 0 ] ], next.attributes[ nextAttributes[ 0 ] ] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

areValuesEquivalent sounds like an expensive calculation to execute on every action (an thus keystroke).

},
} ),

// Track whether changes exist, resetting at each post save. Relies on
// editor initialization firing post reset as an effect.
Expand Down
26 changes: 21 additions & 5 deletions editor/utils/with-history/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ import { includes } from 'lodash';
* Reducer enhancer which transforms the result of the original reducer into an
* object tracking its own history (past, present, future).
*
* @param {Function} reducer Original reducer.
* @param {?Object} options Optional options.
* @param {?Array} options.resetTypes Action types upon which to clear past.
* @param {Function} reducer Original reducer.
* @param {?Object} options Optional options.
* @param {?Array} options.resetTypes Action types upon which to clear past.
* @param {Function} options.shouldOverwriteState A function passed the previous and current action.
* Returning true means we don't create a new undo level
* and replace the previous last item.
*
* @return {Function} Enhanced reducer.
*/
Expand All @@ -18,10 +21,11 @@ export default function withHistory( reducer, options = {} ) {
past: [],
present: reducer( undefined, {} ),
future: [],
previousAction: null,
};

return ( state = initialState, action ) => {
const { past, present, future } = state;
const { past, present, future, previousAction } = state;

switch ( action.type ) {
case 'UNDO':
Expand All @@ -34,6 +38,7 @@ export default function withHistory( reducer, options = {} ) {
past: past.slice( 0, past.length - 1 ),
present: past[ past.length - 1 ],
future: [ present, ...future ],
previousAction: null,
};

case 'REDO':
Expand All @@ -46,6 +51,7 @@ export default function withHistory( reducer, options = {} ) {
past: [ ...past, present ],
present: future[ 0 ],
future: future.slice( 1 ),
previousAction: null,
};
}

Expand All @@ -56,17 +62,27 @@ export default function withHistory( reducer, options = {} ) {
past: [],
present: nextPresent,
future: [],
previousAction: null,
};
}

if ( present === nextPresent ) {
return state;
}

let newPast;
const { shouldOverwriteState = () => false } = options;
if ( past.length && previousAction && shouldOverwriteState( previousAction, action ) ) {
newPast = [ ...past.slice( 0, -1 ), present ];
} else {
newPast = [ ...past, present ];
}

return {
past: [ ...past, present ],
past: newPast,
present: nextPresent,
future: [],
previousAction: action,
};
};
}
5 changes: 5 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"element-closest": "2.0.2",
"escape-string-regexp": "1.0.5",
"eslint-plugin-wordpress": "git://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress.git#327b6bdec434177a6e841bd3210e87627ccfcecb",
"fast-diff": "1.1.2",
"hpq": "1.2.0",
"is-equal-shallow": "0.1.3",
"jed": "1.1.1",
Expand Down