-
Notifications
You must be signed in to change notification settings - Fork 75
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
support exporting a plot from an unopened/inactive plugin #2934
support exporting a plot from an unopened/inactive plugin #2934
Conversation
64c23be
to
d1f0915
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2934 +/- ##
=======================================
Coverage 88.79% 88.79%
=======================================
Files 111 111
Lines 17236 17245 +9
=======================================
+ Hits 15305 15313 +8
- Misses 1931 1932 +1 ☔ View full report in Codecov by Sentry. |
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.
Great work, everything looks good to me and this is working really well!
Hmm, it looks like this saved something for the histogram, but I don't think the saved plot is correct. It saved out this image: [Deleted the images] Edit: Actually, I tried again and it worked the second time - maybe I hadn't restarted the kernel properly after checking out this branch. Now the only surprise to me is that it didn't give me an Overwrite warning the second time I ran it 🤔 |
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.
This improves the current behavior, so I'm approving. I did notice that the plot as rendered by this PR doesn't have the line indicating the stretch scaling, but I don't know how big a deal that is - we may want to address it later, but if you want to address it now I can re-review.
* when the plot was being updated, a plot update message was being sent when clearing the old data, briefly resetting the choices to an empty list, which cleared the selection. The broadcast now is not duplicated and so the choices remains intact
This is a separate ticket 🐱
Is it possible that they're just outside the default x-limits? |
6e8d435
to
b89689a
Compare
It also seems that sometimes on first attempt, this is still failing and clearing the selection - I'm looking into seeing if that is an easy fix now |
I'm going to go ahead and merge this as-is since it fixes the reported bug and should get in. If we keep running into the "selection clearing" bug (which definitely happens less with this PR than without it), that can be investigated and fixed as a follow-up. |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…cope#2934) * support exporting a plot from an unopened/inactive plugin * prevent clearing plot selection (under most conditions)
Description
This pull request fixes the case of exporting the stretch histogram from the export plugin before the plot options plugin has ever been opened. It does so by implementing an optional callback on the
Plot
object itself to call a plugin method to update itself, which will be called if the "parent" plugin is not considered active. In cases where no widget exists because the parent plugin has never been opened, it also renders the widget offscreen in the export plugin.I'm not really sure the best way to test this since the previous behavior didn't result in errors, just a blank image - but open to any suggestions!
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.