Skip to content

Preserve ui state across Plotly.react redraws #3236

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

Merged
merged 35 commits into from
Nov 30, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
3a70fee
make hover label test independent of selection order
alexcjohnson Nov 3, 2018
a86ca58
fix and :lock: bug with impliedEdits and groupby transform
alexcjohnson Oct 31, 2018
4ca328a
remove impliedEdits that got forgotten in #3044
alexcjohnson Oct 31, 2018
082ac38
alias `Lib.nestedProperty` separately in plot_api
alexcjohnson Oct 31, 2018
b134a52
fix obscure undo/redo bug with container array relayout removal
alexcjohnson Oct 31, 2018
c62cf25
clean up handleHover3d
alexcjohnson Nov 2, 2018
a11ec44
add _gui(Restyle|Relayout|Update) and _storeDirectGUIEdit and track c…
alexcjohnson Nov 2, 2018
10ddbb9
fix and :lock: problem with dragging colorbars in sub-containers
alexcjohnson Nov 6, 2018
4af2bf8
add and coerce uirevision attributes
alexcjohnson Nov 8, 2018
03baca7
use uirevisions in Plotly.react to preserve ui state
alexcjohnson Nov 9, 2018
6a9d0d7
add selectedpoints to uirevision framework
alexcjohnson Nov 9, 2018
a443747
break Plotly.react tests out of plot_api_test into plot_api_react_test
alexcjohnson Nov 9, 2018
54304b4
test uirevision + react logic
alexcjohnson Nov 9, 2018
86b90d4
remove `apply` arg from _storeDirectGUIEdit
alexcjohnson Nov 10, 2018
502082f
:palm_tree: uirevision tests
alexcjohnson Nov 10, 2018
5182f6a
fix and :lock: polar uirevisions
alexcjohnson Nov 10, 2018
89dde84
:lock: ternary uirevisions
alexcjohnson Nov 10, 2018
778ec15
fix and :lock: mapbox uirevisions
alexcjohnson Nov 10, 2018
21f5db5
fix and :lock: editable shapes & annotations uirevision
alexcjohnson Nov 11, 2018
8f0f075
:palm_tree: uirevision tests
alexcjohnson Nov 11, 2018
eb728fe
fix and :lock: editable: true uirevisions
alexcjohnson Nov 11, 2018
c95ae07
revert form of rangeslider relayout call (but it's still _guiRelayout)
alexcjohnson Nov 12, 2018
7ab8630
comment on uirevision<->uid in trace.uirevision attribute description
alexcjohnson Nov 12, 2018
756308f
record real initial axis (auto)range so we can update to autorange
alexcjohnson Nov 13, 2018
874a72a
remove now-obsolete comment about autorange/autofill
alexcjohnson Nov 13, 2018
090231b
fix and :lock: selectionrevision with groupby
alexcjohnson Nov 13, 2018
5a27c00
update trace.uirevision description with what it does/doesn't control
alexcjohnson Nov 13, 2018
2034941
partial preGUI fix for 3D camera
alexcjohnson Nov 19, 2018
6cfe7c5
Merge branch 'master' into ui-key
alexcjohnson Nov 29, 2018
8d11985
write camera.up back to layout only when necessary
alexcjohnson Nov 29, 2018
d9b3aea
Merge branch 'master' into ui-key
alexcjohnson Nov 29, 2018
0621e43
update uirevisions for new title attribute structure #3276
alexcjohnson Nov 30, 2018
bdef7d3
more permissive tickson rotation test for AJ's machine
alexcjohnson Nov 30, 2018
2b77911
looser acceptance for title centering, to work on AJ's machine
alexcjohnson Nov 30, 2018
067bd7d
biger tickwidth difference in tickson rotation test
alexcjohnson Nov 30, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add selectedpoints to uirevision framework
  • Loading branch information
alexcjohnson committed Nov 9, 2018
commit 6a9d0d78e9d7da0882ec8d9fa8214f4fc805cf69
1 change: 1 addition & 0 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2387,6 +2387,7 @@ var layoutUIControlPatterns = [
// same for trace attributes: if `attr` is given it's in layout,
// or with no `attr` we use `trace.uirevision`
var traceUIControlPatterns = [
{pattern: /^selectedpoints$/, attr: 'selectionrevision'},
// "visible" includes trace.transforms[i].styles[j].value.visible
{pattern: /(^|value\.)visible$/, attr: 'legend.uirevision'},
{pattern: /^dimensions\[\d+\]\.constraintrange/},
Expand Down
9 changes: 9 additions & 0 deletions src/plots/cartesian/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,15 @@ function isOnlyOnePointSelected(searchTraces) {
function updateSelectedState(gd, searchTraces, eventData) {
var i, searchInfo, cd, trace;

// before anything else, update preGUI if necessary
for(i = 0; i < searchTraces.length; i++) {
var fullInputTrace = searchTraces[i].cd[0].trace._fullInput;
var tracePreGUI = gd._fullLayout._tracePreGUI[fullInputTrace.uid];
if(tracePreGUI.selectedpoints === undefined) {
tracePreGUI.selectedpoints = fullInputTrace._input.selectedpoints || null;
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Missed this one earlier as it doesn't go through restyle or emit a plotly_restyle event - but anyway it wants to be a special case so that we don't push individual array elements into _tracePreGUI, we link the array as a unit.

BTW I'm working off trace._fullInput here but I notice farther down we go straight from trace to trace._input - have we ever looked at selections on groupby traces?

Copy link
Contributor

Choose a reason for hiding this comment

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

have we ever looked at selections on groupby traces?

I don't remember doing that 😨

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it looks like selections work fine with groupby 😌 there's some potential confusion around pointNumber vs pointIndex in the event data, but all the information is there (curveNumber and pointIndex give you the index in the input arrays, pointNumber is the index within the group), and visible behavior is all correct.

The only thing that looks odd to me is fullTrace.selectedpoints:

  • Immediately after drawing it has the selected input array indices from items in that group, but fullTrace._fullInput.selectedpoints is missing
  • After you make an edit (restyle or zoom, for example) fullTrace.selectedpoints and fullTrace._fullInput.selectedpoints both have the entire array of input array indices.

I don't see any practical effects of this difference, though it's possible Plotly.react's diffing algo will care.

Anyway I think I've found a uirevision + selectedpoints bug, investigating.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyway I think I've found a uirevision + selectedpoints bug, investigating.

haha looks like it's just the missing fullInput.selectedpoints I mentioned above - fix coming, and perhaps I'll make fullTrace.selectedpoints self-consistent across edits as well while I'm at it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alright, 🔒 in 090231b - which should also speed things up a bit in the normal non-transform case where data and fullData can point to the same array (as they will after an edit anyway) so we only need to build it once.


if(eventData) {
var pts = eventData.points || [];

Expand Down
9 changes: 9 additions & 0 deletions src/plots/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,15 @@ module.exports = {
'Defaults to `layout.uirevision`.'
].join(' ')
},
selectionrevision: {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale here for putting selectionrevision in layout instead of in the individual traces?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha I see, data[i].uirevision also works for selectedpoints 👌

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha I see, data[i].uirevision also works for selectedpoints 👌

Does it? I don't think so, at least I don't see it when I try that.

What's the rationale here for putting selectionrevision in layout instead of in the individual traces?

I can certainly think of a use case for resetting selectedpoints but not resetting any of the other trace attributes - any time the basic meaning of the trace is unchanged but the point indices are altered, like if you had a trace showing annual data and you change it to monthly or you change the date range. And I guess you could imagine extending this argument to say selectedpoints could be invalidated for one trace but not for others, though it would create a weird UX situation in which the selected points in the trace where selectedpoints did NOT get reset get lost amid the sea of no-longer-deselected points in the trace where selectedpoints DID get reset. So between that and the fact that when the user creates a selection it's not a per-trace action, that's my argument for making selectedrevision a layout attribute. The app developer would give this attribute some value that depends on every input that can affect the identity of point indices in any trace.

Now there's still a question about what uirevision attributes the other trace attributes should link to. For the most part it seems like uid takes care of resetting these attributes (and note that if uid changes, even attributes linked to uirevision attributes layout will reset)... I linked colorbar.(x|y) to editrevision because this is more about the overall layout of the plot than about anything in the trace itself. And I made visible: 'legendonly' controlled by legend.uirevision, because (a) that's what controls layout.hiddenlabels for pie traces, and (b) the only use case I could think of for it, aside from those covered by trace.uid, is a "reset legend visibility" control (so legend.uirevision would just be set to the nclicks for this control).

That leaves name, colorbar.title, and dimensions[i].constraintrange (and presumably dimensionorder and columnorder when we get to those) controlled by trace.uirevision. I can't really think of a use case for this (again, aside from those covered by trace.uid), but it does seem like if there ever was one the attribute would belong in the trace, and I don't want to leave any attributes only under the control of the main layout.uirevision with no inheritance, since then to reset one of those attributes without resetting the whole rest of the plot would require setting all the other lower-level attributes to some constant value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it? I don't think so, at least I don't see it when I try that.

Ah. I think I got that from:

'Controls persistence of user-driven changes to the trace:',
'interactions like `selectedpoints` and type-specific ones such as',

I can certainly think of a use case for resetting selectedpoints but not resetting any of the other trace attributes

So between that and the fact that when the user creates a selection it's not a per-trace action, that's my argument for making selectedrevision a layout attribute.

You got me convinced. Thanks for writing your thought process down.

but it does seem like if there ever was one the attribute would belong in the trace, and I don't want to leave any attributes only under the control of the main layout.uirevision with no inheritance

Good point. I have no problem leaving trace.uirevision in there even though it has (very) limited use cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah. I think I got that from:

Good catch, that was wrong, thanks! -> 5a27c00

valType: 'any',
role: 'info',
editType: 'none',
description: [
'Controls persistence of user-driven changes in selected points',
'from all traces.'
].join(' ')
},
template: {
valType: 'any',
role: 'info',
Expand Down
1 change: 1 addition & 0 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,7 @@ plots.supplyLayoutGlobalDefaults = function(layoutIn, layoutOut, formatObj) {
coerce('datarevision');
var uirevision = coerce('uirevision');
coerce('editrevision', uirevision);
coerce('selectionrevision', uirevision);

coerce('modebar.orientation');
coerce('modebar.bgcolor', Color.addOpacity(layoutOut.paper_bgcolor, 0.5));
Expand Down