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.

Sign up for GitHub

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

Jump to bottom

Fix: Prevent loss of domain information when template field is present in layout #7283

Merged
merged 6 commits into from
Nov 26, 2024
Merged

Fix: Prevent loss of domain information when template field is present in layout #7283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf6e5db
Clear inputDomain in diffLayout
marthacryan Nov 22, 2024
e506f43
Add test that reproduces bug
marthacryan Nov 25, 2024
33b3ec4
Add draftlogs entry
marthacryan Nov 25, 2024
32abe6a
Remove initial check of domain
marthacryan Nov 25, 2024
7e3c6c9
Change name of test to be more result oriented
marthacryan Nov 25, 2024
5f128bb
Add comment to explain else case
marthacryan Nov 25, 2024
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
1 change: 1 addition & 0 deletions draftlogs/7283_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fix handling of new domain values given in the Plotly.react function to prevent loss of new domain values. [[#7283](https://github.com/plotly/plotly.js/pull/7283)]
31 changes: 26 additions & 5 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2811,6 +2811,32 @@ function diffLayout(gd, oldFullLayout, newFullLayout, immutable, transition) {
return PlotSchema.getLayoutValObject(newFullLayout, parts);
}

// Clear out any _inputDomain that's no longer valid
for (var key in newFullLayout) {
if (!key.startsWith('xaxis') && !key.startsWith('yaxis')) {
continue;
}
if (!oldFullLayout[key]) {
continue;
}
var newDomain = newFullLayout[key].domain;
var oldDomain = oldFullLayout[key].domain;
var oldInputDomain = oldFullLayout[key]._inputDomain;
if (oldFullLayout[key]._inputDomain) {
if (newDomain[0] === oldInputDomain[0] && newDomain[1] === oldInputDomain[1]) {
// what you're asking for hasn't changed, so let plotly.js start with what it
// concluded last time and iterate from there
newFullLayout[key].domain = oldFullLayout[key].domain;
} else if (newDomain[0] !== oldDomain[0] || newDomain[1] !== oldDomain[1]) {
// what you're asking for HAS changed, so clear _inputDomain and let us start from scratch
newFullLayout[key]._inputDomain = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think nestedProperty(...).set(null) is actually equivalent to delete newFullLayout[key]._inputDomain

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcjohnson Using delete may be slower comparing to setting to null. No?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose that’s faster and should be equivalent. This is not a high volume operation so I wouldn’t be too concerned about it, but we can leave it if you prefer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was using this syntax for consistency with the other places in the codebase that delete this field, but I don't have a strong opinion on which we use.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marthacryan Maybe worth leaving a comment explaining why we do nothing in the else case? (I know we discussed but... for posterity)

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 yeah good point Emily!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexcjohnson I added a comment explaining the else case based off of this explanation from you about that case:
"this situation is perhaps actually ambiguous: did you pass back in the previous layout as modified by Plotly.js, so we should assume you really wanted _inputDomain to still apply? or did you explicitly set the new domain and it just happened to match the old final calculated domain - in which case we should clear _inputDomain. I guess generally oldDomain will be some random floating point number so it's unlikely you matched it exactly. better to assume you passed back in the same layout object and change nothing in this case."

Let me know if you have any corrections!

}
// We skip the else case (newDomain !== oldInputDomain && newDomain === oldDomain)
// because it's likely that if the newDomain and oldDomain are the same, the user
// passed in the same layout object and we should keep the _inputDomain.
}
}

var diffOpts = {
getValObject: getLayoutValObject,
flags: flags,
Expand Down Expand Up @@ -2863,11 +2889,6 @@ function getDiffFlags(oldContainer, newContainer, outerparts, opts) {
flags.rangesAltered[outerparts[0]] = 1;
}

// clear _inputDomain on cartesian axes with altered domains
if(AX_DOMAIN_RE.test(astr)) {
nestedProperty(newContainer, '_inputDomain').set(null);
}

// track datarevision changes
if(key === 'datarevision') {
flags.newDataRevision = 1;
Expand Down
17 changes: 17 additions & 0 deletions test/jasmine/tests/axes_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1796,6 +1796,23 @@ describe('Test axes', function() {
'input range, ' + msg + ': ' + ax.range);
}

it('can change axis domain on call to react when template is present', function(done) {
Plotly.newPlot(gd,
[{z: [[0, 1], [2, 3]], type: 'heatmap'}],
{ template: {}, xaxis: { domain: [0,1], scaleanchor: 'y' } }
)
.then(function() {
return Plotly.react(gd,
[{z: [[0, 1], [2, 3]], type: 'heatmap'}],
{ template: {}, xaxis: { domain: [0.1, 0.9] } }
);
})
.then(function() {
assertRangeDomain('xaxis', [-0.5,1.5], [0.1, 0.9], [0.1, 0.9]);
})
.then(done, done.fail);
});

it('can change per-axis constrain:domain/range and constraintoward', function(done) {
Plotly.newPlot(gd,
// start with a heatmap as it has no padding so calculations are easy
Expand Down