Divec has uploaded a new change for review. https://gerrit.wikimedia.org/r/290425
Change subject: WIP: Calculate model ranges on demand, not RangeState creation ...................................................................... WIP: Calculate model ranges on demand, not RangeState creation Change-Id: I244dfad38348b279a51b7233014566d4dd33f4b3 --- M src/ce/keydownhandlers/ve.ce.LinearArrowKeyDownHandler.js M src/ce/ve.ce.RangeState.js M src/ce/ve.ce.Surface.js M src/ce/ve.ce.SurfaceObserver.js M src/ce/ve.ce.js M src/ve.SelectionState.js 6 files changed, 76 insertions(+), 200 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor refs/changes/25/290425/1 diff --git a/src/ce/keydownhandlers/ve.ce.LinearArrowKeyDownHandler.js b/src/ce/keydownhandlers/ve.ce.LinearArrowKeyDownHandler.js index f57abf6..24faf2b 100644 --- a/src/ce/keydownhandlers/ve.ce.LinearArrowKeyDownHandler.js +++ b/src/ce/keydownhandlers/ve.ce.LinearArrowKeyDownHandler.js @@ -186,7 +186,10 @@ } else { // Check where the range has moved to surface.surfaceObserver.pollOnceNoCallback(); - newRange = new ve.Range( surface.surfaceObserver.getRange().to ); + newRange = new ve.Range( ve.ce.getOffset( + surface.nativeSelection.focusNode, + surface.nativeSelection.focusOffset + ) ); } // Adjust range to use old anchor, if necessary diff --git a/src/ce/ve.ce.RangeState.js b/src/ce/ve.ce.RangeState.js index 27a7ef6..0569120 100644 --- a/src/ce/ve.ce.RangeState.js +++ b/src/ce/ve.ce.RangeState.js @@ -34,11 +34,6 @@ this.contentChanged = false; /** - * @property {ve.Range|null} veRange The current selection range - */ - this.veRange = null; - - /** * @property {ve.ce.BranchNode|null} node The current branch node */ this.node = null; @@ -100,10 +95,8 @@ if ( selection.equalsSelection( oldSelection ) ) { // No change; use old values for speed this.selectionChanged = false; - this.veRange = old && old.veRange; } else { this.selectionChanged = true; - this.veRange = ve.ce.veRangeFromSelection( selection ); } anchorNodeChanged = oldSelection.anchorNode !== selection.anchorNode; @@ -119,7 +112,6 @@ // Check this node belongs to our document if ( this.node && this.node.root !== documentNode ) { this.node = null; - this.veRange = null; } } } @@ -167,3 +159,12 @@ // misleadingly different from when the selection was saved). this.misleadingSelection = selection; }; + +/** + * Whether the selection is null + * + * @return {boolean} Whether the selection is null + */ +ve.ce.RangeState.prototype.isNullSelection = function () { + return this.misleadingSelection.isNull(); +}; diff --git a/src/ce/ve.ce.Surface.js b/src/ce/ve.ce.Surface.js index e1dcc5e..1f74770 100644 --- a/src/ce/ve.ce.Surface.js +++ b/src/ce/ve.ce.Surface.js @@ -2618,7 +2618,7 @@ */ ve.ce.Surface.prototype.handleObservedChanges = function ( oldState, newState ) { - var newSelection, transaction, removedUnicorns, + var newSelection, transaction, oldRange, removedUnicorns, activeNode, coveringRange, nodeRange, containsStart, containsEnd, surface = this, dmDoc = this.getModel().getDocument(), @@ -2632,6 +2632,7 @@ newState.node.unicornAnnotations ); if ( transaction ) { + oldRange = ve.ce.veRangeFromSelection( oldState.misleadingSelection ); this.incRenderLock(); try { this.changeModel( transaction ); @@ -2651,21 +2652,22 @@ oldState.node.root && oldState.node instanceof ve.ce.ContentBranchNode ) { + oldRange = ve.ce.veRangeFromSelection( oldState.misleadingSelection ); oldState.node.renderContents(); } - - if ( newState.selectionChanged && !( - // Ignore when the newRange is just a flipped oldRange - oldState && - oldState.veRange && - newState.veRange && - !newState.veRange.isCollapsed() && - oldState.veRange.equalsSelection( newState.veRange ) - ) ) { - if ( newState.veRange ) { - newSelection = new ve.dm.LinearSelection( dmDoc, newState.veRange ); - } else { + if ( + newState.selectionChanged && + // Ignore when the new range is just a flipped old range + newState.misleadingSelection && + newState.misleadingSelection.equalsRange( oldState.misleadingSelection ) + ) { + if ( newState.isNullSelection() ) { newSelection = new ve.dm.NullSelection( dmDoc ); + } else { + newSelection = new ve.dm.LinearSelection( + dmDoc, + ve.ce.veRangeFromSelection( newState.misleadingSelection ) + ); } this.incRenderLock(); try { @@ -2685,16 +2687,22 @@ nodeRange = activeNode.getRange(); containsStart = nodeRange.containsRange( new ve.Range( coveringRange.start ) ); containsEnd = nodeRange.containsRange( new ve.Range( coveringRange.end ) ); - // If the range starts xor ends in the active node, but not both, then it must - // span an active node boundary, so fixup. + // If the range starts or ends in the active node, but not both, then it must + // span an active node boundary, so revert the selection. /*jshint bitwise: false*/ if ( containsStart ^ containsEnd ) { - newSelection = oldState && oldState.veRange ? - new ve.dm.LinearSelection( dmDoc, oldState.veRange ) : - new ve.dm.NullSelection( dmDoc ); + if ( oldState && !oldState.isNullSelection() ) { + if ( !oldRange ) { + oldRange = ve.ce.veRangeFromSelection( oldState.misleadingSelection ); + } + // TODO translate oldRange if newState.contentChanged? + newSelection = new ve.dm.LinearSelection( dmDoc, oldRange ); + } else { + newSelection = new ve.dm.NullSelection( dmDoc ); + } setTimeout( function () { surface.changeModel( null, newSelection ); - surface .showModelSelection(); + surface.showModelSelection(); } ); } /*jshint bitwise: true*/ diff --git a/src/ce/ve.ce.SurfaceObserver.js b/src/ce/ve.ce.SurfaceObserver.js index 4c9d9f1..c05dccc 100644 --- a/src/ce/ve.ce.SurfaceObserver.js +++ b/src/ce/ve.ce.SurfaceObserver.js @@ -184,17 +184,3 @@ ve.ce.SurfaceObserver.prototype.setTimeout = function ( callback, timeout ) { return setTimeout( callback, timeout ); }; - -/** - * Get the range last observed. - * - * Used when you have just polled, but don't want to wait for a 'rangeChange' event. - * - * @return {ve.Range|null} Range - */ -ve.ce.SurfaceObserver.prototype.getRange = function () { - if ( !this.rangeState ) { - return null; - } - return this.rangeState.veRange; -}; diff --git a/src/ce/ve.ce.js b/src/ce/ve.ce.js index 44ed5ea..27884ad 100644 --- a/src/ce/ve.ce.js +++ b/src/ce/ve.ce.js @@ -163,6 +163,8 @@ /** * Gets the linear offset from a given DOM node and offset within it. * + * N.B. The model must be in sync with the view for this to work + * * @method * @param {HTMLElement} domNode DOM node * @param {number} domOffset DOM offset within the DOM node @@ -421,7 +423,9 @@ }; /** - * Find the DM range of a DOM selection + * Find the DM range of a DOM selection. + * + * N.B. The model must be in sync with the view for this to work * * @param {Object} selection DOM-selection-like object * @param {Node} selection.anchorNode @@ -452,161 +456,6 @@ node = node.parentNode; } return $( node ).closest( '.ve-ce-linkAnnotation' )[ 0 ]; -}; - -/** - * Analyse a DOM content change to build a Transaction - * - * Content changes have oldState.node === newState.node and newState.contentChanged === true . - * Annotations are inferred heuristically from plaintext to do what the user intended. - * TODO: treat more changes as simple (not needing a re-render); see - * https://phabricator.wikimedia.org/T114260 . - * - * @method - * @param {ve.ce.RangeState} oldState The prior range state - * @param {ve.ce.RangeState} newState The changed range state - * - * @return {Object} Results of analysis - * @return {ve.dm.Transaction} return.transaction Transaction corresponding to the DOM content change - * @return {ve.dm.Selection} return.selection Changed selection to apply (TODO: unsafe / useless?) - * @return {boolean} return.rerender Whether the DOM needs rerendering after applying the transaction - */ -ve.ce.modelChangeFromContentChange = function ( oldState, newState ) { - var data, len, annotations, bothCollapsed, oldStart, newStart, replacementRange, - fromLeft = 0, - fromRight = 0, - // It is guaranteed that oldState.node === newState.node , so just call it 'node' - node = newState.node, - nodeOffset = node.getModel().getOffset(), - oldText = oldState.text, - newText = newState.text, - oldRange = oldState.veRange, - newRange = newState.veRange, - oldData = oldText.split( '' ), - newData = newText.split( '' ), - lengthDiff = newText.length - oldText.length, - dmDoc = node.getModel().getDocument(), - modelData = dmDoc.data; - - bothCollapsed = oldRange.isCollapsed() && newRange.isCollapsed(); - oldStart = oldRange.start - nodeOffset - 1; - newStart = newRange.start - nodeOffset - 1; - - // If the only change is an insertion just before the new cursor, then apply a - // single insertion transaction, using the annotations from the old start - // position (accounting for whether the cursor was before or after an annotation - // boundary) - if ( - bothCollapsed && - lengthDiff > 0 && - oldText.slice( 0, oldStart ) === newText.slice( 0, oldStart ) && - oldText.slice( oldStart ) === newText.slice( newStart ) - ) { - data = newData.slice( oldStart, newStart ); - if ( node.unicornAnnotations ) { - annotations = node.unicornAnnotations; - } else { - annotations = modelData.getInsertionAnnotationsFromRange( - oldRange, - oldState.focusIsAfterAnnotationBoundary - ); - } - - if ( annotations.getLength() ) { - ve.dm.Document.static.addAnnotationsToData( data, annotations ); - } - - return { - transaction: ve.dm.Transaction.newFromInsertion( - dmDoc, - oldRange.start, - data - ), - selection: new ve.dm.LinearSelection( dmDoc, newRange ), - rerender: false - }; - } - - // If the only change is a removal touching the old cursor position, then apply - // a single removal transaction. - if ( - bothCollapsed && - lengthDiff < 0 && - oldText.slice( 0, newStart ) === newText.slice( 0, newStart ) && - oldText.slice( newStart - lengthDiff ) === newText.slice( newStart ) - ) { - return { - transaction: ve.dm.Transaction.newFromRemoval( - dmDoc, - new ve.Range( newRange.start, newRange.start - lengthDiff ) - ), - selection: new ve.dm.LinearSelection( dmDoc, newRange ), - rerender: false - }; - } - - // Complex change (either removal+insertion or insertion not just before new cursor) - // 1. Count unchanged characters from left and right; - // 2. Assume that the minimal changed region indicates the replacement made by the user; - // 3. Hence guess how to map annotations. - // N.B. this logic can go wrong; e.g. this code will see slice->slide and - // assume that the user changed 'c' to 'd', but the user could instead have changed 'ic' - // to 'id', which would map annotations differently. - - len = Math.min( oldData.length, newData.length ); - - while ( fromLeft < len && oldData[ fromLeft ] === newData[ fromLeft ] ) { - ++fromLeft; - } - - while ( - fromRight < len - fromLeft && - oldData[ oldData.length - 1 - fromRight ] === - newData[ newData.length - 1 - fromRight ] - ) { - ++fromRight; - } - replacementRange = new ve.Range( - nodeOffset + 1 + fromLeft, - nodeOffset + 1 + oldData.length - fromRight - ); - data = newData.slice( fromLeft, newData.length - fromRight ); - - if ( node.unicornAnnotations ) { - // This CBN is unicorned. Use the stored annotations. - annotations = node.unicornAnnotations; - } else { - // Guess the annotations from the (possibly empty) range being replaced. - // - // Still consider focusIsAfterAnnotationBoundary, even though the change is - // not necessarily at the cursor: assume the old focus was inside the same - // DOM text node as the insertion, and therefore has the same annotations. - // Otherwise, when using an IME that selects inserted text, this code path - // can cause an annotation discrepancy that triggers an unwanted re-render, - // closing the IME (For example, when typing at the start of <p><i>x</i></p> - // in Windows 8.1 Korean on IE11). - annotations = modelData.getInsertionAnnotationsFromRange( - replacementRange, - oldRange.isCollapsed() && oldState.focusIsAfterAnnotationBoundary - ); - } - if ( annotations.getLength() ) { - ve.dm.Document.static.addAnnotationsToData( data, annotations ); - } - if ( newRange.isCollapsed() ) { - // TODO: Remove this, or comment why it's necessary - // (When wouldn't we be at a cursor offset?) - newRange = new ve.Range( dmDoc.getNearestCursorOffset( newRange.start, 1 ) ); - } - return { - transaction: ve.dm.Transaction.newFromReplacement( - dmDoc, - replacementRange, - data - ), - selection: new ve.dm.LinearSelection( dmDoc, newRange ), - rerender: true - }; }; /** diff --git a/src/ve.SelectionState.js b/src/ve.SelectionState.js index 9db1434..4e3a61e 100644 --- a/src/ve.SelectionState.js +++ b/src/ve.SelectionState.js @@ -68,6 +68,15 @@ /* Methods */ /** + * Whether the selection is null + * + * @return {boolean} true if the focusNode is null (in which case the anchorNode should be null too) + */ +ve.SelectionState.prototype.isNull = function () { + return this.focusNode === null; +}; + +/** * Returns the selection with the anchor and focus swapped * * @return {ve.SelectionState} selection with anchor/focus swapped. Object-identical to this if isCollapsed @@ -87,10 +96,10 @@ }; /** - * Whether the selection represents is the same range as another DOM Selection-like object + * Whether this and another DOM Selection-like object have the same anchor and the same focus * * @param {Object} other DOM Selection-like object - * @return {boolean} True if the anchors/focuses are equal (including null) + * @return {boolean} True if the anchors and focuses are equal (including null) */ ve.SelectionState.prototype.equalsSelection = function ( other ) { return this.anchorNode === other.anchorNode && @@ -100,6 +109,26 @@ }; /** + * Whether this and another DOM Selection-like object have the same anchor/focus (possibly flipped) + * + * @param {Object} other DOM Selection-like object + * @return {boolean} True if the anchors and focuses are equal (including null) or equal flipped + */ +ve.SelectionState.prototype.equalsRange = function ( other ) { + return ( + this.anchorNode === other.anchorNode && + this.anchorOffset === other.anchorOffset && + this.focusNode === other.focusNode && + this.focusOffset === other.focusOffset + ) || ( + this.anchorNode === other.focusNode && + this.anchorOffset === other.focusOffset && + this.focusNode === other.anchorNode && + this.focusOffset === other.anchorOffset + ); +}; + +/** * Get a range representation of the selection * * N.B. Range objects do not show whether the selection is backwards -- To view, visit https://gerrit.wikimedia.org/r/290425 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I244dfad38348b279a51b7233014566d4dd33f4b3 Gerrit-PatchSet: 1 Gerrit-Project: VisualEditor/VisualEditor Gerrit-Branch: master Gerrit-Owner: Divec <da...@troi.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits