Configurable select direction #2506

GlennMatthys · 2018-03-29T09:19:21Z

Proper implementation of #2495. This PR includes:

Adding the "selectdirection" attribute to layout
Implementation in Cartesian select
Image mocks for the various "selectdirection" options

Also adds a "d" selectiondirection to only allow diagonal selects.

Working on writing a proper test for this functionality, I'm still figuring out how to do the necessary calculations.

…elect

alexcjohnson · 2018-03-29T15:00:38Z

src/components/fx/layout_attributes.js

+        description: [
+            'When "dragmode" is set to "select", this limits the selection of the drag to',
+            'horizontal, vertical or diagonal. "h" only allows horizontal selection,',
+            '"v" only vertical, "d" only diagonal and "any" sets no limit.'


Nice, I like 'd', I guess what it allows that 'any' doesn't is selecting regions that are very narrow in height or width, which would otherwise devolve to pure horizontal or vertical selections. Cool.

alexcjohnson · 2018-03-29T15:01:16Z

src/components/fx/layout_defaults.js

@@ -17,6 +17,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
    }

    coerce('dragmode');
+    coerce('selectdirection', 'any');


since the attribute has dflt: 'any' already, you do not need to include it here.

alexcjohnson · 2018-03-29T15:20:44Z

test/image/mocks/limit_select_any.json

+    "yaxis2": { "domain": [0.51, 1] },
+    "showlegend": false,
+    "dragmode": "select",
+    "selectdirection": "any"


I don't think we need to make image test mocks for selectdirection, since it has no effect on the static images. For the jasmine tests, if you don't like the mock (14) that we use in the neighboring tests, you can use any other predefined mock, or just make a new plot explicitly in the test, something like:

