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

Preserve ui state across Plotly.react redraws #3236

Merged
merged 35 commits into from
Nov 30, 2018
Merged

Preserve ui state across Plotly.react redraws #3236

merged 35 commits into from
Nov 30, 2018

Conversation

alexcjohnson
Copy link
Collaborator

For plotly/dash-core-components#198

  • Track gui-driven changes via internal-use variants of restyle/relayout/update -> _guiRestyle etc that wrap the original methods to record the original state.
  • Add a class of uirevision attributes (that all inherit directly or indirectly from layout.uirevision so in the simplest case you can just use that and forget the others) that can be used to maintain the state user interaction has left the plot in. If uirevision exists and did not change, any attribute the user has modified via the built-in GUI will keep the user-specified value unless it has a new explicit value in the new figure.

cc @chriddyp @nicolaskruchten

@etpinard best to review commit-by-commit. The first six (and the eighth as well as test reorg in a443747) are pre-cleaning and various obscure bugfixes. The meat of the changes are in a11ec44 (tracking changes), 4af2bf8 (adding the new attributes & inheritance), and 03baca7 (responding to the changes in Plotly.react). I missed selectedpoints in the first iteration, that's added in 6a9d0d7, and tests are added in the last commit 54304b4.

@@ -2074,7 +2072,7 @@ function _relayout(gd, aobj) {
if(manageArrays.isAddVal(vi)) {
undoit[ai] = null;
} else if(manageArrays.isRemoveVal(vi)) {
undoit[ai] = obji;
undoit[ai] = (nestedProperty(layout, arrayStr).get() || [])[i];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We might not have anyone who cares about undo/redo anymore, but we have it so here's a fix. We were missing the .get() the way this was written before.

Copy link
Member

Choose a reason for hiding this comment

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

@dmt0 @nicolaskruchten for the future perhaps ^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have a tentative plan to 🔪 undo/redo in v2 #420 (comment), with the rationale:

we used these in our old workspace, but this is really not the right level to be managing this issue, as plots may be coupled to other application state (ie changes in data arrays) and using our queue would muck up that correspondence.

so unless we reverse that conclusion let's not build anything else using it.

// gd.data[inputIndices[i]],
// fullLayout._tracePreGUI[gd._fullData[fullIndices[i]]._fullInput.uid],
// {dimensions: gdDimensions[i]}
// );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Feels like the better way to handle this is to create a dimensionorder attribute, so we can just store that, rather than directly mutating the dimensions array.

Like columnorder in table traces, but that feature isn't fully built out either so I left both of these as open items for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably deserves a new issue 🚀

@chriddyp
Copy link
Member

a parallel release candidate for this feature in dcc

A parallel release works for me 👍 Users can try each feature out independently

@chriddyp
Copy link
Member

LMK, I can make that branch if you like.

That would be great!

@alexcjohnson
Copy link
Collaborator Author

A branch containing both transitions and uirevision, to be used for dcc 0.39.0rc4: https://github.com/plotly/plotly.js/compare/transitions-plus-uirevision?expand=1
https://18863-45646037-gh.circle-artifacts.com/0/plotly.min.js

@chriddyp
Copy link
Member

a parallel release candidate for this feature in dcc

For those reading along, see plotly/dash-core-components#387 for the dash-core-components prerelease and https://community.plot.ly/t/preserving-ui-state-like-zoom-in-dcc-graph/15793/3 for the installation instructions and examples 🎉

@nicolaskruchten
Copy link
Member

Last chance attempt to change the name... would we consider uidescriptor or uidigest or something like that instead of revision? revision still strongly suggests monotonically-increasing numbers to me, like version numbers.

@nicolaskruchten
Copy link
Member

uiregime, uicontext? I like uisetting actually.

@etpinard
Copy link
Contributor

revision still strongly suggests monotonically-increasing numbers to me, like version numbers.

datarevision also needs not to be monotonically increasing to work. Still voting uirevision.

@nicolaskruchten
Copy link
Member

datarevision also needs not to be monotonically increasing to work

Yep, I have the same problems with that key name :) But perhaps that ship has indeed sailed. ⛵️

@alexcjohnson
Copy link
Collaborator Author

I agree it's not a great name, but at this point I think it's the least bad. I wouldn't insist on concordance with datarevision, since that one at least has a very common monotonic use case (ie real time or as-available data)... but that's at least a vote in its favor, and all the other options we've floated IMO have some sort of equally or more problematic association.

@@ -136,6 +136,9 @@ var radialAxisAttrs = {
}
};

// radial title is not gui-editable, so it needs dflt: '', similar to carpet axes.
radialAxisAttrs.title.text.dflt = '';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI @rmoestl - one we missed during #3276

@alexcjohnson
Copy link
Collaborator Author

@etpinard a few updates to make this work compatible with camera up #3256 and title alignment #3276, and some looser tests for titles and tickson #3275 to work on my machine. Does this all look OK to merge now that we have the 👍 from @chriddyp ?

@etpinard
Copy link
Contributor

Yep, ok to merge 💃 !

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

Successfully merging this pull request may close these issues.

None yet

5 participants