fix makeTemplate and test it & template interactions

alexcjohnson · alexcjohnson · commit c2bcfe3e73d1 · 2018-07-01T14:43:36.000+01:00
diff --git a/src/plot_api/make_template.js b/src/plot_api/make_template.js
@@ -12,8 +12,10 @@
 var Lib = require('../lib');
 var isPlainObject = Lib.isPlainObject;
 var PlotSchema = require('./plot_schema');
+var Plots = require('../plots/plots');
 var plotAttributes = require('../plots/attributes');
 var Template = require('./plot_template');
+var dfltConfig = require('./plot_config');
 
 /**
  * Plotly.makeTemplate: create a template off an existing figure to reuse
@@ -29,8 +31,13 @@ var Template = require('./plot_template');
  *     `layout.template` in another figure.
  */
 module.exports = function makeTemplate(figure) {
+    figure = Lib.extendDeep({_context: dfltConfig}, figure);
+    Plots.supplyDefaults(figure);
     var data = figure.data || [];
     var layout = figure.layout || {};
+    // copy over a few items to help follow the schema
+    layout._basePlotModules = figure._fullLayout._basePlotModules;
+    layout._modules = figure._fullLayout._modules;
 
     var template = {
         data: {},
@@ -75,6 +82,7 @@ module.exports = function makeTemplate(figure) {
      * since valid options can be context-dependent. It could be solved with
      * a *list* of values, but that would be huge complexity for little gain.
      */
+    delete template.layout.template;
     var oldTemplate = layout.template;
     if(isPlainObject(oldTemplate)) {
         var oldLayoutTemplate = oldTemplate.layout;
@@ -93,9 +101,9 @@ module.exports = function makeTemplate(figure) {
                     typeLen = typeTemplates.length;
                     oldTypeLen = oldTypeTemplates.length;
                     for(i = 0; i < typeLen; i++) {
-                        mergeTemplates(oldTypeTemplates[i % typeLen], typeTemplates[i]);
+                        mergeTemplates(oldTypeTemplates[i % oldTypeLen], typeTemplates[i]);
                     }
-                    for(; i < oldTypeLen; i++) {
+                    for(i = typeLen; i < oldTypeLen; i++) {
                         typeTemplates.push(Lib.extendDeep({}, oldTypeTemplates[i]));
                     }
                 }
@@ -121,68 +129,120 @@ function mergeTemplates(oldTemplate, newTemplate) {
     var oldKeys = Object.keys(oldTemplate).sort();
     var i, j;
 
+    function mergeOne(oldVal, newVal, key) {
+        if(isPlainObject(newVal) && isPlainObject(oldVal)) {
+            mergeTemplates(oldVal, newVal);
+        }
+        else if(Array.isArray(newVal) && Array.isArray(oldVal)) {
+            // Note: omitted `inclusionAttr` from arrayTemplater here,
+            // it's irrelevant as we only want the resulting `_template`.
+            var templater = Template.arrayTemplater({_template: oldTemplate}, key);
+            for(j = 0; j < newVal.length; j++) {
+                var item = newVal[j];
+                var oldItem = templater.newItem(item)._template;
+                if(oldItem) mergeTemplates(oldItem, item);
+            }
+            var defaultItems = templater.defaultItems();
+            for(j = 0; j < defaultItems.length; j++) newVal.push(defaultItems[j]._template);
+
+            // templateitemname only applies to receiving plots
+            for(j = 0; j < newVal.length; j++) delete newVal[j].templateitemname;
+        }
+    }
+
     for(i = 0; i < oldKeys.length; i++) {
         var key = oldKeys[i];
         var oldVal = oldTemplate[key];
         if(key in newTemplate) {
-            var newVal = newTemplate[key];
-            if(isPlainObject(newVal) && isPlainObject(oldVal)) {
-                mergeTemplates(oldVal, newVal);
-            }
-            else if(Array.isArray(newVal) && Array.isArray(oldVal)) {
-                // Note: omitted `inclusionAttr` from arrayTemplater here,
-                // it's irrelevant as we only want the resulting `_template`.
-                var templater = Template.arrayTemplater({_template: oldTemplate}, key);
-                for(j = 0; j < newVal.length; j++) {
-                    var item = newVal[j];
-                    var oldItem = templater.newItem(item)._template;
-                    if(oldItem) mergeTemplates(oldItem, item);
+            mergeOne(oldVal, newTemplate[key], key);
+        }
+        else newTemplate[key] = oldVal;
+
+        // if this is a base key from the old template (eg xaxis), look for
+        // extended keys (eg xaxis2) in the new template to merge into
+        if(getBaseKey(key) === key) {
+            for(var key2 in newTemplate) {
+                var baseKey2 = getBaseKey(key2);
+                if(key2 !== baseKey2 && baseKey2 === key && !(key2 in oldTemplate)) {
+                    mergeOne(oldVal, newTemplate[key2], key);
                 }
-                var defaultItems = templater.defaultItems();
-                for(j = 0; j < defaultItems.length; j++) newVal.push(defaultItems[j]);
             }
         }
-        else newTemplate[key] = oldVal;
     }
 }
 
-function walkStyleKeys(parent, templateOut, getAttributeInfo, path) {
+function getBaseKey(key) {
+    return key.replace(/[0-9]+$/, '');
+}
+
+function walkStyleKeys(parent, templateOut, getAttributeInfo, path, basePath) {
+    var pathAttr = basePath && getAttributeInfo(basePath);
     for(var key in parent) {
         var child = parent[key];
         var nextPath = getNextPath(parent, key, path);
-        var attr = getAttributeInfo(nextPath);
+        var nextBasePath = getNextPath(parent, key, basePath);
+        var attr = getAttributeInfo(nextBasePath);
+        if(!attr) {
+            var baseKey = getBaseKey(key);
+            if(baseKey !== key) {
+                nextBasePath = getNextPath(parent, baseKey, basePath);
+                attr = getAttributeInfo(nextBasePath);
+            }
+        }
+
+        // we'll get an attr if path starts with a valid part, then has an
+        // invalid ending. Make sure we got all the way to the end.
+        if(pathAttr && (pathAttr === attr)) continue;
 
-        if(!attr ||
+        if(!attr || attr._noTemplating ||
             attr.valType === 'data_array' ||
             (attr.arrayOk && Array.isArray(child))
         ) {
             continue;
         }
 
         if(!attr.valType && isPlainObject(child)) {
-            walkStyleKeys(child, templateOut, getAttributeInfo, nextPath);
+            walkStyleKeys(child, templateOut, getAttributeInfo, nextPath, nextBasePath);
         }
         else if(attr._isLinkedToArray && Array.isArray(child)) {
             var dfltDone = false;
             var namedIndex = 0;
+            var usedNames = {};
             for(var i = 0; i < child.length; i++) {
                 var item = child[i];
                 if(isPlainObject(item)) {
-                    if(item.name) {
-                        walkStyleKeys(item, templateOut, getAttributeInfo,
-                            getNextPath(child, namedIndex, nextPath));
-                        namedIndex++;
+                    var name = item.name;
+                    if(name) {
+                        if(!usedNames[name]) {
+                            // named array items: allow all attributes except data arrays
+                            walkStyleKeys(item, templateOut, getAttributeInfo,
+                                getNextPath(child, namedIndex, nextPath),
+                                getNextPath(child, namedIndex, nextBasePath));
+                            namedIndex++;
+                            usedNames[name] = 1;
+                        }
                     }
                     else if(!dfltDone) {
                         var dfltKey = Template.arrayDefaultKey(key);
-                        walkStyleKeys(item, templateOut, getAttributeInfo,
-                            getNextPath(parent, dfltKey, path));
+                        var dfltPath = getNextPath(parent, dfltKey, path);
+
+                        // getAttributeInfo will fail if we try to use dfltKey directly.
+                        // Instead put this item into the next array element, then
+                        // pull it out and move it to dfltKey.
+                        var pathInArray = getNextPath(child, namedIndex, nextPath);
+                        walkStyleKeys(item, templateOut, getAttributeInfo, pathInArray,
+                            getNextPath(child, namedIndex, nextBasePath));
+                        var itemPropInArray = Lib.nestedProperty(templateOut, pathInArray);
+                        var dfltProp = Lib.nestedProperty(templateOut, dfltPath);
+                        dfltProp.set(itemPropInArray.get());
+                        itemPropInArray.set(null);
+
                         dfltDone = true;
                     }
                 }
             }
         }
-        else if(attr.role === 'style') {
+        else {
             var templateProp = Lib.nestedProperty(templateOut, nextPath);
             templateProp.set(child);
         }
diff --git a/src/plots/attributes.js b/src/plots/attributes.js
@@ -16,7 +16,8 @@ module.exports = {
         role: 'info',
         values: [],     // listed dynamically
         dflt: 'scatter',
-        editType: 'calc+clearAxisTypes'
+        editType: 'calc+clearAxisTypes',
+        _noTemplating: true // we handle this at a higher level
     },
     visible: {
         valType: 'enumerated',
diff --git a/src/plots/cartesian/layout_attributes.js b/src/plots/cartesian/layout_attributes.js
@@ -61,6 +61,11 @@ module.exports = {
         dflt: '-',
         role: 'info',
         editType: 'calc',
+        // we forget when an axis has been autotyped, just writing the auto
+        // value back to the input - so it doesn't make sense to template this.
+        // TODO: should we prohibit this in `coerce` as well, or honor it if
+        // someone enters it explicitly?
+        _noTemplating: true,
         description: [
             'Sets the axis type.',
             'By default, plotly attempts to determined the axis type',
diff --git a/test/image/mocks/template.json b/test/image/mocks/template.json
@@ -13,7 +13,7 @@
         "yaxis2": {"anchor": "x2", "title": "y2"},
         "annotations": [
             {"text": "Hi!", "x": 0, "y": 3.5},
-            {"templateitemname": "watermark", "font": {"size": 120}},
+            {"templateitemname": "watermark", "font": {"size": 120}, "name": "new watermark"},
             {"templateitemname": "watermark", "font": {"size": 110}},
             {"templateitemname": "watermark", "font": {"size": 100}},
             {"templateitemname": "nope", "text": "Buh-bye"}
diff --git a/test/jasmine/tests/plotschema_test.js b/test/jasmine/tests/plotschema_test.js
@@ -115,7 +115,11 @@ describe('plot schema', function() {
                     var valObject = valObjects[attr.valType],
                         opts = valObject.requiredOpts
                             .concat(valObject.otherOpts)
-                            .concat(['valType', 'description', 'role', 'editType', 'impliedEdits', '_compareAsJSON']);
+                            .concat([
+                                'valType', 'description', 'role',
+                                'editType', 'impliedEdits',
+                                '_compareAsJSON', '_noTemplating'
+                            ]);
 
                     Object.keys(attr).forEach(function(key) {
                         expect(opts.indexOf(key) !== -1).toBe(true, key, attr);
diff --git a/test/jasmine/tests/template_test.js b/test/jasmine/tests/template_test.js
