Repository navigation
Implement toggling colorbar and bounding box in layerlist context menu - #8319
Conversation
There was a problem hiding this comment.
This works in the original PR as:
The right clicked layer must be the active layer (this is confusing to me!!!)
- bounding box enabled only when 1 layer selected
- colorbar enabled only when 1 layer selected and its an image or surface
I propose with my comments different behavior. I tested locally and found this to perform much closer to my expectation with both these features and similar layer list context items (which can perform on multiple layers)
- bounding box enabled when any layers are selected, and will be toggled for all selected layers
- colorbar enabled with any number of layers, as long as those layers are surface or image. will toggle for all selected layers
Even if we don't go with my proposed enablement, the callbacks can be used as I've proposed (though the for loop is unnecessary), because enablement already protects against the guards. We could add back the non-image/surface guard to present the warning and do enablement on any number of selected layers
Let me know if I'm missing something
| MenuId.LAYERLIST_CONTEXT, | ||
| SubmenuItem( | ||
| submenu=MenuId.LAYERS_CONTEXT_VISUALIZATION, | ||
| title=trans._('Visualization'), |
There was a problem hiding this comment.
We could alternatively use "Overlays". I think either is ok, especially because we may want to add some kind of non-overlay visualization to this menu?
| ll.append(new) | ||
|
|
||
|
|
||
| def _toogle_bounding_box(ll: LayerList) -> None: |
There was a problem hiding this comment.
| def _toogle_bounding_box(ll: LayerList) -> None: | |
| def _toggle_bounding_box(ll: LayerList) -> None: |
| Action( | ||
| id='napari.layer.bounding_box', | ||
| title=trans._('Bounding Box'), | ||
| callback=_layer_actions._toogle_bounding_box, |
There was a problem hiding this comment.
| callback=_layer_actions._toogle_bounding_box, | |
| callback=_layer_actions._toggle_bounding_box, |
(method in _layer_actions needs updated as well)
| if not (layer := ll.selection.active): | ||
| return | ||
|
|
||
| if not isinstance(layer, Image | Surface): | ||
| raise NotImplementedError( | ||
| trans._( | ||
| 'Colorbar is only implemented for Images and Surfaces', | ||
| deferred=True, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This is all unnecessary because of the enablement, right? Why would we want this?
| (LLSCK.num_selected_layers == 1) | ||
| & ( | ||
| LLSCK.all_selected_layers_image | ||
| | LLSCK.all_selected_layers_surfaces | ||
| ) |
There was a problem hiding this comment.
| (LLSCK.num_selected_layers == 1) | |
| & ( | |
| LLSCK.all_selected_layers_image | |
| | LLSCK.all_selected_layers_surfaces | |
| ) | |
| LLSCK.all_selected_layers_image | |
| | LLSCK.all_selected_layers_surfaces |
| 'id': MenuId.LAYERS_CONTEXT_VISUALIZATION, | ||
| } | ||
| ], | ||
| enablement=LLSCK.num_selected_layers == 1, |
There was a problem hiding this comment.
| enablement=LLSCK.num_selected_layers == 1, | |
| enablement=LLSCK.num_selected_layers > 0, |
| if not (layer := ll.selection.active): | ||
| return | ||
|
|
||
| if not isinstance(layer, Image | Surface): | ||
| raise NotImplementedError( | ||
| trans._( | ||
| 'Colorbar is only implemented for Images and Surfaces', | ||
| deferred=True, | ||
| ) | ||
| ) | ||
| layer.colorbar.visible = not layer.colorbar.visible |
There was a problem hiding this comment.
| if not (layer := ll.selection.active): | |
| return | |
| if not isinstance(layer, Image | Surface): | |
| raise NotImplementedError( | |
| trans._( | |
| 'Colorbar is only implemented for Images and Surfaces', | |
| deferred=True, | |
| ) | |
| ) | |
| layer.colorbar.visible = not layer.colorbar.visible | |
| for layer in ll.selection: | |
| layer.colorbar.visible = not layer.colorbar.visible |
| if not (layer := ll.selection.active): | ||
| return | ||
| layer.bounding_box.visible = not layer.bounding_box.visible |
There was a problem hiding this comment.
| if not (layer := ll.selection.active): | |
| return | |
| layer.bounding_box.visible = not layer.bounding_box.visible | |
| for layer in ll.selection | |
| layer.bounding_box.visible = not layer.bounding_box.visible |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8319 +/- ##
==========================================
+ Coverage 93.03% 93.08% +0.05%
==========================================
Files 710 710
Lines 63989 64006 +17
==========================================
+ Hits 59529 59581 +52
+ Misses 4460 4425 -35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Tim Monko <timmonko@gmail.com>
| raise NotImplementedError( | ||
| trans._( | ||
| 'Colorbar is only implemented for Images and Surfaces', | ||
| deferred=True, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I keep this raise to have clean information about what goes wrong when our check do not properly limit layer selection.
| def _all_support_colorbar(s: LayerSel) -> bool: | ||
| return bool(s and all(hasattr(x, 'colorbar') for x in s)) |
There was a problem hiding this comment.
really like this addition -- will be much clearer to support and allow custom layers to access!!!!
|
@brisvag I added highlight to this, I'll leave it up to you to add. We should at least add this new feature to the docs somewhere. |
|
I added the whole overlays shebang to the docs, should have done this a while back :P napari/docs#857 |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
References and relevant issues
https://napari.zulipchat.com/#narrow/channel/215289-release/topic/0.2E6.2E5/near/542262710
Description
Add an option to control the visibility of colorbar and bounding box from layerlist context menu.