[MediaWiki-commits] [Gerrit] ve.ui.MWTemplateDialog: Implement inferring of template data - change (mediawiki...VisualEditor)

2013-05-30 Thread Krinkle (Code Review)
Krinkle has uploaded a new change for review.

  https://gerrit.wikimedia.org/r/66101


Change subject: ve.ui.MWTemplateDialog: Implement inferring of template data
..

ve.ui.MWTemplateDialog: Implement inferring of template data

Clean up of logic implemented during the template-sprint:

* Store spec inside the content model, directly associated
  with the content-part. This allowed fixing the bug where
  two spec-less template invocations overwrote eachothers
  made-up template data due to it using target.wt as key.
  The opener now provides the fetcher with a specId which
  is set to part/id for wt-generated template targets.

* Batching is now implemented inside the fetcher instead
  of outside. This allows calling getTemplateSpecs inside
  the loop with a dedicated callback for each spec to store
  it in the content.parts[i] object passed by reference.

  It also makes it easier to use by different codepaths.
  You call it as much as you like and it will queue up
  naturally through javascript yielding and then make a batch
  request. This is based on the pattern I used in MediaWiki core
  for mw.loader#addEmbeddedCSS.

Follows-up e7af635, da679b7.

Change-Id: I4d7121229d060a96d927585c987a1a81a474b922
---
M modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
1 file changed, 184 insertions(+), 59 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor 
refs/changes/01/66101/1

diff --git a/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js 
b/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
index 3de1e65..b1e6c75 100644
--- a/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
+++ b/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
@@ -27,7 +27,9 @@
// Properties
this.node = null;
this.content = null;
-   this.specs = {};
+   // Buffer for getTemplateSpecs
+   this.fetchQueue = [];
+   this.fetchCallbacks = $.Callbacks();
 };
 
 /* Inheritance */