Plotly.newPlot(gd, [ {x: [1,1,1,1,1,2,2,2,3,3], type: 'histogram'}, {x: [1,2,3], y:[-1,-2,-3], type: 'bar'} ], { showlegend: false, dragmode: 'select' }) .then(function() { expect(gd.fullLayout.selectdirection).toBe('any'); // do a drag and check that the selection is correct for the default `selectdirection` return Plotly.relayout(gd, {selectdirection: 'h'}); }) .then(function() { // do probably the SAME drag and check that the selection is correct for the `selectdirection='h'` return Plotly.relayout(gd, {selectdirection: 'v'}); }) .then(function() { // ... }) .catch(fail) // hmm, we should change the name of this import to failTest, since fail is already a jasmine global, for sync failure, we're overriding it here with our async version (we've already done this in other places) .then(done);

alexcjohnson · 2018-03-29T16:03:00Z

I'm still figuring out how to do the necessary calculations.

While it's nice to be able to write a test where you've pre-calculated what the outcome should be, it's not always necessary: as long as you can demonstrate a difference between the different modes, you can just write tests, watch them fail when you run them, and change them so they pass. But if you do want to do the calculation yourself, I've had good luck setting explicit width, height, margin, and axis range so you can make an easy calculation of data value -> pixel position.

GlennMatthys · 2018-04-05T14:43:21Z

@alexcjohnson can you have a look at this commit to see if I'm on the right track?

alexcjohnson · 2018-04-05T15:28:52Z

test/jasmine/tests/select_test.js

+
+                return Plotly.relayout(gd, {selectdirection: 'h'});
+            })
+            .then(function() {


Yep, this looks good! Make sure you keep chaining the async events - your selectedPromise.then... is not being waited for by the following code so it's not entirely clear what sequence things will happen in. Try something like:

Plotly.newPlot(...) .then(function() { resetEvents(gd); drag(selectPath); return selectedPromise; }) .then(function() { expect(selectedData.points.length).toBe(1); // etc return Plotly.relayout(...); })

GlennMatthys · 2018-04-06T11:08:40Z

Here's a tool I used to help create the tests: https://jsfiddle.net/c0ojh6vq/8/ (just shows you the mouse hover position on the drag layer).

GlennMatthys · 2018-04-06T11:17:02Z

The CI test appears to be failing on an unrelated part before the selectdirection tests ("Test polar interactions: should response to drag interactions on plot area FAILED"), are these tests known to fail from time to time?

GlennMatthys · 2018-04-13T06:18:02Z

@alexcjohnson Can you have a look at this? Can't reproduce the test failures locally.

alexcjohnson · 2018-04-13T13:38:27Z

Oh sorry about letting this one slip through last week - thanks for pinging me again. Yes, the test failure is unrelated, we should probably mark that test as @flaky as I have seen that fail spuriously before.

Anyway I could rerun it for you but it still looks like the promise chain in your test is branched - see #2506 (comment) - can you rearrange so every .then is chained off the original Plotly.newPlot? The 'should skip over BADNUM items' test is a good reference for this structure.

alexcjohnson · 2018-04-17T16:22:21Z

@GlennMatthys I'll finish this up, so it can go out with v1.36 this afternoon.

- flatten the promise chain - simplify the test plot so I can better understand what *should* happen

alexcjohnson · 2018-04-17T17:18:14Z

@etpinard care to take a quick look?

etpinard · 2018-04-17T17:29:59Z

one more for the flaky gang?

GlennMatthys · 2018-04-17T17:33:15Z

I wanted to finish this up but I'm struggling to get the tests to succeed. How do you calculate the select path? To which element are these coordinates relative to? Is there some offset that needs to be taken into account?

etpinard · 2018-04-17T17:35:10Z

src/components/fx/layout_defaults.js

@@ -17,6 +17,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
    }

    coerce('dragmode');
+    coerce('selectdirection');


This only has an effect for dragmode: 'select', correct? So should this be:

var dragMode = coerce('dragmode'); if(dragMode === 'select') { coerce('selectdirection') }

yep -> conditional coerce 9dd166c

alexcjohnson · 2018-04-17T17:37:54Z

@GlennMatthys I think it's all set now, check out 6d4a4f8

But for reference, if you're contributing more in the future, it's relative to the corner of the plot, including the margins. That's why in that commit you can see I set the margins, size, and axis ranges explicitly so I know x is 100px->0, 300px->4, and y is the opposite 100px->4, 300px-> 0

etpinard · 2018-04-17T17:38:45Z

src/plots/cartesian/select.js

@@ -192,6 +192,8 @@ function prepSelect(e, startX, startY, dragOptions, mode) {
        }
    }

+    var direction = fullLayout.selectdirection;


direction doesn't get used in the outer scope. Could be move that var direction into the moveFn scope below?

good call -> pushed to inner scope 9dd166c

alexcjohnson · 2018-04-17T17:47:55Z

Not sure if CI is extra-flaky today or if splom and other new features are causing it to bog down... but anyway I moved those new shape tests to @flaky as well... 🤞

etpinard · 2018-04-17T18:08:35Z

Nicely done 💃

GlennMatthys · 2018-04-17T18:13:24Z

Can the coordinate system for selections be documented somewhere, perhaps in the contributing guide? This should help newcomers orient themselves, plus the tips like fixing the margins and axis ranges to produce a much more reliable canvas to do select tests on. Some visual representation would be nice too:

alexcjohnson · 2018-04-17T18:33:08Z

Can the coordinate system for selections be documented somewhere, perhaps in the contributing guide?

That's a good idea - @etpinard where would be the right place for this?

I like your diagram. Note that [175, 175] and [225, 225] go one (data) unit down and right of where you have them, ie 175 is actually where you've drawn 225.

GlennMatthys · 2018-04-17T18:48:55Z

Made a new PR so this can be discussed more appropriately, also fixed the diagram.

GlennMatthys added 3 commits March 29, 2018 11:03

Add "selectdirection" layout attribute

6d820f7

Implement support for selectdirection layout attribute in cartesian s…

9595cbe

…elect

Image mocks for selectdirection

5cfc474

GlennMatthys mentioned this pull request Mar 29, 2018

Feature request: allow limiting dragmode 'select' to horizontal or vertical only #2495

Closed

alexcjohnson reviewed Mar 29, 2018

View reviewed changes

alexcjohnson mentioned this pull request Mar 29, 2018

Allow clamping select mode to horizontal or vertical #2498

Closed

GlennMatthys added 3 commits March 30, 2018 10:28

Remove redundant default

b782c88

Remove unnecessary test cases

2112759

Add initial test case

c49d829

alexcjohnson reviewed Apr 5, 2018

View reviewed changes

Add tests for h, v and d select directions & cleanup

69c3157

etpinard added feature status: in progress labels Apr 17, 2018

alexcjohnson added 2 commits April 17, 2018 12:24

Merge branch 'master' into configurable-select-direction

d65ffc8

edit selectdirection test

6d4a4f8

- flatten the promise chain - simplify the test plot so I can better understand what *should* happen

alexcjohnson added status: reviewable and removed status: in progress labels Apr 17, 2018

etpinard added this to the v1.36.0 milestone Apr 17, 2018

etpinard reviewed Apr 17, 2018

View reviewed changes

selectdirection PR review edits

9dd166c

alexcjohnson merged commit 6af4695 into plotly:master Apr 17, 2018

GlennMatthys deleted the configurable-select-direction branch April 17, 2018 18:19

GlennMatthys mentioned this pull request Apr 17, 2018

Add tips for writing selection tests #2567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Configurable select direction #2506

Configurable select direction #2506

GlennMatthys commented Mar 29, 2018 •

edited

Loading

alexcjohnson Mar 29, 2018

alexcjohnson Mar 29, 2018

alexcjohnson Mar 29, 2018

alexcjohnson commented Mar 29, 2018

GlennMatthys commented Apr 5, 2018

alexcjohnson Apr 5, 2018

GlennMatthys commented Apr 6, 2018

GlennMatthys commented Apr 6, 2018

GlennMatthys commented Apr 13, 2018

alexcjohnson commented Apr 13, 2018

alexcjohnson commented Apr 17, 2018

alexcjohnson commented Apr 17, 2018

etpinard commented Apr 17, 2018

GlennMatthys commented Apr 17, 2018

etpinard Apr 17, 2018 •

edited

Loading

alexcjohnson Apr 17, 2018

alexcjohnson commented Apr 17, 2018

etpinard Apr 17, 2018

alexcjohnson Apr 17, 2018

alexcjohnson commented Apr 17, 2018

etpinard commented Apr 17, 2018

GlennMatthys commented Apr 17, 2018 •

edited

Loading

alexcjohnson commented Apr 17, 2018

GlennMatthys commented Apr 17, 2018

Configurable select direction #2506

Configurable select direction #2506

Conversation

GlennMatthys commented Mar 29, 2018 • edited Loading

alexcjohnson Mar 29, 2018

Choose a reason for hiding this comment

alexcjohnson Mar 29, 2018

Choose a reason for hiding this comment

alexcjohnson Mar 29, 2018

Choose a reason for hiding this comment

alexcjohnson commented Mar 29, 2018

GlennMatthys commented Apr 5, 2018

alexcjohnson Apr 5, 2018

Choose a reason for hiding this comment

GlennMatthys commented Apr 6, 2018

GlennMatthys commented Apr 6, 2018

GlennMatthys commented Apr 13, 2018

alexcjohnson commented Apr 13, 2018

alexcjohnson commented Apr 17, 2018

alexcjohnson commented Apr 17, 2018

etpinard commented Apr 17, 2018

GlennMatthys commented Apr 17, 2018

etpinard Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

alexcjohnson Apr 17, 2018

Choose a reason for hiding this comment

alexcjohnson commented Apr 17, 2018

etpinard Apr 17, 2018

Choose a reason for hiding this comment

alexcjohnson Apr 17, 2018

Choose a reason for hiding this comment

alexcjohnson commented Apr 17, 2018

etpinard commented Apr 17, 2018

GlennMatthys commented Apr 17, 2018 • edited Loading

alexcjohnson commented Apr 17, 2018

GlennMatthys commented Apr 17, 2018

GlennMatthys commented Mar 29, 2018 •

edited

Loading

etpinard Apr 17, 2018 •

edited

Loading

GlennMatthys commented Apr 17, 2018 •

edited

Loading