jenkins-bot has submitted this change and it was merged.
Change subject: Support previews and concurrent updates in
ce.GeneratedContentNode
......................................................................
Support previews and concurrent updates in ce.GeneratedContentNode
GeneratedContentNode didn't track concurrent updates at all, so a
race condition was possible: if the node was updated a second time
before the first update had been rendered, the second update might
render first and then be overwritten by the other one.
To prevent this, we track the promise associated with the current
render. If a new update is launched while a previous one is still
pending we attempt to abort the old one by calling .abort() on it,
and ignore any future resolution or rejection from it.
Also allow rerenders based on non-model data by calling
.update( { config object } );
Change-Id: I8feefd9e8fb6c41d06b8b20131e3be5e37954e83
---
M modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js
M modules/ve-mw/test/dm/ve.dm.mwExample.js
M modules/ve/ce/nodes/ve.ce.AlienNode.js
M modules/ve/ce/nodes/ve.ce.GeneratedContentNode.js
M modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js
5 files changed, 126 insertions(+), 49 deletions(-)
Approvals:
Jiabao: Looks good to me, but someone else must approve
Trevor Parscal: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js
b/modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js
index 61a53f7..7b9452a 100644
--- a/modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js
+++ b/modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js
@@ -53,15 +53,15 @@
/* Methods */
/** */
-ve.ce.MWTransclusionNode.prototype.generateContents = function () {
- var deferred = $.Deferred();
- $.ajax( {
+ve.ce.MWTransclusionNode.prototype.generateContents = function ( config ) {
+ var xhr, promise, deferred = $.Deferred();
+ xhr = $.ajax( {
'url': mw.util.wikiScript( 'api' ),
'data': {
'action': 'visualeditor',
'paction': 'parsefragment',
'page': mw.config.get( 'wgRelevantPageName' ),
- 'wikitext': this.model.getWikitext(),
+ 'wikitext': ( config && config.wikitext ) ||
this.model.getWikitext(),
'token': mw.user.tokens.get( 'editToken' ),
'format': 'json'
},
@@ -73,7 +73,11 @@
'success': ve.bind( this.onParseSuccess, this, deferred ),
'error': ve.bind( this.onParseError, this, deferred )
} );
- return deferred.promise();
+ promise = deferred.promise();
+ promise.abort = function () {
+ xhr.abort();
+ };
+ return promise;
};
/**
diff --git a/modules/ve-mw/test/dm/ve.dm.mwExample.js
b/modules/ve-mw/test/dm/ve.dm.mwExample.js
index af5a244..64fe396 100644
--- a/modules/ve-mw/test/dm/ve.dm.mwExample.js
+++ b/modules/ve-mw/test/dm/ve.dm.mwExample.js
@@ -108,19 +108,19 @@
};
ve.dm.mwExample.MWTransclusion.mixedDataClose = { 'type':
'/mwTransclusionInline' };
-ve.dm.mwExample.MWTransclusion.blockParamsHash = ve.getHash(
ve.dm.MWTransclusionNode.static.getHashObject(
ve.dm.mwExample.MWTransclusion.blockData ) );
+ve.dm.mwExample.MWTransclusion.blockParamsHash = ve.getHash( [
ve.dm.MWTransclusionNode.static.getHashObject(
ve.dm.mwExample.MWTransclusion.blockData ), undefined ] );
ve.dm.mwExample.MWTransclusion.blockStoreItems = {
'hash': ve.dm.mwExample.MWTransclusion.blockParamsHash,
'value': $( ve.dm.mwExample.MWTransclusion.blockOpen +
ve.dm.mwExample.MWTransclusion.blockContent ).toArray()
};
-ve.dm.mwExample.MWTransclusion.inlineParamsHash = ve.getHash(
ve.dm.MWTransclusionNode.static.getHashObject(
ve.dm.mwExample.MWTransclusion.inlineData ) );
+ve.dm.mwExample.MWTransclusion.inlineParamsHash = ve.getHash( [
ve.dm.MWTransclusionNode.static.getHashObject(
ve.dm.mwExample.MWTransclusion.inlineData ), undefined ] );
ve.dm.mwExample.MWTransclusion.inlineStoreItems = {
'hash': ve.dm.mwExample.MWTransclusion.inlineParamsHash,
'value': $( ve.dm.mwExample.MWTransclusion.inlineOpen +
ve.dm.mwExample.MWTransclusion.inlineContent +
ve.dm.mwExample.MWTransclusion.inlineClose ).toArray()
};
-ve.dm.mwExample.MWTransclusion.mixedParamsHash = ve.getHash(
ve.dm.MWTransclusionNode.static.getHashObject(
ve.dm.mwExample.MWTransclusion.mixedDataOpen ) );
+ve.dm.mwExample.MWTransclusion.mixedParamsHash = ve.getHash( [
ve.dm.MWTransclusionNode.static.getHashObject(
ve.dm.mwExample.MWTransclusion.mixedDataOpen ), undefined ] );
ve.dm.mwExample.MWTransclusion.mixedStoreItems = {
'hash': ve.dm.mwExample.MWTransclusion.mixedParamsHash,
'value': $( ve.dm.mwExample.MWTransclusion.mixed ).toArray()
@@ -805,7 +805,7 @@
],
'storeItems': [
{
- 'hash':
'{"mw":{"params":{"1":{"wt":"foo"}}},"type":"mwTransclusionBlock"}',
+ 'hash':
'[{"mw":{"params":{"1":{"wt":"foo"}}},"type":"mwTransclusionBlock"},null]',
'value': $( '<p about="#mwt1"
typeof="mw:Transclusion"
data-mw="{"params":{"1":{"wt":"foo"}}}"
data-parsoid="1">foo</p>' ).toArray()
}
]
diff --git a/modules/ve/ce/nodes/ve.ce.AlienNode.js
b/modules/ve/ce/nodes/ve.ce.AlienNode.js
index ce64ab0..552d751 100644
--- a/modules/ve/ce/nodes/ve.ce.AlienNode.js
+++ b/modules/ve/ce/nodes/ve.ce.AlienNode.js
@@ -53,7 +53,7 @@
*
* @method
*/
-ve.ce.AlienNode.prototype.onUpdate = function () {
+ve.ce.AlienNode.prototype.update = function () {
// TODO use GeneratedContentNode the way it was meant to be used
this.$.html( ve.copyDomElements( this.model.getAttribute( 'domElements'
) || [], this.getElementDocument() ) );
};
diff --git a/modules/ve/ce/nodes/ve.ce.GeneratedContentNode.js
b/modules/ve/ce/nodes/ve.ce.GeneratedContentNode.js
index 395b8cb..fc207d5 100644
--- a/modules/ve/ce/nodes/ve.ce.GeneratedContentNode.js
+++ b/modules/ve/ce/nodes/ve.ce.GeneratedContentNode.js
@@ -14,15 +14,18 @@
* @constructor
*/
ve.ce.GeneratedContentNode = function VeCeGeneratedContentNode() {
+ // Properties
+ this.generatingPromise = null;
+
// DOM Changes
this.$.addClass( 've-ce-generatedContentNode' );
this.$.attr( 'contenteditable', false );
// Events
- this.model.connect( this, { 'update': 'onUpdate' } );
+ this.model.connect( this, { 'update': 'update' } );
// Initialization
- this.onUpdate();
+ this.update();
};
/* Events */
@@ -39,49 +42,115 @@
* @event rerender
*/
-/* Methods */
-
-/**
- * Handle update events.
- *
- * @method
- * @emits setup
- * @emits teardown
- * @emits rerender
- */
-ve.ce.GeneratedContentNode.prototype.onUpdate = function () {
- var doc = this.getElementDocument(),
- store = this.model.doc.getStore(),
- index = store.indexOfHash( ve.getHash( this.model ) );
- if ( index !== null ) {
- if ( this.live ) {
- this.emit( 'teardown' );
- }
- this.$.empty().append(
- ve.copyDomElements( store.value( index ), doc )
- );
- if ( this.live ) {
- this.emit( 'setup' );
- this.emit( 'rerender' );
- }
- } else {
- this.startGenerating();
- this.generateContents()
- .done( ve.bind( this.doneGenerating, this ) )
- .fail( ve.bind( this.failGenerating, this ) );
- }
-};
+/* Abstract methods */
/**
* Start a deferred process to generate the contents of the node.
- * @returns {jQuery.Promise} Promise object
+ *
+ * If successful, the returned promise must be resolved with the generated DOM
elements passed
+ * in as the first parameter, i.e. promise.resolve( domElements ); . Any other
parameters to
+ * .resolve() are ignored.
+ *
+ * If the returned promise object is abortable (has an .abort() method),
.abort() will be called if
+ * a newer update is started before the current update has finished. When a
promise is aborted, it
+ * should cease its work and shouldn't be resolved or rejected. If an outdated
update's promise
+ * is resolved or rejected anyway (which may happen if an aborted promise
misbehaves, or if the
+ * promise wasn't abortable), this is ignored and
doneGenerating()/failGenerating() is not called.
+ *
+ * Additional data may be passed in the config object to instruct this
function to render something
+ * different than what's in the model. This data is implementation-specific
and is passed through
+ * by forceUpdate().
+ *
+ * @abstract
+ * @param {Object} [config] Optional additional data
+ * @returns {jQuery.Promise} Promise object, may be abortable
*/
ve.ce.GeneratedContentNode.prototype.generateContents = function () {
throw new Error( 've.ce.GeneratedContentNode subclass must implement
generateContents' );
};
+/* Methods */
+
+/**
+ * Rerender the contents of this node.
+ *
+ * @param {HTMLElement[]} domElements Array of DOM elements
+ * @emits setup
+ * @emits teardown
+ * @emits rerender
+ */
+ve.ce.GeneratedContentNode.prototype.render = function ( domElements ) {
+ var doc = this.getElementDocument();
+ if ( this.live ) {
+ this.emit( 'teardown' );
+ }
+ this.$.empty().append( ve.copyDomElements( domElements, doc ) );
+ if ( this.live ) {
+ this.emit( 'setup' );
+ this.emit( 'rerender' );
+ }
+};
+
+/**
+ * Update the contents of this node based on the model and config data. If
this combination of
+ * model and config data has been rendered before, the cached rendering in the
store will be used.
+ *
+ * @param {Object} [config] Optional additional data to pass to
generateContents()
+ */
+ve.ce.GeneratedContentNode.prototype.update = function ( config ) {
+ var store = this.model.doc.getStore(),
+ index = store.indexOfHash( ve.getHash( [ this.model, config ] )
);
+ if ( index !== null ) {
+ this.render( store.value( index ) );
+ } else {
+ this.forceUpdate();
+ }
+};
+
+/**
+ * Force the contents to be updated. Like update(), but bypasses the store.
+ *
+ * @param {Object} [config] Optional additional data to pass to
generateContents()
+ */
+ve.ce.GeneratedContentNode.prototype.forceUpdate = function ( config ) {
+ var promise, node = this;
+ if ( this.generatingPromise ) {
+ // Abort the currently pending generation process if possible
+ // Unset this.generatingPromise first so that if the promise is
resolved or rejected
+ // when we abort, this is ignored as it should be
+ promise = this.generatingPromise;
+ this.generatingPromise = null;
+ if ( $.isFunction( promise.abort ) ) {
+ promise.abort();
+ }
+ } else {
+ // Only call startGenerating() if we weren't generating before
+ this.startGenerating();
+ }
+
+ // Create a new promise
+ promise = this.generatingPromise = this.generateContents( config );
+ promise
+ // If this promise is no longer the currently pending one,
ignore it completely
+ .done( function ( domElements ) {
+ if ( node.generatingPromise === promise ) {
+ node.doneGenerating( domElements, config );
+ }
+ } )
+ .fail( function () {
+ if ( node.generatingPromise === promise ) {
+ node.failGenerating();
+ }
+ } );
+};
+
/**
* Called when the node starts generating new content.
+ *
+ * This function is only called when the node wasn't already generating
content. If a second update
+ * comes in, this function will only be called if the first update has already
finished (i.e.
+ * doneGenerating or failGenerating has already been called).
+ *
* @method
*/
ve.ce.GeneratedContentNode.prototype.startGenerating = function () {
@@ -93,19 +162,23 @@
*
* @method
* @param {HTMLElement[]} domElements Generated content
+ * @param {Object} [config] Config object passed to forceUpdate()
*/
-ve.ce.GeneratedContentNode.prototype.doneGenerating = function ( domElements )
{
+ve.ce.GeneratedContentNode.prototype.doneGenerating = function ( domElements,
config ) {
var store = this.model.doc.getStore(),
- hash = ve.getHash( this.model );
+ hash = ve.getHash( [ this.model, config ] );
store.index( domElements, hash );
// TODO: remove 'generating' style
- this.onUpdate();
+ this.generatingPromise = null;
+ this.render( domElements );
};
/**
* Called when the has failed to generate new content.
+ *
* @method
*/
ve.ce.GeneratedContentNode.prototype.failGenerating = function () {
// TODO: remove 'generating' style
+ this.generatingPromise = null;
};
\ No newline at end of file
diff --git a/modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js
b/modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js
index bba784b..e64ae20 100644
--- a/modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js
+++ b/modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js
@@ -29,6 +29,6 @@
* @returns {number} Index of stored data
*/
ve.dm.GeneratedContentNode.static.storeDomElements = function ( dataElement,
domElements, store ) {
- var hash = ve.getHash( this.getHashObject( dataElement ) );
+ var hash = ve.getHash( [ this.getHashObject( dataElement ), undefined ]
);
return store.index( domElements, hash );
};
\ No newline at end of file
--
To view, visit https://gerrit.wikimedia.org/r/77564
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8feefd9e8fb6c41d06b8b20131e3be5e37954e83
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Catrope <[email protected]>
Gerrit-Reviewer: Jiabao <[email protected]>
Gerrit-Reviewer: Trevor Parscal <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
