From 7673a39878cec0ff4071511b9e4b9f33d24da566 Mon Sep 17 00:00:00 2001 From: Roan Kattouw Date: Sat, 3 Aug 2013 16:05:51 -0700 Subject: [PATCH] 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 --- .../ce/nodes/ve.ce.MWTransclusionNode.js | 14 +- modules/ve-mw/test/dm/ve.dm.mwExample.js | 8 +- modules/ve/ce/nodes/ve.ce.AlienNode.js | 2 +- .../ve/ce/nodes/ve.ce.GeneratedContentNode.js | 149 +++++++++++++----- .../ve/dm/nodes/ve.dm.GeneratedContentNode.js | 2 +- 5 files changed, 126 insertions(+), 49 deletions(-) diff --git a/modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js b/modules/ve-mw/ce/nodes/ve.ce.MWTransclusionNode.js index 61a53f73c3..7b9452abb3 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 @@ ve.ce.MWTransclusionNode.static.renderHtmlAttributes = false; /* 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 @@ ve.ce.MWTransclusionNode.prototype.generateContents = function () { '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 af5a244d3d..64fe39648f 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.mixedDataOpen = { }; 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 @@ ve.dm.mwExample.domToDataCases = { ], 'storeItems': [ { - 'hash': '{"mw":{"params":{"1":{"wt":"foo"}}},"type":"mwTransclusionBlock"}', + 'hash': '[{"mw":{"params":{"1":{"wt":"foo"}}},"type":"mwTransclusionBlock"},null]', 'value': $( '

foo

' ).toArray() } ] diff --git a/modules/ve/ce/nodes/ve.ce.AlienNode.js b/modules/ve/ce/nodes/ve.ce.AlienNode.js index ce64ab015e..552d751e0f 100644 --- a/modules/ve/ce/nodes/ve.ce.AlienNode.js +++ b/modules/ve/ce/nodes/ve.ce.AlienNode.js @@ -53,7 +53,7 @@ ve.ce.AlienNode.static.$phantomTemplate = ve.ce.AlienNode.static.$phantomTemplat * * @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 395b8cb6e1..fc207d58f4 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 @@ ve.ce.GeneratedContentNode = function VeCeGeneratedContentNode() { * @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 @@ ve.ce.GeneratedContentNode.prototype.startGenerating = function () { * * @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 bba784b4a8..e64ae20a27 100644 --- a/modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js +++ b/modules/ve/dm/nodes/ve.dm.GeneratedContentNode.js @@ -29,6 +29,6 @@ ve.dm.GeneratedContentNode.static = {}; * @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