Divec has uploaded a new change for review.
https://gerrit.wikimedia.org/r/82858
Change subject: WIP: Refactor CE Surface to reduce event feedback
......................................................................
WIP: Refactor CE Surface to reduce event feedback
Restructure SurfaceObserver methods so that the method calls are more precise.
Sequence all key*/mouse*/composition* events using EventSequencer.
modules/ve/ce/ve.ce.SurfaceObserver.js
* Move timing code into startTimerLoop / stopTimerLoop / timerLoop
* Split sync method calls into syncBothWays / syncToModel
* Move postponed sync logic into syncBothWaysAfter / syncToModelAfter
modules/ve/ce/ve.ce.Surface.js
* Use EventSequencer for keyboard/mouse events
* Change all surfaceObserver calls to use new API
modules/ve/ui/ve.ui.Surface.js
* Change all surfaceObserver calls to use new API
modules/ve/ve.EventSequencer.js
* Implement clearListeners
Change-Id: I0085e4a53c5a776733dce6944b867b8d2228ba4b
---
M modules/ve/ce/ve.ce.Surface.js
M modules/ve/ce/ve.ce.SurfaceObserver.js
M modules/ve/ui/ve.ui.Surface.js
M modules/ve/ve.EventSequencer.js
4 files changed, 140 insertions(+), 139 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/58/82858/1
diff --git a/modules/ve/ce/ve.ce.Surface.js b/modules/ve/ce/ve.ce.Surface.js
index 327ff09..fa85aa6 100644
--- a/modules/ve/ce/ve.ce.Surface.js
+++ b/modules/ve/ce/ve.ce.Surface.js
@@ -35,6 +35,12 @@
this.selectionTimeout = null;
this.keyPressTimeout = null;
this.$document = $( this.getElementDocument() );
+ this.eventSequencer = new ve.EventSequencer(
+ this.$document,
+ ['keydown', 'keypress', 'keyup', 'mousedown', 'mouseup',
'mousemove',
+ 'compositionstart', 'compositionend'],
+ ve.bind( ve.log, ve )
+ );
this.clipboard = {};
this.renderLocks = 0;
this.dragging = false;
@@ -285,18 +291,27 @@
* @param {jQuery.Event} e Focus event
*/
ve.ce.Surface.prototype.documentOnFocus = function () {
- this.$document.off( '.ve-ce-Surface' );
- this.$document.on( {
- 'keydown.ve-ce-Surface': ve.bind( this.onDocumentKeyDown, this
),
- 'keyup.ve-ce-Surface': ve.bind( this.onDocumentKeyUp, this ),
- 'keypress.ve-ce-Surface': ve.bind( this.onDocumentKeyPress,
this ),
- 'mousedown.ve-ce-Surface': ve.bind( this.onDocumentMouseDown,
this ),
- 'mouseup.ve-ce-Surface': ve.bind( this.onDocumentMouseUp, this
),
- 'mousemove.ve-ce-Surface': ve.bind( this.onDocumentMouseMove,
this ),
- 'compositionstart.ve-ce-Surface': ve.bind(
this.onDocumentCompositionStart, this ),
- 'compositionend.ve-ce-Surface': ve.bind(
this.onDocumentCompositionEnd, this )
- } );
- this.surfaceObserver.start( true, true );
+ var eventName, preListeners = {
+ 'keydown': ve.bind( this.onDocumentKeyDown, this ),
+ 'keyup': ve.bind( this.onDocumentKeyUp, this ),
+ 'keypress': ve.bind( this.onDocumentKeyPress, this ),
+ 'mousedown': ve.bind( this.onDocumentMouseDown, this ),
+ 'mouseup': ve.bind( this.onDocumentMouseUp, this ),
+ 'mousemove': ve.bind( this.onDocumentMouseMove, this ),
+ 'compositionstart': ve.bind( this.onDocumentCompositionStart,
this ),
+ 'compositionend': ve.bind( this.onDocumentCompositionEnd, this )
+ }, postListeners = {
+ 'keypress': ve.bind( this.onDocumentPostKeyPress, this )
+ };
+
+ this.eventSequencer.clearListeners();
+ for ( eventName in preListeners ) {
+ this.eventSequencer.addPreListener( eventName,
preListeners[eventName] );
+ }
+ for ( eventName in postListeners ) {
+ this.eventSequencer.addPostListener( eventName,
postListeners[eventName] );
+ }
+ this.surfaceObserver.startTimerLoop();
};
/**
@@ -306,8 +321,9 @@
* @param {jQuery.Event} e Element blur event
*/
ve.ce.Surface.prototype.documentOnBlur = function () {
- this.$document.off( '.ve-ce-Surface' );
- this.surfaceObserver.stop( true, true );
+ this.eventSequencer.clearListeners();
+ this.surfaceObserver.stopTimerLoop();
+ this.surfaceObserver.pollOnce();
this.dragging = false;
};
@@ -327,7 +343,8 @@
// this.$$( e.target ).closest( '.ve-ce-documentNode' ).length === 0
if ( e.which === 1 ) {
- this.surfaceObserver.stop( true, true );
+ this.surfaceObserver.stopTimerLoop();
+ this.surfaceObserver.pollOnce(); // TODO: guard with
incRenderLock?
}
// Handle triple click
@@ -353,7 +370,8 @@
* @emits selectionEnd
*/
ve.ce.Surface.prototype.onDocumentMouseUp = function ( e ) {
- this.surfaceObserver.start( false, true );
+ this.surfaceObserver.startTimerLoop();
+ this.surfaceObserver.pollOnce(); // TODO: guard with incRenderLock?
if ( !e.shiftKey && this.selecting ) {
this.emit( 'selectionEnd' );
this.selecting = false;
@@ -454,7 +472,6 @@
*/
ve.ce.Surface.prototype.onDocumentKeyDown = function ( e ) {
var trigger;
- this.forceKeyPressTimeout();
// Ignore keydowns while in IME mode but do not preventDefault them (so
text actually appear on
// the screen).
@@ -472,7 +489,13 @@
return;
}
- this.surfaceObserver.stop( true, false );
+ this.surfaceObserver.stopTimerLoop();
+ this.incRenderLock();
+ try {
+ this.surfaceObserver.pollOnce(); // TODO: is this correct?
+ } finally {
+ this.decRenderLock();
+ }
switch ( e.keyCode ) {
case ve.Keys.LEFT:
case ve.Keys.RIGHT:
@@ -507,7 +530,13 @@
}
break;
}
- this.surfaceObserver.start( false, false );
+ this.incRenderLock();
+ try {
+ this.surfaceObserver.pollOnce();
+ } finally {
+ this.decRenderLock();
+ }
+ this.surfaceObserver.startTimerLoop();
};
/**
@@ -518,8 +547,6 @@
*/
ve.ce.Surface.prototype.onDocumentKeyPress = function ( e ) {
var selection, prevNode, documentModel = this.model.getDocument();
-
- this.forceKeyPressTimeout();
// Prevent IE from editing Aliens/Entities
// TODO: Better comment about what's going on here is needed.
@@ -546,39 +573,10 @@
}
this.handleInsertion();
- this.setKeyPressTimeout();
};
-/**
- * Append a call to onKeyPressTimeout to the event queue.
- * @method
- */
-ve.ce.Surface.prototype.setKeyPressTimeout = function () {
- this.keyPressTimeout = setTimeout( ve.bind( function() {
- this.keyPressTimeout = null;
- this.onKeyPressTimeout();
- }, this ) );
-};
-
-/**
- * If there is a pending call to onKeyPressTimeout in the event queue, delete
it and call now
- * @method
- */
-ve.ce.Surface.prototype.forceKeyPressTimeout = function () {
- if ( this.keyPressTimeout === null ) {
- return;
- }
- clearTimeout( this.keyPressTimeout );
- this.keyPressTimeout = null;
- this.onKeyPressTimeout();
-};
-
-/**
- * post-keypress handler: re-sync the surface and model
- * @method
- */
-ve.ce.Surface.prototype.onKeyPressTimeout = function () {
- this.surfaceObserver.start( false, true );
+ve.ce.Surface.prototype.onDocumentPostKeyPress = function () {
+ this.surfaceObserver.pollOnce();
};
/**
@@ -603,7 +601,8 @@
* @param {jQuery.Event} e Cut event
*/
ve.ce.Surface.prototype.onCut = function ( e ) {
- this.surfaceObserver.stop( false, true );
+ // TODO: is it correct that we don't need to pollOnce here?
+ this.surfaceObserver.stopTimerLoop();
this.onCopy( e );
setTimeout( ve.bind( function () {
var selection, tx;
@@ -620,7 +619,8 @@
this.model.change( tx, new ve.Range( selection.start ) );
this.surfaceObserver.clear();
- this.surfaceObserver.start( false, true );
+ this.surfaceObserver.startTimerLoop();
+ this.surfaceObserver.pollOnce();
}, this ) );
};
@@ -680,7 +680,8 @@
eventPasteKey = clipboardData && clipboardData.getData(
'text/xcustom' ),
eventPasteText = clipboardData && clipboardData.getData(
'text/plain' );
- this.surfaceObserver.stop( false, true );
+ // TODO: is it correct that we don't need to pollOnce here?
+ this.surfaceObserver.stopTimerLoop();
// Pasting into a range? Remove first.
if ( !rangy.getSelection( this.$document[0] ).isCollapsed ) {
@@ -784,7 +785,13 @@
*/
ve.ce.Surface.prototype.onDocumentCompositionEnd = function () {
this.inIme = false;
- this.surfaceObserver.start( false, false );
+ this.incRenderLock();
+ try {
+ this.surfaceObserver.pollOnce();
+ } finally {
+ this.decRenderLock();
+ }
+ this.surfaceObserver.startTimerLoop();
};
/*! Custom Events */
@@ -836,7 +843,7 @@
/**
* Handle selection change events.
*
- * @see ve.ce.SurfaceObserver#poll
+ * @see ve.ce.SurfaceObserver#pollOnce
*
* @method
* @param {ve.Range} oldRange
@@ -847,18 +854,13 @@
// Ignore when the newRange is just a flipped oldRange
return;
}
- this.incRenderLock();
- try {
- this.model.change( null, newRange );
- } finally {
- this.decRenderLock();
- }
+ this.model.change( null, newRange );
};
/**
* Handle content change events.
*
- * @see ve.ce.SurfaceObserver#poll
+ * @see ve.ce.SurfaceObserver#pollOnce
*
* @method
* @param {HTMLElement} node DOM node the change occured in
@@ -1031,7 +1033,7 @@
* @method
*/
ve.ce.Surface.prototype.onLock = function () {
- this.surfaceObserver.stop( false, true );
+ this.surfaceObserver.locked = true;
};
/**
@@ -1040,8 +1042,8 @@
* @method
*/
ve.ce.Surface.prototype.onUnlock = function () {
- this.surfaceObserver.clear( this.model.getSelection() );
- this.surfaceObserver.start( false, true );
+ this.surfaceObserver.locked = false;
+ // TODO: Do we need to pollOnce?
};
/*! Relocation */
@@ -1088,8 +1090,13 @@
// Selection is going to be displayed programmatically so prevent
default browser behaviour
e.preventDefault();
- // Stop with final poll cycle so we have correct information in model
- this.surfaceObserver.stop( true, false );
+ this.surfaceObserver.stopTimerLoop(); // TODO: onDocumentKeyDown did
this already
+ this.incRenderLock();
+ try {
+ this.surfaceObserver.pollOnce(); // TODO: onDocumentKeyDown did
this already
+ } finally {
+ this.decRenderLock();
+ }
selection = this.model.getSelection();
if ( this.$$( e.target ).css( 'direction' ) === 'rtl' ) {
// If the language direction is RTL, switch left/right
directions:
@@ -1105,7 +1112,8 @@
e.shiftKey
);
this.model.change( null, range );
- this.surfaceObserver.start( false, true );
+ this.surfaceObserver.startTimerLoop(); // TODO: onDocumentKeyDown does
this anyway
+ this.surfaceObserver.pollOnce();
};
/**
@@ -1122,7 +1130,8 @@
nativeSel.modify( 'extend', 'left', 'character' );
return;
}
- this.surfaceObserver.stop( true, true );
+ this.surfaceObserver.stopTimerLoop(); // TODO: onDocumentKeyDown did
this already
+ this.surfaceObserver.pollOnce(); // TODO: onDocumentKeyDown did this
already
selection = this.model.getSelection();
rangySelection = rangy.getSelection( this.$document[0] );
// Perform programatic handling only for selection that is expanded and
backwards according to
@@ -1146,18 +1155,18 @@
if ( !$element.hasClass( 've-ce-branchNode-slug' ) ) {
$element.remove();
}
- this.surfaceObserver.start( false, true );
- this.surfaceObserver.stop( false, true );
+ this.surfaceObserver.pollOnce();
if ( e.shiftKey === true ) { // expanded range
range = new ve.Range( selection.from,
this.model.getSelection().to );
} else { // collapsed range (just a cursor)
range = new ve.Range(
this.model.getSelection().to );
}
this.model.change( null, range );
- this.surfaceObserver.start( false, true );
+ this.surfaceObserver.pollOnce();
}, this ), 0 );
} else {
- this.surfaceObserver.start( false, true );
+ this.surfaceObserver.startTimerLoop(); // TODO:
onDocumentKeyDown does this anyway
+ this.surfaceObserver.pollOnce();
}
};
@@ -1213,7 +1222,8 @@
}
}
- this.surfaceObserver.stop( true, true );
+ this.surfaceObserver.stopTimerLoop();
+ this.surfaceObserver.pollOnce();
};
/**
diff --git a/modules/ve/ce/ve.ce.SurfaceObserver.js
b/modules/ve/ce/ve.ce.SurfaceObserver.js
index e49f406..edca74b 100644
--- a/modules/ve/ce/ve.ce.SurfaceObserver.js
+++ b/modules/ve/ce/ve.ce.SurfaceObserver.js
@@ -24,8 +24,9 @@
this.documentView = documentView;
this.domDocument = null;
this.polling = false;
+ this.locked = false;
this.timeoutId = null;
- this.frequency = 250; //ms
+ this.frequency = 250; // ms
// Initialization
this.clear();
@@ -51,6 +52,7 @@
* @param {Object} next.text New plain text content
* @param {Object} next.hash New DOM hash
* @param {ve.Range} next.range New selection
+ * @param {boolean} fromSurface Whether the change originates in the Surface
*/
/**
@@ -61,6 +63,7 @@
* @event selectionChange
* @param {ve.Range} oldRange
* @param {ve.Range} newRange
+ * @param {boolean} fromSurface Whether the change originates in the Surface
*/
/* Methods */
@@ -80,35 +83,41 @@
};
/**
- * Start polling.
- *
- * If {postpone} is false or undefined the first poll cycle will occur
immediately and synchronously.
+ * Start the setTimeout synchronisation loop
*
* @method
- * @param {boolean} postpone Add the first poll to the end of the event queue
- * @param {boolean} emitContentChanges Allow contentChange to be emitted
*/
-ve.ce.SurfaceObserver.prototype.start = function ( postpone,
emitContentChanges ) {
+ve.ce.SurfaceObserver.prototype.startTimerLoop = function () {
this.domDocument =
this.documentView.getDocumentNode().getElementDocument();
this.polling = true;
- this.poll( postpone, emitContentChanges );
+ this.timerLoop( true ); // will not sync immediately, because timeoutId
should be null
};
/**
- * Stop polling.
- *
- * If {poll} is false or undefined than no final poll cycle will occur and
changes can be lost. If
- * it's true then a final poll cycle will occur immediately and synchronously.
+ * Loop once with `setTimeout`
+ * @method
+ * @param {boolean} firstTime Wait before polling
+ */
+ve.ce.SurfaceObserver.prototype.timerLoop = function ( firstTime ) {
+ if ( this.timeoutId ) {
+ // in case we're not running from setTimeout
+ clearTimeout( this.timeoutId );
+ this.timeoutId = null;
+ }
+ if ( ! firstTime && ! this.locked ) {
+ this.pollOnce();
+ }
+ // only reach this point if syncBothWays does not throw an exception
+ this.timeoutId = setTimeout( ve.bind( this.timerLoop, this ),
this.frequency );
+};
+
+/**
+ * Stop polling
*
* @method
- * @param {boolean} poll Poll one last time before stopping future polling
- * @param {boolean} emitContentChanges Allow contentChange to be emitted
*/
-ve.ce.SurfaceObserver.prototype.stop = function ( poll, emitContentChanges ) {
+ve.ce.SurfaceObserver.prototype.stopTimerLoop = function () {
if ( this.polling === true ) {
- if ( poll === true ) {
- this.poll( false, emitContentChanges );
- }
this.polling = false;
clearTimeout( this.timeoutId );
this.timeoutId = null;
@@ -122,36 +131,15 @@
*
* TODO: fixing selection in certain cases, handling selection across multiple
nodes in Firefox
*
- * FIXME: Does not work well (selectionChange is not emited) when cursor is
placed inside a slug
+ * FIXME: Does not work well (selectionChange is not emitted) when cursor is
placed inside a slug
* with a mouse.
*
* @method
- * @param {boolean} postpone Append the poll action to the end of the event
queue
- * @param {boolean} emitContentChanges Allow contentChange to be emitted
* @emits contentChange
* @emits selectionChange
*/
-ve.ce.SurfaceObserver.prototype.poll = function ( postpone, emitContentChanges
) {
+ve.ce.SurfaceObserver.prototype.pollOnce = function () {
var $nodeOrSlug, node, text, hash, range, rangyRange;
-
- if ( this.polling === false ) {
- return;
- }
-
- if ( this.timeoutId !== null ) {
- clearTimeout( this.timeoutId );
- this.timeoutId = null;
- }
-
- if ( postpone === true ) {
- this.timeoutId = setTimeout(
- ve.bind( this.poll, this ),
- 0,
- false,
- emitContentChanges
- );
- return;
- }
range = this.range;
node = this.node;
@@ -183,14 +171,12 @@
text = ve.ce.getDomText( node.$[0] );
hash = ve.ce.getDomHash( node.$[0] );
if ( this.text !== text || this.hash !== hash ) {
- if ( emitContentChanges ) {
- this.emit(
- 'contentChange',
- node,
- { 'text': this.text, 'hash': this.hash,
'range': this.range },
- { 'text': text, 'hash': hash, 'range':
range }
- );
- }
+ this.emit(
+ 'contentChange',
+ node,
+ { 'text': this.text, 'hash': this.hash,
'range': this.range },
+ { 'text': text, 'hash': hash, 'range': range }
+ );
this.text = text;
this.hash = hash;
}
@@ -205,11 +191,4 @@
);
this.range = range;
}
-
- this.timeoutId = setTimeout(
- ve.bind( this.poll, this ),
- this.frequency,
- false,
- emitContentChanges
- );
};
diff --git a/modules/ve/ui/ve.ui.Surface.js b/modules/ve/ui/ve.ui.Surface.js
index 1510eff..a8349cb 100644
--- a/modules/ve/ui/ve.ui.Surface.js
+++ b/modules/ve/ui/ve.ui.Surface.js
@@ -82,7 +82,7 @@
// By re-asserting the current selection and forcing a poll we force
selection to be something
// reasonable - otherwise in Firefox, the initial selection is (0,0),
causing bug 42277
this.model.getFragment().select();
- this.view.surfaceObserver.poll( false, true );
+ this.view.surfaceObserver.pollOnce();
this.model.startHistoryTracking();
};
diff --git a/modules/ve/ve.EventSequencer.js b/modules/ve/ve.EventSequencer.js
index fdb4bc3..83c910b 100644
--- a/modules/ve/ve.EventSequencer.js
+++ b/modules/ve/ve.EventSequencer.js
@@ -25,11 +25,10 @@
* @param {Function} [boundLogFunc] Logging function, pre-bound with ve.bind
*/
ve.EventSequencer = function ( node, eventNames, boundLogFunc ) {
- var i, len, eventName, $node = $( node );
this.node = node;
- this.preListenersForEvent = {};
- this.postListenersForEvent = {};
+ this.$node = $( node );
this.log = boundLogFunc || function () {};
+ this.eventNames = eventNames;
/**
* @property {Object[]}
@@ -39,9 +38,22 @@
* - eventName {string} Name, such as keydown
*/
this.pendingCalls = [];
- for ( i = 0, len = eventNames.length; i < len; i++ ) {
- eventName = eventNames[i];
- $node.on( eventName, ve.bind( this.onEvent, this, eventName ) );
+ this.clearListeners();
+};
+
+/**
+ * Clear all listeners
+ * @method
+ */
+ve.EventSequencer.prototype.clearListeners = function () {
+ var i, len, eventName;
+ this.preListenersForEvent = {};
+ this.postListenersForEvent = {};
+ // TODO: what should we do with pending calls?
+ for ( i = 0, len = this.eventNames.length; i < len; i++ ) {
+ eventName = this.eventNames[i];
+ this.$node.off( eventName );
+ this.$node.on( eventName, ve.bind( this.onEvent, this,
eventName ) );
this.preListenersForEvent[eventName] = [];
this.postListenersForEvent[eventName] = [];
}
--
To view, visit https://gerrit.wikimedia.org/r/82858
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0085e4a53c5a776733dce6944b867b8d2228ba4b
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Divec <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
