From aa22c90f2cc45771150abdf131b1004724dfb8c8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 1 Aug 2019 11:25:04 +0200 Subject: [PATCH 1/7] HistoryManager + unit tests --- src/editor/history.js | 101 ++++++++++++++++++++++++++++++ test/editor/history-test.js | 121 ++++++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+) create mode 100644 src/editor/history.js create mode 100644 test/editor/history-test.js diff --git a/src/editor/history.js b/src/editor/history.js new file mode 100644 index 0000000000..02f72bb5b4 --- /dev/null +++ b/src/editor/history.js @@ -0,0 +1,101 @@ +/* +Copyright 2019 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +export default class HistoryManager { + constructor() { + this._stack = []; + this._newlyTypedCharCount = 0; + this._currentIndex = -1; + this._changedSinceLastPush = false; + this._lastCaret = null; + } + + _shouldPush(inputType, diff) { + if (inputType === "insertText") { + if (diff.removed) { + // always append when removing text + return true; + } + if (diff.added) { + // only append after every 5th keystroke while typing + this._newlyTypedCharCount += diff.added.length; + return this._newlyTypedCharCount > 5; + } + } else { + return true; + } + } + + _pushState(model, caret) { + // remove all steps after current step + while (this._currentIndex < (this._stack.length - 1)) { + this._stack.pop(); + } + const parts = model.serializeParts(); + this._stack.push({parts, caret}); + this._currentIndex = this._stack.length - 1; + this._lastCaret = null; + this._changedSinceLastPush = false; + this._newlyTypedCharCount = 0; + } + + // needs to persist parts and caret position + tryPush(model, caret, inputType, diff) { + // ignore state restoration echos. + // these respect the inputType values of the input event, + // but are actually passed in from MessageEditor calling model.reset() + // in the keydown event handler. + if (inputType === "historyUndo" || inputType === "historyRedo") { + return false; + } + const shouldPush = this._shouldPush(inputType, diff); + if (shouldPush) { + this._pushState(model, caret); + } else { + this._lastCaret = caret; + this._changedSinceLastPush = true; + } + return shouldPush; + } + + canUndo() { + return this._currentIndex >= 1 || this._changedSinceLastPush; + } + + canRedo() { + return this._currentIndex < (this._stack.length - 1); + } + + // returns state that should be applied to model + undo(model) { + if (this.canUndo()) { + if (this._changedSinceLastPush) { + this._pushState(model, this._lastCaret); + } + this._currentIndex -= 1; + return this._stack[this._currentIndex]; + } + } + + // returns state that should be applied to model + redo() { + if (this.canRedo()) { + this._changedSinceLastPush = false; + this._currentIndex += 1; + return this._stack[this._currentIndex]; + } + } +} diff --git a/test/editor/history-test.js b/test/editor/history-test.js new file mode 100644 index 0000000000..13a145201b --- /dev/null +++ b/test/editor/history-test.js @@ -0,0 +1,121 @@ +/* +Copyright 2019 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import expect from 'expect'; +import HistoryManager from "../../src/editor/history"; + +describe('editor/history', function() { + it('push, then undo', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + const caret1 = {}; + const result1 = history.tryPush(model, caret1); + expect(result1).toEqual(true); + parts[0] = "hello world"; + history.tryPush(model, {}); + expect(history.canUndo()).toEqual(true); + const undoState = history.undo(model); + expect(undoState.caret).toEqual(caret1); + expect(undoState.parts).toEqual(["hello"]); + expect(history.canUndo()).toEqual(false); + }); + it('push, undo, then redo', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + history.tryPush(model, {}); + parts[0] = "hello world"; + const caret2 = {}; + history.tryPush(model, caret2); + history.undo(model); + expect(history.canRedo()).toEqual(true); + const redoState = history.redo(); + expect(redoState.caret).toEqual(caret2); + expect(redoState.parts).toEqual(["hello world"]); + expect(history.canRedo()).toEqual(false); + expect(history.canUndo()).toEqual(true); + }); + it('push, undo, push, ensure you can`t redo', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + history.tryPush(model, {}); + parts[0] = "hello world"; + history.tryPush(model, {}); + history.undo(model); + parts[0] = "hello world!!"; + history.tryPush(model, {}); + expect(history.canRedo()).toEqual(false); + }); + it('not every keystroke stores a history step', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + const firstCaret = {}; + history.tryPush(model, firstCaret); + const diff = {added: "o"}; + let keystrokeCount = 0; + do { + parts[0] = parts[0] + diff.added; + keystrokeCount += 1; + } while (!history.tryPush(model, {}, "insertText", diff)); + const undoState = history.undo(model); + expect(undoState.caret).toEqual(firstCaret); + expect(undoState.parts).toEqual(["hello"]); + expect(history.canUndo()).toEqual(false); + expect(keystrokeCount).toBeGreaterThan(2); + expect(keystrokeCount).toBeLessThan(20); + }); + it('keystroke that didn\'t add a step can undo', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + const firstCaret = {}; + history.tryPush(model, {}); + parts[0] = "helloo"; + const result = history.tryPush(model, {}, "insertText", {added: "o"}); + expect(result).toEqual(false); + expect(history.canUndo()).toEqual(true); + const undoState = history.undo(model); + expect(undoState.caret).toEqual(firstCaret); + expect(undoState.parts).toEqual(["hello"]); + }); + it('undo after keystroke that didn\'t add a step is able to redo', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + history.tryPush(model, {}); + parts[0] = "helloo"; + const caret = {last: true}; + history.tryPush(model, caret, "insertText", {added: "o"}); + history.undo(model); + expect(history.canRedo()).toEqual(true); + const redoState = history.redo(); + expect(redoState.caret).toEqual(caret); + expect(redoState.parts).toEqual(["helloo"]); + }); + it('overwriting text always stores a step', function() { + const history = new HistoryManager(); + const parts = ["hello"]; + const model = {serializeParts: () => parts.slice()}; + const firstCaret = {}; + history.tryPush(model, firstCaret); + const diff = {at: 1, added: "a", removed: "e"}; + const result = history.tryPush(model, {}, "insertText", diff); + expect(result).toEqual(true); + }); +}); From 98bc0d24f429b524998119215bae012ae7727572 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 1 Aug 2019 11:26:20 +0200 Subject: [PATCH 2/7] push changes to history manager --- src/components/views/elements/MessageEditor.js | 9 ++++++--- src/editor/model.js | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/components/views/elements/MessageEditor.js b/src/components/views/elements/MessageEditor.js index a121f3384e..bdbbcba026 100644 --- a/src/components/views/elements/MessageEditor.js +++ b/src/components/views/elements/MessageEditor.js @@ -20,6 +20,7 @@ import {_t} from '../../../languageHandler'; import PropTypes from 'prop-types'; import dis from '../../../dispatcher'; import EditorModel from '../../../editor/model'; +import HistoryManager from '../../../editor/history'; import {setCaretPosition} from '../../../editor/caret'; import {getCaretOffsetAndText} from '../../../editor/dom'; import {htmlSerializeIfNeeded, textSerialize} from '../../../editor/serialize'; @@ -134,7 +135,7 @@ export default class MessageEditor extends React.Component { return this.context.matrixClient.getRoom(this.props.editState.getEvent().getRoomId()); } - _updateEditorState = (caret) => { + _updateEditorState = (caret, inputType, diff) => { renderModel(this._editorRef, this.model); if (caret) { try { @@ -144,6 +145,7 @@ export default class MessageEditor extends React.Component { } } this.setState({autoComplete: this.model.autoComplete}); + this.historyManager.tryPush(this.model, caret, inputType, diff); } _onInput = (event) => { @@ -288,7 +290,7 @@ export default class MessageEditor extends React.Component { } componentDidMount() { - this.model = this._createEditorModel(); + this._createEditorModel(); // initial render of model this._updateEditorState(); // initial caret position @@ -317,7 +319,8 @@ export default class MessageEditor extends React.Component { parts = parseEvent(editState.getEvent(), partCreator); } - return new EditorModel( + this.historyManager = new HistoryManager(partCreator); + this.model = new EditorModel( parts, partCreator, this._updateEditorState, diff --git a/src/editor/model.js b/src/editor/model.js index 530aeef0b2..e1640c31ca 100644 --- a/src/editor/model.js +++ b/src/editor/model.js @@ -107,7 +107,7 @@ export default class EditorModel { const caretOffset = diff.at - removedOffsetDecrease + addedLen; const newPosition = this.positionForOffset(caretOffset, true); this._setActivePart(newPosition, canOpenAutoComplete); - this._updateCallback(newPosition); + this._updateCallback(newPosition, inputType, diff); } _setActivePart(pos, canOpenAutoComplete) { From 234404e5981b5d5195188bab6709ba6eecc32ea5 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 1 Aug 2019 11:27:09 +0200 Subject: [PATCH 3/7] add mod+z/y shortcuts, set editor state to what history manager returns --- .../views/elements/MessageEditor.js | 23 +++++++++++++++++++ src/editor/model.js | 5 ++++ 2 files changed, 28 insertions(+) diff --git a/src/components/views/elements/MessageEditor.js b/src/components/views/elements/MessageEditor.js index bdbbcba026..2a2872b2f0 100644 --- a/src/components/views/elements/MessageEditor.js +++ b/src/components/views/elements/MessageEditor.js @@ -34,6 +34,8 @@ import {MatrixClient} from 'matrix-js-sdk'; import classNames from 'classnames'; import {EventStatus} from 'matrix-js-sdk'; +const IS_MAC = navigator.platform.indexOf("Mac") !== -1; + function _isReply(mxEvent) { const relatesTo = mxEvent.getContent()["m.relates_to"]; const isReply = !!(relatesTo && relatesTo["m.in_reply_to"]); @@ -174,6 +176,27 @@ export default class MessageEditor extends React.Component { } _onKeyDown = (event) => { + const modKey = IS_MAC ? event.metaKey : event.ctrlKey; + // undo + if (modKey && event.key === "z") { + if (this.historyManager.canUndo()) { + const {parts, caret} = this.historyManager.undo(this.model); + // pass matching inputType so historyManager doesn't push echo + // when invoked from rerender callback. + this.model.reset(parts, caret, "historyUndo"); + } + event.preventDefault(); + } + // redo + if (modKey && event.key === "y") { + if (this.historyManager.canRedo()) { + const {parts, caret} = this.historyManager.redo(); + // pass matching inputType so historyManager doesn't push echo + // when invoked from rerender callback. + this.model.reset(parts, caret, "historyRedo"); + } + event.preventDefault(); + } // insert newline on Shift+Enter if (event.shiftKey && event.key === "Enter") { event.preventDefault(); // just in case the browser does support this diff --git a/src/editor/model.js b/src/editor/model.js index e1640c31ca..5c0b69bf03 100644 --- a/src/editor/model.js +++ b/src/editor/model.js @@ -90,6 +90,11 @@ export default class EditorModel { } } + reset(serializedParts, caret, inputType) { + this._parts = serializedParts.map(p => this._partCreator.deserializePart(p)); + this._updateCallback(caret, inputType); + } + update(newValue, inputType, caret) { const diff = this._diff(newValue, inputType, caret); const position = this.positionForOffset(diff.at, caret.atNodeEnd); From 9d49a5bb731b8fde8b13c9edc9dd9b4e5de2f31f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 1 Aug 2019 11:28:01 +0200 Subject: [PATCH 4/7] pass caret to history manager upon initial render otherwise caret is put at editor start when undoing last step --- src/components/views/elements/MessageEditor.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/components/views/elements/MessageEditor.js b/src/components/views/elements/MessageEditor.js index 2a2872b2f0..3d113d5223 100644 --- a/src/components/views/elements/MessageEditor.js +++ b/src/components/views/elements/MessageEditor.js @@ -315,9 +315,7 @@ export default class MessageEditor extends React.Component { componentDidMount() { this._createEditorModel(); // initial render of model - this._updateEditorState(); - // initial caret position - this._initializeCaret(); + this._updateEditorState(this._getInitialCaretPosition()); // attach input listener by hand so React doesn't proxy the events, // as the proxied event doesn't support inputType, which we need. this._editorRef.addEventListener("input", this._onInput, true); @@ -350,7 +348,7 @@ export default class MessageEditor extends React.Component { ); } - _initializeCaret() { + _getInitialCaretPosition() { const {editState} = this.props; let caretPosition; if (editState.hasEditorState()) { @@ -362,7 +360,7 @@ export default class MessageEditor extends React.Component { // otherwise, set it at the end caretPosition = this.model.getPositionAtEnd(); } - setCaretPosition(this._editorRef, this.model, caretPosition); + return caretPosition; } render() { From abde8b45d2e50f70f80312cdf8b7b4bc7b2fdebb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 1 Aug 2019 11:28:40 +0200 Subject: [PATCH 5/7] fix bug that prevented a line from being removed when undoing a newline --- src/editor/render.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/editor/render.js b/src/editor/render.js index 9d42bbe947..84e57c2a3f 100644 --- a/src/editor/render.js +++ b/src/editor/render.js @@ -176,7 +176,7 @@ export function renderModel(editor, model) { } }); if (lines.length) { - removeNextSiblings(editor.children[lines.length]); + removeNextSiblings(editor.children[lines.length - 1]); } else { removeChildren(editor); } From 07b2e51dce8e42dfd9a784c4f8605bf36ba23044 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 1 Aug 2019 16:10:21 +0200 Subject: [PATCH 6/7] put max step length in constant --- src/editor/history.js | 5 +++-- test/editor/history-test.js | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/editor/history.js b/src/editor/history.js index 02f72bb5b4..7a4dbd8e66 100644 --- a/src/editor/history.js +++ b/src/editor/history.js @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and limitations under the License. */ +export const MAX_STEP_LENGTH = 10; + export default class HistoryManager { constructor() { this._stack = []; @@ -30,9 +32,8 @@ export default class HistoryManager { return true; } if (diff.added) { - // only append after every 5th keystroke while typing this._newlyTypedCharCount += diff.added.length; - return this._newlyTypedCharCount > 5; + return this._newlyTypedCharCount > MAX_STEP_LENGTH; } } else { return true; diff --git a/test/editor/history-test.js b/test/editor/history-test.js index 13a145201b..5101a39e6c 100644 --- a/test/editor/history-test.js +++ b/test/editor/history-test.js @@ -15,7 +15,7 @@ limitations under the License. */ import expect from 'expect'; -import HistoryManager from "../../src/editor/history"; +import HistoryManager, {MAX_STEP_LENGTH} from "../../src/editor/history"; describe('editor/history', function() { it('push, then undo', function() { @@ -77,8 +77,7 @@ describe('editor/history', function() { expect(undoState.caret).toEqual(firstCaret); expect(undoState.parts).toEqual(["hello"]); expect(history.canUndo()).toEqual(false); - expect(keystrokeCount).toBeGreaterThan(2); - expect(keystrokeCount).toBeLessThan(20); + expect(keystrokeCount).toEqual(MAX_STEP_LENGTH + 1); // +1 before we type before checking }); it('keystroke that didn\'t add a step can undo', function() { const history = new HistoryManager(); From af3eebd0a615c6bdca7e77cff09ec73a0e40438b Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 2 Aug 2019 11:31:01 +0200 Subject: [PATCH 7/7] add undo steps after word boundary (or capped) when typing or removing --- src/editor/history.js | 42 +++++++++++++++++++++++++++++++------ test/editor/history-test.js | 27 ++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/src/editor/history.js b/src/editor/history.js index 7a4dbd8e66..6fd67d067c 100644 --- a/src/editor/history.js +++ b/src/editor/history.js @@ -23,19 +23,46 @@ export default class HistoryManager { this._currentIndex = -1; this._changedSinceLastPush = false; this._lastCaret = null; + this._nonWordBoundarySinceLastPush = false; + this._addedSinceLastPush = false; + this._removedSinceLastPush = false; } _shouldPush(inputType, diff) { - if (inputType === "insertText") { + // right now we can only push a step after + // the input has been applied to the model, + // so we can't push the state before something happened. + // not ideal but changing this would be harder to fit cleanly into + // the editor model. + const isNonBulkInput = inputType === "insertText" || + inputType === "deleteContentForward" || + inputType === "deleteContentBackward"; + if (diff && isNonBulkInput) { + if (diff.added) { + this._addedSinceLastPush = true; + } if (diff.removed) { - // always append when removing text + this._removedSinceLastPush = true; + } + // as long as you've only been adding or removing since the last push + if (this._addedSinceLastPush !== this._removedSinceLastPush) { + // add steps by word boundary, up to MAX_STEP_LENGTH characters + const str = diff.added ? diff.added : diff.removed; + const isWordBoundary = str === " " || str === "\t" || str === "\n"; + if (this._nonWordBoundarySinceLastPush && isWordBoundary) { + return true; + } + if (!isWordBoundary) { + this._nonWordBoundarySinceLastPush = true; + } + this._newlyTypedCharCount += str.length; + return this._newlyTypedCharCount > MAX_STEP_LENGTH; + } else { + // if starting to remove while adding before, or the opposite, push return true; } - if (diff.added) { - this._newlyTypedCharCount += diff.added.length; - return this._newlyTypedCharCount > MAX_STEP_LENGTH; - } } else { + // bulk input (paste, ...) should be pushed every time return true; } } @@ -51,6 +78,9 @@ export default class HistoryManager { this._lastCaret = null; this._changedSinceLastPush = false; this._newlyTypedCharCount = 0; + this._nonWordBoundarySinceLastPush = false; + this._addedSinceLastPush = false; + this._removedSinceLastPush = false; } // needs to persist parts and caret position diff --git a/test/editor/history-test.js b/test/editor/history-test.js index 5101a39e6c..4f227f74dd 100644 --- a/test/editor/history-test.js +++ b/test/editor/history-test.js @@ -79,6 +79,33 @@ describe('editor/history', function() { expect(history.canUndo()).toEqual(false); expect(keystrokeCount).toEqual(MAX_STEP_LENGTH + 1); // +1 before we type before checking }); + it('history step is added at word boundary', function() { + const history = new HistoryManager(); + const model = {serializeParts: () => parts.slice()}; + const parts = ["h"]; + let diff = {added: "h"}; + expect(history.tryPush(model, {}, "insertText", diff)).toEqual(false); + diff = {added: "i"}; + parts[0] = "hi"; + expect(history.tryPush(model, {}, "insertText", diff)).toEqual(false); + diff = {added: " "}; + parts[0] = "hi "; + const spaceCaret = {}; + expect(history.tryPush(model, spaceCaret, "insertText", diff)).toEqual(true); + diff = {added: "y"}; + parts[0] = "hi y"; + expect(history.tryPush(model, {}, "insertText", diff)).toEqual(false); + diff = {added: "o"}; + parts[0] = "hi yo"; + expect(history.tryPush(model, {}, "insertText", diff)).toEqual(false); + diff = {added: "u"}; + parts[0] = "hi you"; + + expect(history.canUndo()).toEqual(true); + const undoResult = history.undo(model); + expect(undoResult.caret).toEqual(spaceCaret); + expect(undoResult.parts).toEqual(["hi "]); + }); it('keystroke that didn\'t add a step can undo', function() { const history = new HistoryManager(); const parts = ["hello"];