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

Transformed array on zoom fix #1717

Merged
merged 3 commits into from
May 23, 2017
Merged

Conversation

etpinard
Copy link
Contributor

fixes #1713

As promised, it's not pretty. cc @n-riesco @alexcjohnson

etpinard added 2 commits May 23, 2017 16:10
- flag fullData traces that passed through calc transforms
  with `_hasCalcTransform: true`
- update calcdata[i][0].trace ref BUT map back
  transformed arrays so that they match the calcdata[i][j] items
  on non-recalc updates (e.g. zoom events)
@etpinard etpinard added status: reviewable bug something broken labels May 23, 2017
@@ -505,12 +506,38 @@ plots.supplyDefaults = function(gd) {
// update object references in calcdata
if((gd.calcdata || []).length === newFullData.length) {
Copy link
Contributor Author

@etpinard etpinard May 23, 2017

Choose a reason for hiding this comment

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

This block updates the gd.calcdata[i][0].trace reference so that the plot step can use the correct fullData attribute values even in cases where Plots.doCalcdata isn't called (e.g. on zoom).


for(i = 0; i < arrayAttrs.length; i++) {
ast = arrayAttrs[i];
Lib.nestedProperty(cd0.trace, ast).set(transformedArrayHash[ast]);
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 we make sure that the array attributes in gd.calcdata[i][0].trace match the transformed array post calc transform. Other non-data-array attributes are left untouched.

var oldTrace = cd0.trace;
var arrayAttrs = PlotSchema.findArrayAttributes(oldTrace);
var transformedArrayHash = {};
var i, ast;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🐄 just so I don't get confused, ast makes me think "abstract syntax tree", elsewhere we call this astr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, astr is what I had in mind. Something wrong happened between brain and keyboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 0a4ce1f

@alexcjohnson
Copy link
Collaborator

Nice use of PlotSchema to keep this clean!

Yeah, annoying that we have to do this (for the record from a separate chat: the underlying issue is that a calcTransform can modify fullData, which is really upstream of the calc step that it's run in. We haven't come up with a cleaner way to separate this but will keep ruminating on it)

💃 after #1717 (comment)

@etpinard etpinard merged commit 6cc5ffd into master May 23, 2017
@etpinard etpinard deleted the transformed-array-on-zoom-fix branch May 23, 2017 22:00
@rreusser
Copy link
Contributor

rreusser commented May 26, 2017

Is it clear what needs to happen with this? I'm still having just a bit of trouble understanding exactly why this is needed and then the downstream problem with animation.

@etpinard
Copy link
Contributor Author

Is it clear what needs to happen with this?

First look at the added assertions. If that's not clear, slack me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong fullData passed plotly_click listeners after zoom
3 participants