@@ -50,47 +52,63 @@
  * @method
  */
 ve.ui.MWTemplateDialog.prototype.onOpen = function () {
-   var i, len, template, title,
-   templates = [];
+   var i, progress, len, template,
+   dialog = this;
 
-   this.node = this.surface.getView().getFocusedNode();
-   if ( !this.node ) {
+   dialog.node = dialog.surface.getView().getFocusedNode();
+   if ( !dialog.node ) {
throw new Error( 'No focused node to edit' );
}
 
-   // Get content values
-   this.content = ve.copyObject( this.node.getModel().getAttribute( 'mw' ) 
);
+   // Get content values and copy it so we can safely change it to our 
liking
+   dialog.content = ve.copyObject( dialog.node.getModel().getAttribute( 
'mw' ) );
+
// Convert single template format to multiple template format
-   if ( this.content.params ) {
-   this.content = { 'parts': [ { 'template': this.content } ] };
+   if ( dialog.content.params ) {
+   dialog.content = {
+   'parts': [
+   {
+   'template': dialog.content
+   }
+   ]
+   };
}
-   // Get all template data asynchronously
-   for ( i = 0, len = this.content.parts.length; i  len; i++ ) {
-   template = this.content.parts[i].template;
-   if ( template ) {
-   if ( template.target.url ) {
-   try {
-   title = new mw.Title( 
template.target.url );
-   templates.push( {
-   'title': title.toString(),
-   'params': template.params
-   } );
-   } catch ( e ) {}
-   }
-   } else {
-   // Wrap plain wikitext in object so editor has 
something to reference
-   this.content.parts[i] = { 'wt': this.content.parts[i] };
+
+   progress = -1;
+   len = dialog.content.parts.length;
+
+   function increaseProgress() {
+   progress++;
+   if ( progress === len ) {
+   dialog.setupPages();
}
}
-   if ( templates.length ) {
-   this.getTemplateData( templates )
-   .done( ve.bind( function ( specs ) {
-   this.specs = specs;
-   }, this ) )
-   .always( ve.bind( this.setupPages, this ) );
-   } else {
-   this.setupPages();
+
+   function makeStoreTemplateSpec( template ) {
+   return function ( specs ) {
+   template.spec = specs[ template.specId ];
+   

[MediaWiki-commits] [Gerrit] ve.ui.MWTemplateDialog: Implement inferring of template data - change (mediawiki...VisualEditor)

2013-05-30 Thread jenkins-bot (Code Review)
jenkins-bot has submitted this change and it was merged.

Change subject: ve.ui.MWTemplateDialog: Implement inferring of template data
..


ve.ui.MWTemplateDialog: Implement inferring of template data

Clean up of logic implemented during the template-sprint:

* Store spec inside the content model, directly associated
  with the content-part. This allowed fixing the bug where
  two spec-less template invocations overwrote each other's
  made-up template data due to it using target.wt as key.
  The opener now provides the fetcher with a specId which
  is set to part/id for wt-generated template targets.

* Batching is now implemented inside the fetcher instead
  of outside. This allows calling getTemplateSpecs inside
  the loop with a dedicated callback for each spec to store
  it in the content.parts[i] object passed by reference.

  It also makes it easier to use by different code paths.
  You call it as much as you like and it will queue up
  naturally through javascript yielding and then make a batch
  request. This is based on the pattern I used in MediaWiki core
  for mw.loader#addEmbeddedCSS.

Follows-up e7af635, da679b7.

Change-Id: I4d7121229d060a96d927585c987a1a81a474b922
---
M modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
1 file changed, 189 insertions(+), 65 deletions(-)

Approvals:
  Catrope: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js 
b/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
index 3de1e65..eff6f6d 100644
--- a/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
+++ b/modules/ve/ui/dialogs/ve.ui.MWTemplateDialog.js
@@ -27,7 +27,9 @@
// Properties
this.node = null;
this.content = null;
-   this.specs = {};
+   // Buffer for getTemplateSpecs
+   this.fetchQueue = [];
+   this.fetchCallbacks = $.Callbacks();
 };
 
 /* Inheritance */
@@ -50,77 +52,111 @@
  * @method
  */
 ve.ui.MWTemplateDialog.prototype.onOpen = function () {
-   var i, len, template, title,
-   templates = [];
+   var i, progress, len, template,
+   dialog = this;
 
-   this.node = this.surface.getView().getFocusedNode();
-   if ( !this.node ) {
+   function increaseProgress() {
+   progress++;
+   if ( progress === len ) {
+   dialog.setupPages();
+   }
+   }
+
+   function makeStoreTemplateSpec( template ) {
+   return function ( specs ) {
+   template.spec = specs[template.specId];
+   increaseProgress();
+   };
+   }
+
+   dialog.node = dialog.surface.getView().getFocusedNode();
+   if ( !dialog.node ) {
throw new Error( 'No focused node to edit' );
}
 
-   // Get content values
-   this.content = ve.copyObject( this.node.getModel().getAttribute( 'mw' ) 
);
+   // Get content values and copy it so we can safely change it to our 
liking
+   dialog.content = ve.copyObject( dialog.node.getModel().getAttribute( 
'mw' ) );
+
// Convert single template format to multiple template format
-   if ( this.content.params ) {
-   this.content = { 'parts': [ { 'template': this.content } ] };
+   if ( dialog.content.params ) {
+   dialog.content = {
+   'parts': [
+   {
+   'template': dialog.content
+   }
+   ]
+   };
}
-   // Get all template data asynchronously
-   for ( i = 0, len = this.content.parts.length; i  len; i++ ) {
-   template = this.content.parts[i].template;
+
+   progress = -1;
+   len = dialog.content.parts.length;
+
+   // Get all template specs asynchronously
+   for ( i = 0; i  len; i++ ) {
+   template = dialog.content.parts[i].template;
if ( template ) {
-   if ( template.target.url ) {
-   try {
-   title = new mw.Title( 
template.target.url );
-   templates.push( {
-   'title': title.toString(),
-   'params': template.params
-   } );
-   } catch ( e ) {}
-   }
+   // Method #getTemplateSpecs will use the part id 
instead of `target.url`
+   // if the target has no url property (which Parsoid 
omits if the target is
+   // dynamically generated from wikitext). In that case 
we want each template
+   // invocation to have its own inferred template spec.
+   template.specId =