-
-
Notifications
You must be signed in to change notification settings - Fork 415
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
Respect background color when calculating scale bar color #5270
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5270 +/- ##
==========================================
+ Coverage 88.76% 89.03% +0.27%
==========================================
Files 579 578 -1
Lines 49093 48946 -147
==========================================
+ Hits 43576 43579 +3
+ Misses 5517 5367 -150
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Thanks for the fix. Not ideal, but I think it's good enough to merge, so I'll do so after 24 hours unless anyone objects.
One optional suggestion is to add a test to cover the original bug. There were some existing issues with the design/implementation here, so I might consider changing some things in the future, but wouldn't want to break this again.
Here's my best attempt at writing a test, which unfortunately needs make_napari_viewer
.
def test_box_color_with_background_color_override(make_napari_viewer):
viewer = make_napari_viewer()
qt_viewer = viewer.window._qt_viewer
qt_viewer.canvas.background_color_override = [1, 0, 0]
np.testing.assert_equal(qt_viewer.scale_bar.node.text.color.rgb, [[0, 1, 1]])
napari/_vispy/overlays/scale_bar.py
Outdated
and hasattr(self.viewer.window, "_qt_viewer") | ||
and self.viewer.window._qt_viewer.canvas.background_color_override |
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.
Not great that we have to use private state here (or more generally that the scale bar has access to the whole of the viewer
), but I can't think of anything better right now.
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 have fixed this by use the parent of node and allowing to use of this fix with ViewerModel
not only Viewer
.
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
* if background_color_override is set then use it for axes color * improve guard of override * fix implementation, add test
Description
Currently color of the scale bar is calculated base on the canvas color assigned in the theme. But since #2270 is possible to overwrite background color. In this PR I add support to respect this option (used for example, in PartSeg).
Type of change
References
How has this been tested?
as there are small differences between the two Qt bindings.
Final checklist:
trans.
to make them localizable.For more information see our translations guide.