restyle/relayout refactor #1999

alexcjohnson · 2017-09-13T06:03:17Z

Something we've wanted to do for a looong time... get (nearly all) the attribute names and regex tests out of _restyle and _relayout, and use the schema instead:

We already had editType as an optional field in the attribute schema, this makes it mandatory and uses it as the key way to determine what flavor of redraw to invoke.
Adds a new optional field impliedEdits for correlated changes - for example range has impliedEdits: {autorange: false} because setting an explicit range is expected to disable the associated autorange. Note that the converse autorange.impliedEdits: {range: undefined} is used as it was before just to record the previous values for use in undo/redo, which we've discussed removing in v2.

Also includes a fix for #1325 as that came along essentially for free with generalizing our attribute string regexes.

I want to do a bit more testing of specific restyle/relayout calls to make sure we're doing the right redraws in all cases - especially of base/module interactions like hoverlabel, and module/module interactions like heatmapgl/colorbar. But I'd appreciate a high-level review ASAP @etpinard @rreusser

…yle/relayout

rreusser · 2017-09-13T15:40:21Z

src/components/annotations/attributes.js

@@ -21,6 +20,7 @@ module.exports = {
        valType: 'boolean',
        role: 'info',
        dflt: true,
+        editType: 'docalcAutorange',


Would docalc+autorange be a sensible name for this? It sounds like an enum camelcased and concatenated, but I could be mistaken.

Ah, I see. Unless I'm mistaken, this adds no new labels. That's just plugging into what already exists. Still seems like maybe an enum in disguise, but nothing to change here then.

One additional note while digging through here is that the do- prefix seems to be carried forward from what was originally just internal variable names. Would editType: 'calcAutorange' or editType: 'calc+autorange' make sense? It seems a bit friendlier if you're trying to make sense of the schema as a standalone concept.

docalcAutorange is a single thing, means "do a recalc only if there's an autoranged axis related to this attribute" - Actually, ideally we'd specify what to do if there isn't an autoranged axis... so like docalcAutorange+dostyle or something.

Would editType: 'calcAutorange' make sense?

Yeah totally, good idea! And to help us out (to figure out what editType a new attribute should get) I should probably describe somewhere what these flags actually mean in terms of the code paths they lead to.

Maybe calcIfAutorange?

to figure out what editType a new attribute should get

FWIW, I have a history of making bad choices here for lack of really understanding the implications of some schema concepts. If you want I can try making docs/schema.md that explains, at the very least, what some of the schema items link up with. Some of the concepts like _isLinkedToArray get a little subtle. Even for the fifth time interacting with it, I think a brief description with links to a couple relevant spots in the code would help.

Maybe calcIfAutorange?

🏆

If you want I can try making docs/schema.md

Definitely time for something like that! @etpinard thoughts about how/where to do this? Should it be a standalone thing, or somehow included in Plotly.PlotSchema.get() or...?

My vote would be for not including in the code so that it's as accessible and readable as possible without having to realize it's an API function and instantiate anything in order to access.

I'd vote for adding a attribute-like edit type declaration object in edit_types.js with a description field. For example,

var traceEditTypes: { docalc: { description: [ // .... ].join(' ') }, // .... }

That description field would get stripped off by the compress_attributes transform. Plotly.PlotSchema.get().defs.editTypes would then return the list of edit types with their description.

I see your point @rreusser about relying an API function to show docs being less-than-ideal. But since we already have a hard time keeping https://github.com/plotly/documentation/ up to speed, adding another layer of static (e.g. a github wiki or a docs/ directory) documentation sounds dangerous. No docs is better than bad docs, right? That said, it wouldn't hurt adding a few lines in our CONTRIBUTING.md over the edit types.

rreusser · 2017-09-13T15:44:20Z

src/components/annotations/attributes.js

-    font: extendFlat({}, fontAttrs, {
+    font: fontAttrs({
+        editType: 'docalcAutorange',
+        colorEditType: 'doarraydraw',


Could you point me toward what doarraydraw does? It's not entirely clear from the name, and if it appears somewhere else in the source, it's not jumping out at me.

Never mind. Found it in the suppressed diff.

Wait, where is doarraydraw being used? I can't find it (even in the suppressed diffs 🙃 )

I think at one point I had none instead of doarraydraw but that looked weird. Then at another point I had a block just for debugging that checked that everything labeled doarraydraw was captured by an arrayContainers block and threw an error otherwise, but I removed that once I was satisfied that it was never getting hit... happy to change it if you think it would make more sense another way.

If I understand correctly, doarraydraw is just a placeholder for edits in array containers that are handled by applyContainerArrayChanges in manage_array.js.

In this case, I prefer setting editType: 'doarraydraw' over editType: 'none'. Just make sure that this behavior is documented in the edit type description field (cc #1999 (comment)).

rreusser · 2017-09-13T15:50:08Z

src/components/colorscale/attributes.js

@@ -13,6 +13,8 @@ module.exports = {
        valType: 'boolean',
        role: 'info',
        dflt: true,
+        editType: 'docalc',
+        impliedEdits: {zmin: undefined, zmax: undefined},


Same with impliedEdits. I assume these are attributes that are automatically applied within the same attr path, but I'm unable to find where it's implemented and it's not immediately clear what the ^ prefix means.

Never mind. Github was suppressing the plot_api diff. Found it.

rreusser · 2017-09-13T15:55:52Z

src/components/fx/layout_attributes.js

-            }),
-            color: extendFlat({}, fontAttrs.color)
-        },
+        font: fontAttrs,


rreusser · 2017-09-13T16:03:30Z

src/lib/relative_attr.js

+ *   an attribute string, such as 'annotations[3].x'. The "current location"
+ *   is the attribute string minus the last component ('annotations[3]')
+ * @param {string} relativeAttr:
+ *   a route to the desired attribute string, using '^' to ascend


Ah, I think I understand. So relative to a.b.c, ^foo would be a.foo. I don't know a solution, but one challenge with the schema is that it implements a lot of very precise semantics that can only be understood by locating the part of the code that handles them and working backwards (which isn't always possible since you might not even know that python is doing something with role, for example). I'm tempted to take a stab at docs/schema.md.

rreusser · 2017-09-13T16:05:04Z

src/lib/relative_attr.js

+'use strict';
+
+
+var ASCEND = /^(.*)(\.[^\.\[\]]+|\[\d\])$/;


Could you add a very short comment about what this matches? In particular figuring out whether ^ is negation or an actual caret takes a good amount of staring and recalling js-specific regex escaping rules since this particular use could very reasonably involve either one.

Re-reading, matches foo.string or foo[5] or foo.bar[5], I think, while capturing everything before the last chunk (as well as the last chunk but only so it can be thrown away). It's not so bad, I guess. Oh, so ASCEND refers to the way it chops off the last attr chunk. I see.

rreusser · 2017-09-13T16:15:21Z

src/plot_api/plot_api.js

+
+            if(valObject && valObject.impliedEdits && newVal !== null) {
+                for(var impliedKey in valObject.impliedEdits) {
+                    doextra(Lib.relativeAttr(ai, impliedKey), valObject.impliedEdits[impliedKey], i);


One question about implied edits is whether it moves away from declarative plots. That is, if you set foo (which, e.g., implicitly also sets bar: false), is it valid to then set bar: true? Is there pathway that might accidentally circumvent the implied edits and lead to a bit of inconsistency? Would it be better/worse to think of implied edits as a concern of the plotly API consumer rather than the plotly API itself? Like the workspace would recognize the attribute you're modifying has an implied edit and send both of those to the API? It seems nice from an end-user perspective; my only concern is that it might be surprising/confusing to modify an attribute and find that additional changes have been made to your plot state.

This PR by itself isn't moving toward or away from anything as far as users are concerned... all of these impliedEdits were previously handled by dedicated blocks in _restyle/_relayout. Without them, a call like relayout(gd, 'xaxis.range', [5, 10]) wouldn't do anything if xaxis.autorange was true.

Looking ahead though to Plotly.react, I'm hoping that by having impliedEdits in the schema we can help people make these implied changes in a transparent way - so if you have a range control that modifies the state and passes it back to the plot in Plotly.react, it would be able to look up the implied changes we think it should make, but the control would have final say over whether to apply those changes or not.

Got it. Thanks for the clarification.

rreusser · 2017-09-13T16:16:36Z

src/plot_api/plot_api.js

                if(valObject) {
-                    /*
-                     * In occasional cases we can't the innermost valObject


"can't ____ the innermost…"

Oh never mind. This is in the removed section. 👍

rreusser · 2017-09-13T16:20:04Z

src/plot_api/plot_schema.js

@@ -208,6 +216,182 @@ exports.findArrayAttributes = function(trace) {
    return arrayAttributes;
 };

+/*
+ * Find the valObject for one attribute in an existing trace


Is a valObject just a value? Or is it a nested property object? Or the attribute definition?
Or something else?

Less vaguely, valObjects are defined here, but I'm trying to process how the return value relates to those definitions.

hah yeah, coerce.valObjects describes valObjects but it isn't itself at set of valObjects. What we mean by it in this context is a schema entry describing one attribute. I'll see what I can do in terms of documenting this better...

Not a big deal. Just another part of the code I haven't gone too deep in. To clear up a common misconception though, naming in CS is a very easy problem (randomly generated strings will do). Naming for maximum interpretability is the hard part. 😄

I feel like coerce.valObjects should be named coerce.valObjectMeta instead. To me and to PlotSchema.isValObject a val object is any object with a valType key - which is essentially what getTraceValObject and getLayoutValObject return.

valObjectMeta - and descriptions for editType and impliedEdits that make their way into the schema - in 96cc57f

etpinard · 2017-09-19T14:08:17Z

src/plot_api/plot_api.js

+                var typeAttr = axAttr + '.type';
+
+                if(layoutUpdate[axAttr] === undefined && layoutUpdate[typeAttr] === undefined) {
+                    Lib.nestedProperty(gd.layout, typeAttr).set(null);


Much better than delete /* */ 👌

etpinard · 2017-09-19T14:15:13Z

package.json

@@ -119,6 +119,7 @@
    "gzip-size": "^3.0.0",
    "image-size": "^0.5.1",
    "jasmine-core": "^2.4.1",
+    "jsdom": "^9.5.0",


Why not use the latest version jsdom@11?

Because 9.5.0 is what we have in streambed, and it looked like it was going to take me more than 5 minutes to figure out the new API 🙈

But you're right, not exactly a good reason - I'll try to upgrade it.

alexcjohnson · 2017-09-19T15:57:36Z

tasks/util/make_schema.js

+        // jsdom by itself doesn't support getContext, and adding the npm canvas
+        // package is annoying and platform-dependent.
+        // see https://github.com/tmpvar/jsdom/issues/1782
+        w.HTMLCanvasElement.prototype.getContext = function() { return null; };


dunno why it didn't happen with the old version, but with the new jsdom without this patch you get an ugly but nonblocking error Error: Not implemented: HTMLCanvasElement.prototype.getContext (without installing the canvas npm package)

Yep, no need to add a dependency for this 👌

etpinard · 2017-09-19T16:03:35Z

src/plot_api/plot_api.js

@@ -2152,10 +2154,6 @@ Plotly.update = function update(gd, traceUpdate, layoutUpdate, traces) {
 // so we auto-set them again
 var axLetters = ['x', 'y', 'z'];
 function clearAxisTypes(gd, traces, layoutUpdate) {
-    if(typeof traces === 'number') traces = [traces];


closes plotly/streambed#8975 (but the fix was earlier in this PR)

etpinard · 2017-09-20T16:09:24Z

test/jasmine/assets/check_ticks.js

+
+var d3 = require('d3');
+
+module.exports = function checkTicks(axLetter, vals, msg) {


I've been trying to centralized common custom assertions like this one in:

https://github.com/plotly/plotly.js/blob/master/test/jasmine/assets/custom_assertions.js

I'll let decide whether separate files or a common module is better for stuff like this.

oh that's nice - yeah, I'll put checkTicks in there.

etpinard · 2017-09-20T16:14:06Z

test/jasmine/assets/negate_if.js

+ *   if(xIsSomethingElse) expect(x).not.toBe(0);
+ *   else expect(x).toBe(0);
+ */
+module.exports = function negateIf(expectation, negate) {


Non-blocking but I wonder if there's a way to rewrite this as a custom jasmine matcher. So that we have something like:

expect(x).negateIf(negate).toBe(0)

expect(x).negateIf(negate).toBe(0) would definitely be more readable - I'll give it a shot.

etpinard · 2017-09-20T16:17:43Z

test/jasmine/tests/histogram_test.js

@@ -463,6 +463,10 @@ describe('Test histogram', function() {
    describe('plot / restyle', function() {
        var gd;

+        beforeAll(function() {
+            jasmine.addMatchers(customMatchers);


(note-to-self comment) I'm getting tired of these jasmine.addMatchers. I'll try to find a way to add our custom matchers globally once and for all.

❤️ 🎉 🥇 👏 💯

global addMatchers, and turning negateIf into a method (which I could only see how to do globally) ->0729921 hopefully it works right on circle 🙏

browserify + requirejs doesn't work.

etpinard · 2017-09-20T19:01:25Z

💃 💃 💃

alexcjohnson added 17 commits September 13, 2017 01:22

test - and fix - most of the relayout doextras

4d3e2ec

test - and fix - most of the doextra calls in restyle

95d7d71

test that xaxis-only items are only in the xaxis in the schema

3326ecc

merge component attribute schemas into core at register time

a948cce

fix lib test for undefined -> null in undoqueue

dd52922

abstract id/name counter regex and standardize cartesian attrRegex

f90f079

fix #1325 - animating multiple axes

e036fea

Plotschema getTraceValObject and getLayoutValObject methods

69e0188

relativeAttr

bbfe399

fix annotation comments/descriptions

29931ec

make common hover label pick up changes quicker

fad72a2

edit_types.overrideAll

e895b32

let PlotSchema.crawl report the complete attribute string

7a7dc6d

better reporting from hover label test

c87b01a

fix registry for new circular dep

658e5cb

massive commit to lock in editType and impliedEdits and clean up rest…

f49ae5e

…yle/relayout

lint

7ea0d25

alexcjohnson added status: in progress bug something broken labels Sep 13, 2017

etpinard mentioned this pull request Sep 13, 2017

Contour throws exceptions when trying to infer x and y axis types after switching from numerical to category. #615

Closed

rreusser reviewed Sep 13, 2017

View reviewed changes

etpinard reviewed Sep 19, 2017

View reviewed changes

alexcjohnson added 4 commits September 19, 2017 10:40

coerceTraceIndices earlier so clearAxisTypes can use it

6e8a68c

move clearAxisTypes into helpers

4f8fc66

update jsdom to v11.2 with new API

97ddf48

oops didn't mean to commit that commented out...

fe7db79

alexcjohnson commented Sep 19, 2017

View reviewed changes

etpinard reviewed Sep 19, 2017

View reviewed changes

alexcjohnson added 6 commits September 19, 2017 15:20

fix and test errorbar visibility toggling

68f5dbc

layout.showlegend test

d15e541

closes #615 - something else in this PR fixed it, just nailing a test

7ec1634

fix #358 - restyling orientation

a107466

test histogram changing data type

42805c2

closes plotly/streambed#8975 (but the fix was earlier in this PR)

fix #2020 - editing plots with multiple histograms

8d9feaf

This was referenced Sep 20, 2017

wishlist for potential breaking changes since v1 #420

Closed

restyle/relayout open items #2025

Closed

lint

0a98a6d

etpinard reviewed Sep 20, 2017

View reviewed changes

alexcjohnson added 3 commits September 20, 2017 12:22

move checkTicks into custom_assertions

e4227aa

load custom_matchers globally, and refactor negateIf as a method

0729921

pull custom_matchers out of requirejs bundle test

407ae5a

browserify + requirejs doesn't work.

alexcjohnson merged commit f81ca4d into master Sep 20, 2017

alexcjohnson deleted the edit-refactor branch September 20, 2017 19:30

etpinard added this to the v1.31.0 milestone Sep 20, 2017

etpinard mentioned this pull request Oct 3, 2017

Table view - squashed #2052

Merged

rreusser mentioned this pull request Oct 12, 2017

editType and impliedEdits not filtered from reference pages plotly/documentation#832

Closed

etpinard mentioned this pull request Nov 13, 2017

Restyling a date axis with timestamp data changes axis type to numeric #2144

Closed


		var d3 = require('d3');

		module.exports = function checkTicks(axLetter, vals, msg) {

restyle/relayout refactor #1999

restyle/relayout refactor #1999

Conversation

alexcjohnson commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

rreusser Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Sep 20, 2017

etpinard Sep 13, 2017 •

edited

Loading

rreusser Sep 13, 2017 •

edited

Loading

rreusser Sep 13, 2017 •

edited

Loading

rreusser Sep 13, 2017 •

edited

Loading

etpinard Sep 20, 2017 •

edited

Loading