Enh: Add zoom options and keybindings to View menu #7200
Conversation
|
@lucyleeow Right? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7200 +/- ##
===========================================
+ Coverage 73.76% 92.75% +18.98%
===========================================
Files 582 623 +41
Lines 54897 57205 +2308
===========================================
+ Hits 40496 53061 +12565
+ Misses 14401 4144 -10257 ☔ View full report in Codecov by Sentry. |
|
So pyapp-kit/app-model#220 has been merged. |
| @@ -90,6 +90,8 @@ def contributables(cls) -> set['MenuId']: | |||
| class MenuGroup: | |||
| NAVIGATION = 'navigation' # always the first group in any menu | |||
| RENDER = '1_render' | |||
| # View menu | |||
| ZOOM = 'zoom' | |||
There was a problem hiding this comment.
FYI I think the order that groups are shown in the menu is lexigraphic (which is why we have '1' and '2' preceding group names). This is fine, just letting you know
There was a problem hiding this comment.
Yeah ngl I have no idea what the structure of this MenuGroup class really is... should Plugins stuff actually be a subclass, like LAYERLIST_CONTEXT? And also File stuff? And then this would be the only group in the View subclass? Agreed we can leave as is for now but should probably decide and update docstring
There was a problem hiding this comment.
I just noticed the subclasses! I mean maybe that is what Talley had in mind as I see it was you and I that added the plugins and file menu bar ones, while the others sit within subclasses. I have no idea with 'RENDER' means though?
There was a problem hiding this comment.
I have no idea with 'RENDER' means though?
lol neither. But it seems like maybe the top level groups are maybe supposed to be used across other menus? Right now we use them in View menu and Help menu but I don't think that answers any questions about its semantics lol
I'm using the app-model Standard keybinding.
It has been merged, so I guess we can wait with this PR for a release of app-model? |
There was a problem hiding this comment.
This works as intended and the code looks good! I don't mind if we wait for app-model release to update this PR or if we just release and you come back later with a tiny PR to update to use the standard. I guess I'd rather have the feature than not? 🤷♀️
@psobolewskiPhD did you find adding the actions a little less convoluted in terms of the files you had to edit? We've been trying to simplify the whole thing.
| @@ -90,6 +90,8 @@ def contributables(cls) -> set['MenuId']: | |||
| class MenuGroup: | |||
| NAVIGATION = 'navigation' # always the first group in any menu | |||
| RENDER = '1_render' | |||
| # View menu | |||
| ZOOM = 'zoom' | |||
There was a problem hiding this comment.
Yeah ngl I have no idea what the structure of this MenuGroup class really is... should Plugins stuff actually be a subclass, like LAYERLIST_CONTEXT? And also File stuff? And then this would be the only group in the View subclass? Agreed we can leave as is for now but should probably decide and update docstring
Hmm yeah it seems like app-model only has equal (for which the virtual key is plus, though not sure what that means) - and this just ends up as |
|
@DragaDoncila yes! it was much easier, I thought I was doing something wrong! See: #7200 (comment) |
|
Amazing! I don't know how I missed the comment: #7200 (comment) but I'm glad it's easier now. Indeed I am considering closing #6516 for now, and opening a new issue if we find any new problems later. WDYT? |
|
I think the outstanding issue is
The interaction between app-model action keybinds and the Prefereces/napari keybinds is still problematic. |
|
Let me make that more explicit in #7059 Edit: the other point I was thinking about is how menu action keybindings take precedence over passing shortcut to window (https://napari.zulipchat.com/#narrow/stream/212875-general/topic/Keybinding.20functionality.20questions/near/462858964) |
|
@psobolewskiPhD app-model 0.3.0 just released, do you want to update this PR to use the standard zoom reset keybinding? I think you'll also need to bump the app-model dependency |
|
Hmm is this where we should have been using the constraints bot rather than updating the files manually...? Is updating them manually fine, just annoying...? @Czaki |
I think this is true, bot would have just made it more convenient. |
| @@ -113,6 +126,45 @@ def _get_current_tooltip_visibility() -> bool: | |||
| keybindings=[{'primary': KeyMod.CtrlCmd | KeyMod.Alt | KeyCode.KeyP}], | |||
| toggled=ToggleRule(get_current=_get_current_play_status), | |||
| ), | |||
| Action( | |||
| id='napari.viewer.reset_view', | |||
| title=trans._('Reset Zoom'), | |||
There was a problem hiding this comment.
I'm not sure "Reset Zoom" is the right name here, because it also resets the camera angles. So I propose:
| title=trans._('Reset Zoom'), | |
| title=trans._('Reset View'), |
There was a problem hiding this comment.
Yeah I do think that is better with the current behavior, thanks for catching it.
But maybe we actually want it to only affect zoom?
So something like Fit to View (this is Command-9 on macOS), which may be slightly more intuitive than Reset View.
Or that plus something like Actual Size which would be zoom = 1
There was a problem hiding this comment.
Hmm yeah I think if it's changing camera angles we shouldn't call it reset zoom. If there is a standard keybinding for "fit to view" I'd say that should be used for this current behaviour, and then Reset Zoom should be separate, and it does what it says on the box and just zooms out...
There was a problem hiding this comment.
But the current behavior isn't really Fit to view because it also rotates the camera...
So still need some zoom-only functions.
Let me check if there already is a viewer keybinding for Reset -- yup, Command/Control-R
So maybe we don't need Reset View (the current "Reset Zoom" behavior) in the menu? It's a viewer button (the home). Or we could add Reset View to the menu with that keybinding, but if the user changes the keybinding in napari settings it will be an issue 🫤
|
Ok, so to do napari/napari/components/viewer_model.py Line 429 in e861f80 So options are:
|
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
|
I'm open to all three options (private new method please), but prefer option 1, with default to True. |
I added the kwarg to Edit: Is @Czaki I get where you're coming from. I'd like them to be detected for collision, but I dont think they need to be editable -- plenty of programs--every web browser I have access to for example--impelement this trio (I made the PR in reponse to a user being surprised they didn't already work), much like cut, copy, paste, and they're not editable. |
|
I like "fit to view" or "fit to canvas". So I'm happy to merge this PR as-is. And I like very much that we have this functionality now @psobolewskiPhD! 😊 🙏 |
References and relevant issues
Closes #4855
Description
This PR adds menu items and "standard keybindings" for Zoom in, Zoom out, and Reset Zoom to the View menu.
The keybindings are Qt defaults (https://doc.qt.io/qtcreator/creator-keyboard-shortcuts.html#image-viewer-shortcuts), as well as defaults on macOS and Windows (see e.g. Adobe Photoshop or Chrome)
Note: I've made a PR to app-model to add the Reset Zoom standard keybinding (pyapp-kit/app-model#220) so if that is merged I can edit this PR to use that here too!
I added to a test for the new behavior.
Note, this approach does not present these as editable in the napari Preferences. I
These are bound like an OS default would be, e.g. copy, paste, etc. They are editable on the OS level at least on macOS.