Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gl2d data-referenced annotations #1301

Merged
merged 14 commits into from
Jan 19, 2017
Merged

gl2d data-referenced annotations #1301

merged 14 commits into from
Jan 19, 2017

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jan 13, 2017

set to ✔️ one item on #130

gifrecord_2017-01-13_171012

TODO:

  • add some jasmine test (and maybe clean up our gl2d test suites along the way)

@etpinard etpinard added this to the v1.22.0 milestone Jan 16, 2017

var relayoutCallback = function(scene) {
this.graphDiv._fullLayout = fullLayout;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems a bit scary - why is this necessary? I'm just worried that if gd._fullLayout and scene.fullLayout have diverged, there could somehow be changes in gd._fullLayout that this will throw away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the problem is in the doModebar relayout shortcut.

I'll double-check why the _fullLayout gets lost. Thanks for the headsup!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be an option to not use this.fullLayout at all, but only to use this.graphDiv._fullLayout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an idea.

So, the _fullLayout gets lost in Plots.supplyDefaults. We could add a block handling refs on scene objects similar to this one for cartesian axes. But I think we could do better.

The reason the lost of reference only affects dragmode relayouts is because all other relayout calls pass through scene2d.plot which update the fullLayout ref here on every pass.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, well I'm not entirely sure what would happen, but the test would be something like:

  • do a relayout that causes a new gd._fullLayout to be constructed by supplyDefaults but DOESN'T result in a new scene2d.plot (like what? change legend background color? change something cosmetic on an axis of a different subplot?)
  • trigger this code by changing dragmode
  • do something else that causes a full plot redraw (maybe just Plotly.redraw(gd))
  • does the initial relayout get reverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the cleanest way (I think) of updating gd._fullLayout._plots.xy._scene2d.fullLayout

b50b7e9

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that seems better, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full solution in #1319

- this is probably a leftover from an old merge conflict
- as the full Plots.supplyDefaults is called before the
  doModeBar subroutine.
- scene2d.updateRefs is now called on scene2d.plot
  and during Plots.supplyDefaults, ensuring that
  scene2d.fullLayout is up-to-date with gd._fullLayout.
- call after each mouse-change / wheel-change,
  as opposed to each time the scene2d ticks changed
  (which didn't cover all the cases)
- make sure gl2d pan test `mouseup` to trigger relayoutCallback
Improvement in how gl2d refs to fullLayout are updated
@etpinard etpinard merged commit 1d5c76b into master Jan 19, 2017
@etpinard etpinard deleted the annotations-gl2d branch January 19, 2017 15:13
@etpinard etpinard mentioned this pull request Jun 8, 2017
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants