-
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
BUG: Imviz bug fixes ported from PR 1340 #1392
Conversation
1a0d82a
to
ae706b6
Compare
Codecov Report
@@ Coverage Diff @@
## main #1392 +/- ##
==========================================
+ Coverage 84.91% 84.99% +0.07%
==========================================
Files 91 91
Lines 8288 8295 +7
==========================================
+ Hits 7038 7050 +12
+ Misses 1250 1245 -5
Continue to review full report at Codecov.
|
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.
Looks good!
- Fixed a bug where coordinates display erroneously showing info from | ||
the reference image even when it is not visible. [#1392] |
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 seems to now account for layer.visible
, but does it need to also account for layer.state.bitmap_visible
and/or layer.state.contour_visible
?
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.
Hmm blink only sets .visible
. I wasn't even aware of the other two attributes. What do they control?
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.
Whether the image and contours are visible. So in order to see an image, both layer.visible
and layer.state.bitmap_visible
must be True (iirc).
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.
I never had to set .bitmap_visible
for it to show. Is something not working for you?
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 you leave the top layer's visibility on, but disable the bitmap_visible
(either through the API or through the plot options), then the coordinates display will still use that layer, even though the user is not seeing it. You could argue that this is expected behavior since the layer as a whole is still enabled and that this will be "addressed" if/when we add an indication of the label of the "top/active" layer.
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.
Approving for now. I think we'll need to give more thought to the bitmap_visible=False
case in determining the top-layer, but that is probably a broader decision than just the line profile case being edited here and likely needs a refactor to move the top-layer-detection logic to the viewer level so it can be implemented consistently across all plugins that need to access that information.
Thanks for the reviews and approvals! |
Description
This pull request is to fix some bugs. The fixes were part of #1340 but now separated out into a proper bugfix PR.
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.CHANGES.rst
?