From 967903673beb86aededf7ca08849ef93b68f8c80 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Wed, 13 Jun 2018 00:06:38 +0200 Subject: [PATCH] fixed undo/redo issues and implemented ability to manually flush the UndoManager --- examples/quill-cursors/index.js | 4 +- examples/quill/index.js | 4 +- examples/serviceworker/index.js | 2 +- src/Bindings/DomBinding/DomBinding.js | 94 +++++++++++++++++++++++---- src/Bindings/DomBinding/selection.js | 91 ++++++-------------------- src/Struct/Item.js | 9 ++- src/Types/YText/YText.js | 2 +- src/Util/UndoManager.js | 85 ++++++++++++++++++------ src/Util/relativePosition.js | 11 +++- test/y-array.tests.js | 2 +- 10 files changed, 191 insertions(+), 113 deletions(-) diff --git a/examples/quill-cursors/index.js b/examples/quill-cursors/index.js index 3b14ac9f..76b270f0 100644 --- a/examples/quill-cursors/index.js +++ b/examples/quill-cursors/index.js @@ -20,7 +20,7 @@ let quill = new Quill('#quill-container', { [{ header: [1, 2, false] }], ['bold', 'italic', 'underline'], ['image', 'code-block'], - [{ color: [] }, { background: [] }], // Snow theme fills in values + [{ color: [] }, { background: [] }], // Snow theme fills in values [{ script: 'sub' }, { script: 'super' }], ['link', 'image'], ['link', 'code-block'], @@ -31,7 +31,7 @@ let quill = new Quill('#quill-container', { } }, placeholder: 'Compose an epic...', - theme: 'snow' // or 'bubble' + theme: 'snow' // or 'bubble' }) let cursors = quill.getModule('cursors') diff --git a/examples/quill/index.js b/examples/quill/index.js index 87ec0de9..c84b9e6d 100644 --- a/examples/quill/index.js +++ b/examples/quill/index.js @@ -13,7 +13,7 @@ let quill = new Quill('#quill-container', { [{ header: [1, 2, false] }], ['bold', 'italic', 'underline'], ['image', 'code-block'], - [{ color: [] }, { background: [] }], // Snow theme fills in values + [{ color: [] }, { background: [] }], // Snow theme fills in values [{ script: 'sub' }, { script: 'super' }], ['link', 'image'], ['link', 'code-block'], @@ -21,7 +21,7 @@ let quill = new Quill('#quill-container', { ] }, placeholder: 'Compose an epic...', - theme: 'snow' // or 'bubble' + theme: 'snow' // or 'bubble' }) let yText = y.define('quill', Y.Text) diff --git a/examples/serviceworker/index.js b/examples/serviceworker/index.js index 4a0ba5a2..c2d50b31 100644 --- a/examples/serviceworker/index.js +++ b/examples/serviceworker/index.js @@ -35,7 +35,7 @@ Y({ toolbar: [ [{ size: ['small', false, 'large', 'huge'] }], ['bold', 'italic', 'underline'], - [{ color: [] }, { background: [] }], // Snow theme fills in values + [{ color: [] }, { background: [] }], // Snow theme fills in values [{ script: 'sub' }, { script: 'super' }], ['link', 'image'], ['link', 'code-block'], diff --git a/src/Bindings/DomBinding/DomBinding.js b/src/Bindings/DomBinding/DomBinding.js index 59ca87c8..87833c13 100644 --- a/src/Bindings/DomBinding/DomBinding.js +++ b/src/Bindings/DomBinding/DomBinding.js @@ -1,8 +1,9 @@ -/* global MutationObserver */ +/* global MutationObserver, getSelection */ +import { fromRelativePosition } from '../../Util/relativePosition.js' import Binding from '../Binding.js' import { createAssociation, removeAssociation } from './util.js' -import { beforeTransactionSelectionFixer, afterTransactionSelectionFixer } from './selection.js' +import { beforeTransactionSelectionFixer, afterTransactionSelectionFixer, getCurrentRelativeSelection } from './selection.js' import { defaultFilter, applyFilterOnType } from './filter.js' import typeObserver from './typeObserver.js' import domObserver from './domObserver.js' @@ -67,16 +68,25 @@ export default class DomBinding extends Binding { characterData: true, subtree: true }) + this._currentSel = null + document.addEventListener('selectionchange', () => { + this._currentSel = getCurrentRelativeSelection(this) + }) const y = type._y + this.y = y // Force flush dom changes before Type changes are applied (they might // modify the dom) this._beforeTransactionHandler = (y, transaction, remote) => { this._domObserver(this._mutationObserver.takeRecords()) - beforeTransactionSelectionFixer(y, this, transaction, remote) + this._mutualExclude(() => { + beforeTransactionSelectionFixer(this, remote) + }) } y.on('beforeTransaction', this._beforeTransactionHandler) this._afterTransactionHandler = (y, transaction, remote) => { - afterTransactionSelectionFixer(y, this, transaction, remote) + this._mutualExclude(() => { + afterTransactionSelectionFixer(this, remote) + }) // remove associations // TODO: this could be done more efficiently // e.g. Always delete using the following approach, or removeAssociation @@ -115,6 +125,67 @@ export default class DomBinding extends Binding { // TODO: apply filter to all elements } + _getUndoStackInfo () { + return this.getSelection() + } + + _restoreUndoStackInfo (info) { + this.restoreSelection(info) + } + + getSelection () { + return this._currentSel + } + + restoreSelection (selection) { + if (selection !== null) { + const { to, from } = selection + let shouldUpdate = false + /** + * There is little information on the difference between anchor/focus and base/extent. + * MDN doesn't even mention base/extent anymore.. though you still have to call + * setBaseAndExtent to change the selection.. + * I can observe that base/extend refer to notes higher up in the xml hierachy. + * Espesially for undo/redo this is preferred. If this becomes a problem in the future, + * we should probably go back to anchor/focus. + */ + const browserSelection = getSelection() + let { baseNode, baseOffset, extentNode, extentOffset } = browserSelection + if (from !== null) { + let sel = fromRelativePosition(this.y, from) + if (sel !== null) { + let node = this.typeToDom.get(sel.type) + let offset = sel.offset + if (node !== baseNode || offset !== baseOffset) { + baseNode = node + baseOffset = offset + shouldUpdate = true + } + } + } + if (to !== null) { + let sel = fromRelativePosition(this.y, to) + if (sel !== null) { + let node = this.typeToDom.get(sel.type) + let offset = sel.offset + if (node !== extentNode || offset !== extentOffset) { + extentNode = node + extentOffset = offset + shouldUpdate = true + } + } + } + if (shouldUpdate) { + browserSelection.setBaseAndExtent( + baseNode, + baseOffset, + extentNode, + extentOffset + ) + } + } + } + /** * Remove all properties that are handled by this class. */ @@ -130,11 +201,10 @@ export default class DomBinding extends Binding { super.destroy() } } - - /** - * A filter defines which elements and attributes to share. - * Return null if the node should be filtered. Otherwise return the Map of - * accepted attributes. - * - * @typedef {function(nodeName: String, attrs: Map): Map|null} FilterFunction - */ +/** + * A filter defines which elements and attributes to share. + * Return null if the node should be filtered. Otherwise return the Map of + * accepted attributes. + * + * @typedef {function(nodeName: String, attrs: Map): Map|null} FilterFunction + */ diff --git a/src/Bindings/DomBinding/selection.js b/src/Bindings/DomBinding/selection.js index 366a1db9..25798edb 100644 --- a/src/Bindings/DomBinding/selection.js +++ b/src/Bindings/DomBinding/selection.js @@ -1,84 +1,35 @@ /* globals getSelection */ -import { getRelativePosition, fromRelativePosition } from '../../Util/relativePosition.js' +import { getRelativePosition } from '../../Util/relativePosition.js' -let browserSelection = null let relativeSelection = null -/** - * @private - */ -export let beforeTransactionSelectionFixer -if (typeof getSelection !== 'undefined') { - beforeTransactionSelectionFixer = function _beforeTransactionSelectionFixer (y, domBinding, transaction, remote) { - if (!remote) { - return - } - relativeSelection = { from: null, to: null, fromY: null, toY: null } - browserSelection = getSelection() - const anchorNode = browserSelection.anchorNode - const anchorNodeType = domBinding.domToType.get(anchorNode) - if (anchorNode !== null && anchorNodeType !== undefined) { - relativeSelection.from = getRelativePosition(anchorNodeType, browserSelection.anchorOffset) - relativeSelection.fromY = anchorNodeType._y - } - const focusNode = browserSelection.focusNode - const focusNodeType = domBinding.domToType.get(focusNode) - if (focusNode !== null && focusNodeType !== undefined) { - relativeSelection.to = getRelativePosition(focusNodeType, browserSelection.focusOffset) - relativeSelection.toY = focusNodeType._y +function _getCurrentRelativeSelection (domBinding) { + const { baseNode, baseOffset, extentNode, extentOffset } = getSelection() + const baseNodeType = domBinding.domToType.get(baseNode) + const extentNodeType = domBinding.domToType.get(extentNode) + if (baseNodeType !== undefined && extentNodeType !== undefined) { + return { + from: getRelativePosition(baseNodeType, baseOffset), + to: getRelativePosition(extentNodeType, extentOffset) } } -} else { - beforeTransactionSelectionFixer = function _fakeBeforeTransactionSelectionFixer () {} + return null +} + +export const getCurrentRelativeSelection = typeof getSelection !== 'undefined' ? _getCurrentRelativeSelection : () => null + +export function beforeTransactionSelectionFixer (domBinding, remote) { + if (remote) { + relativeSelection = getCurrentRelativeSelection(domBinding) + } } /** * @private */ -export function afterTransactionSelectionFixer (y, domBinding, transaction, remote) { - if (relativeSelection === null || !remote) { - return - } - const to = relativeSelection.to - const from = relativeSelection.from - const fromY = relativeSelection.fromY - const toY = relativeSelection.toY - let shouldUpdate = false - let anchorNode = browserSelection.anchorNode - let anchorOffset = browserSelection.anchorOffset - let focusNode = browserSelection.focusNode - let focusOffset = browserSelection.focusOffset - if (from !== null) { - let sel = fromRelativePosition(fromY, from) - if (sel !== null) { - let node = domBinding.typeToDom.get(sel.type) - let offset = sel.offset - if (node !== anchorNode || offset !== anchorOffset) { - anchorNode = node - anchorOffset = offset - shouldUpdate = true - } - } - } - if (to !== null) { - let sel = fromRelativePosition(toY, to) - if (sel !== null) { - let node = domBinding.typeToDom.get(sel.type) - let offset = sel.offset - if (node !== focusNode || offset !== focusOffset) { - focusNode = node - focusOffset = offset - shouldUpdate = true - } - } - } - if (shouldUpdate) { - browserSelection.setBaseAndExtent( - anchorNode, - anchorOffset, - focusNode, - focusOffset - ) +export function afterTransactionSelectionFixer (domBinding, remote) { + if (relativeSelection !== null && remote) { + domBinding.restoreSelection(relativeSelection) } } diff --git a/src/Struct/Item.js b/src/Struct/Item.js index 9f17d1e7..3b03e33b 100644 --- a/src/Struct/Item.js +++ b/src/Struct/Item.js @@ -120,7 +120,7 @@ export default class Item { * * @private */ - _redo (y) { + _redo (y, redoitems) { if (this._redone !== null) { return this._redone } @@ -130,7 +130,10 @@ export default class Item { let parent = this._parent // make sure that parent is redone if (parent._deleted === true && parent._redone === null) { - parent._redo(y) + // try to undo parent if it will be undone anyway + if (!redoitems.has(parent) || !parent._redo(y, redoitems)) { + return false + } } if (parent._redone !== null) { parent = parent._redone @@ -157,7 +160,7 @@ export default class Item { struct._parentSub = this._parentSub struct._integrate(y) this._redone = struct - return struct + return true } /** diff --git a/src/Types/YText/YText.js b/src/Types/YText/YText.js index 42401f55..5b9c4c4a 100644 --- a/src/Types/YText/YText.js +++ b/src/Types/YText/YText.js @@ -254,7 +254,7 @@ function deleteText (y, length, parent, left, right, currentAttributes) { * @typedef {Array} Delta */ - /** +/** * Attributes that can be assigned to a selection of text. * * @example diff --git a/src/Util/UndoManager.js b/src/Util/UndoManager.js index 0810ffc8..9b8dce50 100644 --- a/src/Util/UndoManager.js +++ b/src/Util/UndoManager.js @@ -2,7 +2,7 @@ import ID from './ID/ID.js' import isParentOf from './isParentOf.js' class ReverseOperation { - constructor (y, transaction) { + constructor (y, transaction, bindingInfos) { this.created = new Date() const beforeState = transaction.beforeState if (beforeState.has(y.userID)) { @@ -12,15 +12,26 @@ class ReverseOperation { this.toState = null this.fromState = null } - this.deletedStructs = transaction.deletedStructs + this.deletedStructs = new Set() + transaction.deletedStructs.forEach(struct => { + this.deletedStructs.add({ + from: struct._id, + len: struct._length + }) + }) + /** + * Maps from binding to binding information (e.g. cursor information) + */ + this.bindingInfos = bindingInfos } } function applyReverseOperation (y, scope, reverseBuffer) { let performedUndo = false + let undoOp y.transact(() => { while (!performedUndo && reverseBuffer.length > 0) { - let undoOp = reverseBuffer.pop() + undoOp = reverseBuffer.pop() // make sure that it is possible to iterate {from}-{to} if (undoOp.fromState !== null) { y.os.getItemCleanStart(undoOp.fromState) @@ -35,23 +46,39 @@ function applyReverseOperation (y, scope, reverseBuffer) { } }) } - for (let op of undoOp.deletedStructs) { - if ( - isParentOf(scope, op) && - op._parent !== y && - ( - op._id.user !== y.userID || - undoOp.fromState === null || - op._id.clock < undoOp.fromState.clock || - op._id.clock > undoOp.toState.clock - ) - ) { - performedUndo = true - op._redo(y) - } + const redoitems = new Set() + for (let del of undoOp.deletedStructs) { + const fromState = del.from + const toState = new ID(fromState.user, fromState.clock + del.len - 1) + y.os.getItemCleanStart(fromState) + y.os.getItemCleanEnd(toState) + y.os.iterate(fromState, toState, op => { + if ( + isParentOf(scope, op) && + op._parent !== y && + ( + op._id.user !== y.userID || + undoOp.fromState === null || + op._id.clock < undoOp.fromState.clock || + op._id.clock > undoOp.toState.clock + ) + ) { + redoitems.add(op) + } + }) } + redoitems.forEach(op => { + const opUndone = op._redo(y, redoitems) + performedUndo = performedUndo || opUndone + }) } }) + if (performedUndo) { + // should be performed after the undo transaction + undoOp.bindingInfos.forEach((info, binding) => { + binding._restoreUndoStackInfo(info) + }) + } return performedUndo } @@ -66,6 +93,7 @@ export default class UndoManager { */ constructor (scope, options = {}) { this.options = options + this._bindings = new Set(options.bindings) options.captureTimeout = options.captureTimeout == null ? 500 : options.captureTimeout this._undoBuffer = [] this._redoBuffer = [] @@ -76,16 +104,28 @@ export default class UndoManager { const y = scope._y this.y = y y._hasUndoManager = true + let bindingInfos + y.on('beforeTransaction', (y, transaction, remote) => { + if (!remote) { + // Store binding information before transaction is executed + // By restoring the binding information, we can make sure that the state + // before the transaction can be recovered + bindingInfos = new Map() + this._bindings.forEach(binding => { + bindingInfos.set(binding, binding._getUndoStackInfo()) + }) + } + }) y.on('afterTransaction', (y, transaction, remote) => { if (!remote && transaction.changedParentTypes.has(scope)) { - let reverseOperation = new ReverseOperation(y, transaction) + let reverseOperation = new ReverseOperation(y, transaction, bindingInfos) if (!this._undoing) { let lastUndoOp = this._undoBuffer.length > 0 ? this._undoBuffer[this._undoBuffer.length - 1] : null if ( this._redoing === false && this._lastTransactionWasUndo === false && lastUndoOp !== null && - reverseOperation.created - lastUndoOp.created <= options.captureTimeout + (options.captureTimeout < 0 || reverseOperation.created - lastUndoOp.created <= options.captureTimeout) ) { lastUndoOp.created = reverseOperation.created if (reverseOperation.toState !== null) { @@ -110,6 +150,13 @@ export default class UndoManager { }) } + /** + * Enforce that the next change is created as a separate item in the undo stack + */ + flushChanges () { + this._lastTransactionWasUndo = true + } + /** * Undo the last locally created change. */ diff --git a/src/Util/relativePosition.js b/src/Util/relativePosition.js index df85b00c..421a5d2f 100644 --- a/src/Util/relativePosition.js +++ b/src/Util/relativePosition.js @@ -76,7 +76,10 @@ export function fromRelativePosition (y, rpos) { } else { id = new RootID(rpos[3], rpos[4]) } - const type = y.os.get(id) + let type = y.os.get(id) + while (type._redone !== null) { + type = type._redone + } if (type === null || type.constructor === GC) { return null } @@ -87,12 +90,16 @@ export function fromRelativePosition (y, rpos) { } else { let offset = 0 let struct = y.os.findNodeWithUpperBound(new ID(rpos[0], rpos[1])).val + const diff = rpos[1] - struct._id.clock + while (struct._redone !== null) { + struct = struct._redone + } const parent = struct._parent if (struct.constructor === GC || parent._deleted) { return null } if (!struct._deleted) { - offset = rpos[1] - struct._id.clock + offset = diff } struct = struct._left while (struct !== null) { diff --git a/test/y-array.tests.js b/test/y-array.tests.js index 26647338..8cb1b266 100644 --- a/test/y-array.tests.js +++ b/test/y-array.tests.js @@ -111,7 +111,7 @@ function compareEvent (t, is, should) { t.assert( should[key] === is[key] || JSON.stringify(should[key]) === JSON.stringify(is[key]) - , 'event works as expected' + , 'event works as expected' ) } }