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

fix uirevision autorange bug #6046

Merged
merged 2 commits into from
Dec 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions draftlogs/6046_fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fix interaction between `uirevision` and `autorange`. Because we push `autorange` and `range` back into `layout`, there can be times it looks like we're applying GUI-driven changes on top of explicit autorange and other times it's an implicit autorange, even though the user's intent was always implicit. This fix treats them as equivalent. [[#6046](https://github.com/plotly/plotly.js/pull/6046)]
45 changes: 37 additions & 8 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2396,7 +2396,8 @@ function findUIPattern(key, patternSpecs) {
var spec = patternSpecs[i];
var match = key.match(spec.pattern);
if(match) {
return {head: match[1], attr: spec.attr};
var head = match[1] || '';
return {head: head, tail: key.substr(head.length + 1), attr: spec.attr};
}
}
}
Expand Down Expand Up @@ -2448,26 +2449,54 @@ function valsMatch(v1, v2) {

function applyUIRevisions(data, layout, oldFullData, oldFullLayout) {
var layoutPreGUI = oldFullLayout._preGUI;
var key, revAttr, oldRev, newRev, match, preGUIVal, newNP, newVal;
var key, revAttr, oldRev, newRev, match, preGUIVal, newNP, newVal, head, tail;
var bothInheritAutorange = [];
var newAutorangeIn = {};
var newRangeAccepted = {};
for(key in layoutPreGUI) {
match = findUIPattern(key, layoutUIControlPatterns);
if(match) {
revAttr = match.attr || (match.head + '.uirevision');
head = match.head;
tail = match.tail;
revAttr = match.attr || (head + '.uirevision');
oldRev = nestedProperty(oldFullLayout, revAttr).get();
newRev = oldRev && getNewRev(revAttr, layout);

if(newRev && (newRev === oldRev)) {
preGUIVal = layoutPreGUI[key];
if(preGUIVal === null) preGUIVal = undefined;
newNP = nestedProperty(layout, key);
newVal = newNP.get();

if(valsMatch(newVal, preGUIVal)) {
if(newVal === undefined && key.substr(key.length - 9) === 'autorange') {
bothInheritAutorange.push(key.substr(0, key.length - 10));
if(newVal === undefined && tail === 'autorange') {
bothInheritAutorange.push(head);
}
newNP.set(undefinedToNull(nestedProperty(oldFullLayout, key).get()));
continue;
} else if(tail === 'autorange' || tail.substr(0, 6) === 'range[') {
// Special case for (auto)range since we push it back into the layout
// so all null should be treated equivalently to autorange: true with any range
var pre0 = layoutPreGUI[head + '.range[0]'];
var pre1 = layoutPreGUI[head + '.range[1]'];
var preAuto = layoutPreGUI[head + '.autorange'];
if(preAuto || (preAuto === null && pre0 === null && pre1 === null)) {
// Only read the input layout once and stash the result,
// so we get it before we start modifying 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.

This one took me a while to figure out... thorough testing pays dividends: prior to this stashing, everything worked except react to STAY implicit, which I would have thought would be an obvious noop.

if(!(head in newAutorangeIn)) {
var newContainer = nestedProperty(layout, head).get();
newAutorangeIn[head] = newContainer && (
newContainer.autorange ||
(newContainer.autorange !== false && (
!newContainer.range || newContainer.range.length !== 2)
)
);
}
if(newAutorangeIn[head]) {
newNP.set(undefinedToNull(nestedProperty(oldFullLayout, key).get()));
continue;
}
}
}
}
} else {
Expand All @@ -2478,12 +2507,12 @@ function applyUIRevisions(data, layout, oldFullData, oldFullLayout) {
// so remove it from _preGUI for next time.
delete layoutPreGUI[key];

if(key.substr(key.length - 8, 6) === 'range[') {
newRangeAccepted[key.substr(0, key.length - 9)] = 1;
if(match && match.tail.substr(0, 6) === 'range[') {
newRangeAccepted[match.head] = 1;
}
}

// Special logic for `autorange`, since it interacts with `range`:
// More special logic for `autorange`, since it interacts with `range`:
// If the new figure's matching `range` was kept, and `autorange`
// wasn't supplied explicitly in either the original or the new figure,
// we shouldn't alter that - but we may just have done that, so fix it.
Expand Down
77 changes: 57 additions & 20 deletions test/jasmine/tests/plot_api_react_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,59 @@ describe('Plotly.react and uirevision attributes', function() {
.then(done, done.fail);
});

function setCartesianRanges(xRange, yRange) {
return function() {
return Registry.call('_guiRelayout', gd, {
'xaxis.range': xRange,
'yaxis.range': yRange
});
};
}

function checkCartesianRanges(xRange, yRange, msg) {
return checkState([], {
'xaxis.range': [xRange],
'yaxis.range': [yRange]
}, msg);
}

it('treats explicit and implicit cartesian autorange the same', function(done) {
function fig(explicit, uirevision) {
return {
data: [{z: [[1, 2], [3, 4]], type: 'heatmap', x: [0, 1, 2], y: [3, 4, 5]}],
layout: {
xaxis: explicit ? {autorange: true, range: [0, 2]} : {},
yaxis: explicit ? {autorange: true, range: [3, 5]} : {},
uirevision: uirevision
}
};
}

// First go from implicit to explicit and back after zooming in
Plotly.newPlot(gd, fig(false, 'a'))
.then(checkCartesianRanges([0, 2], [3, 5], 'initial implicit'))
.then(setCartesianRanges([2, 4], [5, 7]))
.then(checkCartesianRanges([2, 4], [5, 7], 'zoomed from implicit'))
.then(_react(fig(true, 'a')))
.then(checkCartesianRanges([2, 4], [5, 7], 'react to explicit'))
.then(_react(fig(true, 'a')))
.then(checkCartesianRanges([2, 4], [5, 7], 'react to STAY explicit'))
.then(_react(fig(false, 'a')))
.then(checkCartesianRanges([2, 4], [5, 7], 'back to implicit'))
// then go from explicit to implicit and back after zooming in
.then(_react(fig(true, 'b')))
.then(checkCartesianRanges([0, 2], [3, 5], 'new uirevision explicit'))
.then(setCartesianRanges([4, 6], [7, 9]))
.then(checkCartesianRanges([4, 6], [7, 9], 'zoomed from explicit'))
.then(_react(fig(false, 'b')))
.then(checkCartesianRanges([4, 6], [7, 9], 'react to implicit'))
.then(_react(fig(false, 'b')))
.then(checkCartesianRanges([4, 6], [7, 9], 'react to STAY implicit'))
.then(_react(fig(true, 'b')))
.then(checkCartesianRanges([4, 6], [7, 9], 'back to explicit'))
.then(done, done.fail);
});

it('respects reverting an explicit cartesian axis range to auto', function(done) {
function fig(xRange, yRange) {
return {
Expand All @@ -1378,28 +1431,12 @@ describe('Plotly.react and uirevision attributes', function() {
};
}

function setRanges(xRange, yRange) {
return function() {
return Registry.call('_guiRelayout', gd, {
'xaxis.range': xRange,
'yaxis.range': yRange
});
};
}

function checkRanges(xRange, yRange) {
return checkState([], {
'xaxis.range': [xRange],
'yaxis.range': [yRange]
});
}

Plotly.newPlot(gd, fig([1, 3], [4, 6]))
.then(checkRanges([1, 3], [4, 6]))
.then(setRanges([2, 4], [5, 7]))
.then(checkRanges([2, 4], [5, 7]))
.then(checkCartesianRanges([1, 3], [4, 6], 'initial explicit ranges'))
.then(setCartesianRanges([2, 4], [5, 7]))
.then(checkCartesianRanges([2, 4], [5, 7], 'zoomed to different explicit'))
.then(_react(fig(undefined, undefined)))
.then(checkRanges([0, 2], [3, 5]))
.then(checkCartesianRanges([0, 2], [3, 5], 'react to autorange'))
.then(done, done.fail);
});

Expand Down