Axes ref cleanup #1431

etpinard · 2017-03-01T21:29:21Z

No more ax_gd !!!

- to show console.log in terminal in karma ^1.5.0 - See karma-runner/karma@89a7a1c#commitcomment-21009216

- make setConvert take fullLayout, use fullLayout._size in setScale instead of gd._fullLayout._size - add _input ref to fullLayout axis, use that in doAutorange to copy back computed range to input layout

- instead of pre-initialization - the pattern is more similar to other subplots (e.g. ternary) and requires one less module

etpinard · 2017-03-01T21:30:38Z

src/plots/cartesian/layout_defaults.js

        return Lib.coerce(axLayoutIn, axLayoutOut, layoutAttributes, attr, dflt);
    }

-    axesList.forEach(function(axName) {


as promised in #1261 (comment)

There are still two maps and a filter 🐎

done in 5eb6202

etpinard · 2017-03-01T21:31:47Z

src/plots/plots.js

    var axList = Plotly.Axes.list(gd);
    for(i = 0; i < axList.length; i++) {
        var ax = axList[i];
-        ax._gd = gd;


@alexcjohnson this commit is very important.

First, no need to keep ax_gd in-sync anymore!

etpinard · 2017-03-01T21:32:14Z

src/plots/cartesian/set_convert.js

 */
-module.exports = function setConvert(ax) {
+module.exports = function setConvert(ax, fullLayout) {
+    fullLayout = fullLayout || {};


Second, make setConvert take in fullLayout

etpinard · 2017-03-01T21:32:41Z

src/plots/cartesian/set_convert.js

    // set scaling to pixels
    ax.setScale = function(usePrivateRange) {
-        var gs = ax._gd._fullLayout._size,
+        var gs = fullLayout._size,


... so that setScale can look into _size.

etpinard · 2017-03-01T21:34:08Z

src/plots/cartesian/set_convert.js

                'Something went wrong with axis scaling',
                'long');
-            ax._gd._replotting = false;
            throw new Error('axis scaling');


🔪 ax._gd._replotting = false; on axis scaling errors. Isn't that error throw enough?

If we think the error is fatal to the plot then yes, it's enough. But if there would be a use case for catching the error and continuing, then the plot will be in an odd state without resetting gd._replotting. I suppose at the very least we can move it to fullLayout but we should really figure out a better solution than the fragile _replotting flag to prevent infinite recursion.

Sounds good. I'll try to cook up a test case for this edge case. Any help would be appreciated ;)

done in 9612549

etpinard · 2017-03-01T21:35:10Z

src/plots/cartesian/set_convert.js


+    // copy ref to fullLayout.separators so that
+    // methods in Axes can use it w/o having to pass fullLayout
+    ax.separators = fullLayout.separators;


Important: link fullLayout.separators to full axis object, so that Axes method can use it without having to know anything about fullLayout.

shall we call it _separators to distinguish it from regular settings managed by supplyDefaults? That actually adds overhead (as relinkPrivateKeys will look at it) but it does seem to be the pattern we've been using.

Good call here. I was thinking maybe down the road we would add per-axis separators, making this line obsolete (by supply defaults logic). But no, you're right, non-underscore keys in containers should match input attributes.

done in 6e25640

etpinard · 2017-03-01T21:37:34Z

src/plots/cartesian/layout_defaults.js

-        if(!layoutIn[axName] && axLayoutIn.type !== '-') {
-            layoutIn[axName] = {type: axLayoutIn.type};
-        }
+        axLayoutOut._input = axLayoutIn;


This is essentially the new ax_gd. This pattern is similar to other components that require a ref to the user container to mutate it on interactions (e.g. updatemenus and sliders). Moreover, IMHO this pattern is a lot better than having to keep the full gd in-sync.

etpinard · 2017-03-01T21:38:09Z

src/plots/cartesian/axes.js

-        }
+        var axIn = ax._input;
+        axIn.range = ax.range.slice();
+        axIn.autorange = ax.autorange;


Now, doAutorange writes in ax._input instead of having dig in _gd.layout ... 🎉

etpinard · 2017-03-01T21:40:05Z

src/plots/cartesian/axes.js


 function tickTextObj(ax, x, text) {
-    var tf = ax.tickfont || ax._gd._fullLayout.font;
+    var tf = ax.tickfont || {};


Oh. I need to double-check this.

Removing the || {} should do the trick - as axis tickfont already default to layout font, but removing || {} makes the fonts.json mock fail. Not sure why.

After investigation, I'll keep the || {} fallback, if ok.

Whenever ax.showticklabels: false, ax.tickfont isn't coerced and isn't defined here.

We could make sure that tickTextObj isn't called when ax.showticklabels: false, but this requires adding a few early returns in axes.js, 3d and gl2d convert routines which I believe isn't worth it.

alexcjohnson · 2017-03-02T00:50:14Z

src/plots/gl3d/scene.js

+        var ax = this.fullSceneLayout[axisProperties[i]];
+        Axes.setConvert(ax, this.fullLayout);
+    }
+};


Nice simplification! Why don't we need the containerOut.setScale = Lib.noop; line anymore?

Not anymore. 🔪

To be more precise, 3d doesn't use any of the _m and friends fields that setScale computes at the moment. So setting setScale to a noop might be a minor 🐎 boost. But, for 3d annotations, I'm planning on using / mocking those scale fields. To be continued.

alexcjohnson · 2017-03-02T00:52:32Z

test/jasmine/karma.conf.js

+    // See https://github.com/karma-runner/karma/commit/89a7a1c#commitcomment-21009216
+    func.defaultConfig.browserConsoleLogOptions = {
+        level: 'log'
+    };


Oh awesome - that'll be super helpful.

- to avoid confusion as separators isn't an axis attribute

alexcjohnson · 2017-03-02T17:56:54Z

Thanks for the changes. I guess you're still sorting something out re: fonts but I'm ready to 💃

etpinard added 6 commits March 1, 2017 15:06

add extra karma config

1700908

- to show console.log in terminal in karma ^1.5.0 - See karma-runner/karma@89a7a1c#commitcomment-21009216

replace for-each by for-loop in cartesian layout defaults

11073f2

rm _gd ref in fullLayout axis containers

2289657

- make setConvert take fullLayout, use fullLayout._size in setScale instead of gd._fullLayout._size - add _input ref to fullLayout axis, use that in doAutorange to copy back computed range to input layout

call setConvert on 3d axis during Scene.plot

6a271ed

- instead of pre-initialization - the pattern is more similar to other subplots (e.g. ternary) and requires one less module

require axis_ids instead of Axes in cartesian index

704596a

improve comment for setScale in lsInner

045da99

etpinard added status: reviewable labels Mar 1, 2017

etpinard commented Mar 1, 2017

View reviewed changes

alexcjohnson reviewed Mar 2, 2017

View reviewed changes

etpinard added 4 commits March 2, 2017 12:08

Merge branch 'master' into axes-ref-cleanup

50d1288

replace .map with for-loops

5eb6202

link fullLayout.separators to ax._separators

6e25640

- to avoid confusion as separators isn't an axis attribute

replace gd._replotting -> fullLayout._replotting

9612549

etpinard merged commit 8a310b3 into master Mar 2, 2017

etpinard deleted the axes-ref-cleanup branch March 2, 2017 20:37

etpinard mentioned this pull request Mar 9, 2017

[WIP] Carpet plots #1239

Closed

44 tasks

etpinard mentioned this pull request Mar 17, 2017

Relayout edge case fixes #1494

Merged

Uh oh!

Axes ref cleanup #1431

Axes ref cleanup #1431

Uh oh!

Conversation

etpinard commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Mar 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

etpinard Mar 1, 2017 •

edited

Loading

etpinard Mar 1, 2017 •

edited

Loading

etpinard Mar 2, 2017 •

edited

Loading