Shape clean up #378

n-riesco · 2016-04-01T00:32:28Z

Here's an outline of the plan I'm following to fix issues #359 and #148:

Split method shapes.draw into smaller functions
Move the creation of shapelayer into function plotLayers in makeCartesianPlotFramwork (this fixes Zoom overlay drawn beneath shapes. #359)
Split shapelayer into shapeLowerLayer and shapeUpperLayer (this fixes Drawing traces on top of shapes #148)

I've checked refactoring shapes.drawhasn't introduced new bugs by running both the image and the jasmine tests.

etpinard · 2016-04-01T12:50:54Z

@n-riesco This looks like a solid plan! I like the way you cleaned up the shapes drawing step.

Before moving on to the other 2 step, it would be nice add a few interaction tests for shapes.

To do so,

add a shapes_test.js file in test/jasmine/tests/
add a suite similar to this one using the shapes.json mock.
assert the shape nodes in the right place after Plotly.relayout

n-riesco · 2016-04-04T16:00:16Z

Although I managed to move all the shapes into each subplot (at the correct positions), this has an undesired side effect: all the shapes that lie outside of a subplot aren't visible. I think this means the approach I was following isn't feasible.

I'm considering an alternative approach: move shapelayer, zoombox and zoombox-corners outside plotinfo.plot.

@etpinard , what do you think?

etpinard · 2016-04-04T19:04:54Z

this has an undesired side effect: all the shapes that lie outside of a subplot aren't visible.

Could you elaborate? Do you mean all data-referenced shapes here? See this codepen id the distinction between data-refs and paper-ref isn't clear to you.

n-riesco · 2016-04-04T21:02:21Z

I'm taking into account the effect of xref. You can see the implementation in this branch. I modified shapePath to account for xref.

The way I understand the problem is that by moving the shapelayer into this svg, I'm clipping the shapelayer to the viewBox of that svg.

etpinard · 2016-04-04T21:47:19Z

The way I understand the problem is that by moving the shapelayer into this svg, I'm clipping the shapelayer to the viewBox of that svg.

Correct. This is the desired behavior for data-referenced shapes but not for paper-referenced annotations. Currently, data-referenced shapes are clipped using <svg> clip paths here. We should be able to use the same pattern in the case where the <g class="shapelayer"> is below the plotinfo.plot layer.

n-riesco · 2016-04-04T22:12:03Z

Just to be clear, we're now thinking of moving the shapelayer between these two lines?

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

After looking through the requirements for issues #359 and #148 more closely, I think it is worth waiting a couple weeks before putting more work into them.

Starting soon @mdtusz will remove the nested subplot <svg> in favor of clipPath. Those nested <svg> have been plaguing us for quite some time now. Among others, Abobe illustrator and RStudio don't like them. Moreover, without the nested <svg>s, a lot of the shape layer ordering logic should be simplified.

This PR in its current states looks good. Were you planning on adding any other patches to it? If not, I'm thinking about merging it for the next release.

n-riesco · 2016-04-05T14:53:53Z

I'm testing a PR that addresses #359 , but I can open another PR for it.

n-riesco · 2016-04-06T10:41:06Z

For our future selves, here's a list of the approaches I tried:

move shape layer into each subplot layer while keeping gd._fullLayout.shapes
move shape layer into each subplot svg (shape positions needed correcting, shapes only visible in the area covered by subplots)
move shape layer into each subplot layer and move gd._fullLayout.shapes into each gd._fullLayout._plots[i].shapes
move zoom elements into a zoombox layer
move select and zoom elements into the shape layer

etpinard · 2016-04-06T16:22:05Z

src/components/shapes/index.js

-                    Plotly.Lib.extendFlat({}, value) :
-                    {text: 'New text'};
+    updateShape(gd, index, opt, value);
+    return;


no need for that return. Save the bytes.

n-riesco · 2016-04-06T16:26:45Z

One more thing I almost forgot. I think here we're missing a layout.shapes.push({});. This may explain this failure.

n-riesco · 2016-04-06T16:38:19Z

@etpinard I've just removed the returns.

etpinard · 2016-04-06T17:00:21Z

src/components/shapes/index.js

-            delete layout.shapes;
-            fullLayout.shapes = [];
-            shapes.drawAll(gd);
+            deleteAllShapes(gd);


it would be nice add a few test cases for adding and removing shapes.

Here's the official way to add/remove shapes (and annotations for that matter): http://codepen.io/etpinard/pen/xVPoYG

* Removed call to `.remove()` in `shapes.drawAll()`, since `.remove()` is also call in `shapes.draw()`. * No new test failed after this change.

* Test whether Plotly.relayout() can add and remove shapes.

etpinard · 2016-04-07T14:42:29Z

test/jasmine/tests/shapes_test.js

+
+            Plotly.relayout(gd, 'shapes[' + index + ']', shape).then(function() {
+                expect(getLastShape(gd)).toEqual(shape);
+                expect(countShapes(gd)).toEqual(index + 1);


@n-riesco nice test. Thanks.

Could you also add an expect(countShapeLayers()) to make sure that the shapes are drawn correctly on relayout?

* Count number of shape layers after relayout.

etpinard · 2016-04-07T15:29:36Z

test/jasmine/tests/shapes_test.js

                expect(getLastShape(gd)).toEqual(shape);
                expect(countShapes(gd)).toEqual(index + 1);
            }).then(function() {
                Plotly.relayout(gd, 'shapes[' + index + ']', 'remove');
            }).then(function() {
+                expect(countShapeLayers()).toEqual(1);


Oops. My bad. I meant expect(countShapeLayerPaths()).

But testing expect(countShapeLayers()) isn't bad either. test 🐅 🐅

Oh... that makes sense. I should've guessed you meant that.

* Count the number of paths in the shape layer after relayout.

etpinard · 2016-04-07T15:54:55Z

test/jasmine/tests/shapes_test.js

                expect(getLastShape(gd)).toEqual(shape);
                expect(countShapes(gd)).toEqual(index + 1);
            }).then(function() {
                Plotly.relayout(gd, 'shapes[' + index + ']', 'remove');
            }).then(function() {
                expect(countShapeLayers()).toEqual(1);
+                expect(countShapePaths()).toEqual(pathCount);


etpinard · 2016-04-07T16:23:20Z

Great job @n-riesco 🍻

etpinard added the status: in progress label Apr 1, 2016

n-riesco mentioned this pull request Apr 2, 2016

Add shapes tests #389

Merged

3 tasks

n-riesco mentioned this pull request Apr 6, 2016

Fix zoom overlay positioning (Fixes #359) #395

Closed

etpinard reviewed Apr 6, 2016
View reviewed changes

etpinard changed the title ~~[WIP] Fix issues #359 and #148~~ Shape clean up Apr 6, 2016

etpinard added type: maintenance and removed status: in progress labels Apr 6, 2016

etpinard reviewed Apr 6, 2016
View reviewed changes

n-riesco added 2 commits April 6, 2016 23:40

Split method shapes.draw into smaller functions

c53303b

Remove unnecessary step from shapes.drawAll()

f67ff4d

* Removed call to `.remove()` in `shapes.drawAll()`, since `.remove()` is also call in `shapes.draw()`. * No new test failed after this change.

n-riesco force-pushed the issue-148-and-359 branch from 46ad23d to e9f91de Compare April 6, 2016 22:48

Add more shapes tests

22d7248

* Test whether Plotly.relayout() can add and remove shapes.

n-riesco force-pushed the issue-148-and-359 branch from e9f91de to 22d7248 Compare April 6, 2016 23:16

etpinard reviewed Apr 7, 2016
View reviewed changes

Update shapes test

f8dbdd1

* Count number of shape layers after relayout.

etpinard reviewed Apr 7, 2016
View reviewed changes

Update shapes test

92e46e3

* Count the number of paths in the shape layer after relayout.

etpinard reviewed Apr 7, 2016
View reviewed changes

etpinard merged commit e8fc236 into plotly:master Apr 7, 2016

n-riesco deleted the issue-148-and-359 branch April 15, 2016 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shape clean up #378

Shape clean up #378

n-riesco commented Apr 1, 2016

etpinard commented Apr 1, 2016

n-riesco commented Apr 4, 2016

etpinard commented Apr 4, 2016

n-riesco commented Apr 4, 2016

etpinard commented Apr 4, 2016

n-riesco commented Apr 4, 2016

etpinard commented Apr 5, 2016

n-riesco commented Apr 5, 2016

n-riesco commented Apr 6, 2016

etpinard Apr 6, 2016

n-riesco commented Apr 6, 2016

n-riesco commented Apr 6, 2016

etpinard Apr 6, 2016

etpinard Apr 7, 2016

etpinard Apr 7, 2016

n-riesco Apr 7, 2016

etpinard Apr 7, 2016

etpinard commented Apr 7, 2016

Shape clean up #378

Shape clean up #378

Conversation

n-riesco commented Apr 1, 2016

etpinard commented Apr 1, 2016

n-riesco commented Apr 4, 2016

etpinard commented Apr 4, 2016

n-riesco commented Apr 4, 2016

etpinard commented Apr 4, 2016

n-riesco commented Apr 4, 2016

etpinard commented Apr 5, 2016

n-riesco commented Apr 5, 2016

n-riesco commented Apr 6, 2016

etpinard Apr 6, 2016

Choose a reason for hiding this comment

n-riesco commented Apr 6, 2016

n-riesco commented Apr 6, 2016

etpinard Apr 6, 2016

Choose a reason for hiding this comment

etpinard Apr 7, 2016

Choose a reason for hiding this comment

etpinard Apr 7, 2016

Choose a reason for hiding this comment

n-riesco Apr 7, 2016

Choose a reason for hiding this comment

etpinard Apr 7, 2016

Choose a reason for hiding this comment

etpinard commented Apr 7, 2016