-
Notifications
You must be signed in to change notification settings - Fork 79
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 unmix state when changing value #3296
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3296 +/- ##
=======================================
Coverage 88.80% 88.80%
=======================================
Files 125 125
Lines 19019 19021 +2
=======================================
+ Hits 16890 16892 +2
Misses 2129 2129 ☔ View full report in Codecov by Sentry. |
* used in RGB presets logic
64fb8b4
to
d79d837
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was super intricate to reproduce the bug in the first place, code changes are straightforward so I'm good with merging the PR as is if you are!
if new_value != self.value: | ||
self.value = new_value | ||
else: | ||
self._on_value_changed({'new': new_value}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If new_value == self.value
, why is _on_value_changed
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to propagate to all the linked states (this happens when clicking to unmix the state, self.value
is the nominal value (of the first linked state) and we want to propagate that to all other linked glue states)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see the description above for the scenario where this makes a difference, probably could do the same by forcing a send_state, but that would then call _on_value_changed
twice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks like code.
Co-authored-by: Kyle Conroy <[email protected]>
Description
This pull request fixes a bug in plot options
unmix_state
when the value is being changed by unmixing (which as far as I can tell only happens from calls within the RGB presets and only shows as a bug when the original tab selection is the last tab that is iterated during the RGB presets call and so hard to cover with a regression test).Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.