Emit a plotly_relayout' event upon mouseup, wheel #466

monfera · 2016-04-22T12:47:08Z

Fixes #FI-30.

etpinard · 2016-04-22T13:16:50Z

@monfera it might be a good time to lint scene.js so that we don't have to override our linting config. Thanks in advance.

etpinard · 2016-04-22T13:18:54Z

src/plots/gl3d/scene.js

+    };
+
+    scene.glplot.canvas.addEventListener('mouseup', relayoutCallback.bind(null, scene));
+    scene.glplot.canvas.addEventListener('wheel', relayoutCallback.bind(null, scene));


Great. It would be nice to add a mouse wheel test. See the legend_scroll for inspiration.

Thanks for the feedback!

monfera · 2016-05-02T18:20:15Z

test/jasmine/tests/plot_interact_test.js

+                    mouseEvent('mousedown', 110, 150);
+                    mouseEvent('mousemove', 220, 150);
+                    mouseEvent('mouseup', 220, 150);
+


@etpinard @bpostlethwaite In this non-WebGL case, it's been fairly straightforward to simulate the panning activity by the user (to check if plotly_relayout is emitted, and layout on DIV is changed properly). However the analogous thing on scattergl (WebGL based) doesn't yield as easily; my suspicion is that it's more demanding, maybe needing more than the clientX / clientY coordinates. I've blindly tried additionally simulating mouseenter, mouseover etc. in gl_plot_interact_test.js but it didn't help. I'm pushing on happily with that but if either of you have any memory about gl mouse magic, it might obviate the need to step through event handling.

maybe needing more than the clientX / clientY coordinates.

Not sure if you've tried that yet, but here's how I made the plotly_click test work in gl3d.

… lint incompatibilities

…changes on the DIV via unit testing

…oHaveBeenCalledTimes

etpinard · 2016-05-03T18:14:56Z

src/plots/gl2d/scene2d.js

+        lastInputTime: scene.camera.lastInputTime, // helps determine which one is the latest input (if async)
+        xrange: xrange.slice(0),
+        yrange: yrange.slice(0)
+    };


In gl2d, scene is purely internal. I'd vote for something like:

update = { lastInputTime: scene.camera.lastInputTime // great idea here! }; update[xaxis._name] = xrange.slice(); update[yaxis._name] = yrange.slice();

where ?axis.name will be xaxis, xaxis2, yaxis, yaxis2 ...

etpinard · 2016-05-03T18:17:08Z

test/jasmine/tests/plot_interact_test.js


+var MODEBAR_DELAY = 500;


do we really need this in SVG graphs?

@etpinard this is needed as the callback turns out to be deferred - the tests pass if I set it to 0, but fail if I unroll the setTimeouts into the main loop (callback has been called 0 times).

Makes sense. Thanks!

etpinard · 2016-05-03T18:21:11Z

@monfera Looking good. I've made one blocking #466 (comment).

Looking forward to merging this.

… has changed, in sync with the plotly_relayout event

…the data structure of the callback.

etpinard · 2016-05-04T14:47:28Z

src/plots/gl3d/scene.js

@@ -174,6 +174,7 @@ function initializeGLPlot(scene, fullLayout, canvas, gl) {
    var relayoutCallback = function(scene) {
        var update = {};
        update[scene.id] = getLayoutCamera(scene.camera);
+        scene.saveCamera(scene.graphDiv.layout);


I'm ok with this here.

BUT we should add a comment in the resetCameraLastSave mode bar button handler.

That handler looks in the un-updated fullLayout.scene.camera object to reset the camera to the last saved position.

etpinard · 2016-05-04T14:50:44Z

@monfera Great. I'll merge this after:

add comment as mentioned in e467c88#r62052121
lint gl_plot_interact_test.js see https://circleci.com/gh/plotly/plotly.js/1327

etpinard · 2016-05-05T20:13:45Z

src/components/modebar/buttons.js

@@ -350,6 +350,8 @@ function handleCamera3d(gd, ev) {

        if(attr === 'resetDefault') scene.setCameraToDefault();
        else if(attr === 'resetLastSave') {
+            // This handler looks in the un-updated fullLayout.scene.camera object to reset the camera
+            // to the last saved position.


…ing it; test case refactor for multiple mocks, fewer globals and explicit async steps

…ed by CI. The plan is to move more and more tests here, i.e. continuously improve on testing coverage. This test file purposefully avoids beforeEach and afterEach to favor a simpler to follow data flow (no globals).

…t of having two files is that this new file is expected to pass reliably on a local non-headless jasmine run. So we should put more and more (reliable) test cases into this one so that test coverage for at least local automated testing is increasing, even though they're excluded from CI.

monfera · 2016-05-06T19:44:18Z

test/jasmine/tests/gl_plot_interact_basic_test.js

+    // The teardown function needs information of what to tear down so afterEach can not be used without global vars.
+    // In addition to removing the plot from the DOM it also destroy possibly present 2D or 3D scenes
+
+    // TODO we should figure out something to only rely on public API calls


@etpinard if you have a suggestion for using a public API call for the destroy procedure, I'm glad to switch to it. In any case, general feedback is useful as it's a slightly different way of teardown compared to the global var + afterEach method. The purpose is to be as local about state management as possible, i.e. fewer conventions such as the gd global.

Oh. Plotly.purge should do the trick. It was added back in #300

@etpinard awesome, I'll switch to that in both the old and the new GL interaction test files. Although maybe it should be incorporated in the test helper destroy_graph_div to more easily catch all uses, let's see.

etpinard · 2016-05-06T22:09:15Z

Great PR 🍻

Thanks for helping us bring our gl3d tests suite up to speed.

My comment above about Plotly.purge can be resolved as part of an another PR. @monfera looks like this is far from your last plotly.js gl PR.

monfera · 2016-05-06T22:14:49Z

@etpinard cheers :-)

etpinard reviewed Apr 22, 2016
View reviewed changes

etpinard added bug something broken status: in progress labels Apr 22, 2016

monfera force-pushed the 30a-emit-relayout-event-on-layout-change branch 2 times, most recently from 18e4954 to 5180356 Compare April 29, 2016 16:09

monfera reviewed May 2, 2016
View reviewed changes

monfera force-pushed the 30a-emit-relayout-event-on-layout-change branch 3 times, most recently from 36a571f to 5f0a4f0 Compare May 3, 2016 12:57

monfera added 11 commits May 3, 2016 16:31

#30a emit an event upon manipulation

a7177f2

#30a removing preexisting lint exemption directives and fixing former…

1b19b3a

… lint incompatibilities

#30a removing preexisting lint exemption directives and fixing former…

34d15aa

… lint incompatibilities

#30a covering 2D WebGL plot with the plotly_relayout event on pan / zoom

3d8fe81

#30a test case for mouse drag and wheel on gl3d plots

a10caec

#30a proper grouping by interaction type

604d739

#30a test case for mouse drag on gl2d plots

ee8ff65

#30a reifying plotly_relayout callback count, as well as layout data …

da01879

…changes on the DIV via unit testing

#30a adding graphDiv to Scene2d similarly to Scene / thanks @etpinard

48682a4

#30a bumping jasmine to 2.4 to enjoy its new facilities such as spy.t…

63ec931

…oHaveBeenCalledTimes

#30a panning test on WebGL canvas; adding the DIV update for scene2d

19c478d

monfera force-pushed the 30a-emit-relayout-event-on-layout-change branch from 5f0a4f0 to 19c478d Compare May 3, 2016 14:33

etpinard added status: reviewable and removed status: in progress labels May 3, 2016

etpinard reviewed May 3, 2016
View reviewed changes

monfera added 4 commits May 3, 2016 20:39

#30a saving the camera position on the DIV layout when the projection…

e467c88

… has changed, in sync with the plotly_relayout event

#30a PR feedback: not expose scene id and use axis names. Plus: test …

b212086

…the data structure of the callback.

#30a PR feedback: remove delay

e8aa9cb

#30a adding tests for the 3d callback and DIV layout camera data update

e02a3f6

etpinard reviewed May 4, 2016
View reviewed changes

#30a PR feedback: commenting and removing disused binding

4975417

etpinard reviewed May 5, 2016
View reviewed changes

#30a PR feedback: adding test case with partially set camera and solv…

7c23e2e

…ing it; test case refactor for multiple mocks, fewer globals and explicit async steps

monfera force-pushed the 30a-emit-relayout-event-on-layout-change branch from 743b6b7 to 7c23e2e Compare May 6, 2016 16:42

monfera added 2 commits May 6, 2016 20:34

monfera reviewed May 6, 2016
View reviewed changes

etpinard merged commit dca69f9 into plotly:master May 6, 2016

etpinard mentioned this pull request May 16, 2016

Click-drag zooming does not trigger a plotly_relayout event for 'scattergl' #134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit a plotly_relayout' event upon mouseup, wheel #466

Emit a plotly_relayout' event upon mouseup, wheel #466

monfera commented Apr 22, 2016 •

edited

Loading

etpinard commented Apr 22, 2016

etpinard Apr 22, 2016

monfera Apr 22, 2016

monfera May 2, 2016

etpinard May 2, 2016 •

edited

Loading

etpinard May 3, 2016

etpinard May 3, 2016

monfera May 3, 2016

etpinard May 4, 2016

etpinard commented May 3, 2016

etpinard May 4, 2016 •

edited

Loading

etpinard commented May 4, 2016 •

edited

Loading

etpinard May 5, 2016

monfera May 6, 2016

etpinard May 6, 2016

monfera May 6, 2016

etpinard commented May 6, 2016

monfera commented May 6, 2016

Emit a plotly_relayout' event upon mouseup, wheel #466

Emit a plotly_relayout' event upon mouseup, wheel #466

Conversation

monfera commented Apr 22, 2016 • edited Loading

etpinard commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 2, 2016 • 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 May 3, 2016

etpinard May 4, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard commented May 4, 2016 • 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

etpinard commented May 6, 2016

monfera commented May 6, 2016

monfera commented Apr 22, 2016 •

edited

Loading

etpinard May 2, 2016 •

edited

Loading

etpinard May 4, 2016 •

edited

Loading

etpinard commented May 4, 2016 •

edited

Loading