Skip to content
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

Bugfix: ensure pan/zoom buttons work, along with spacebar keybinding #5669

Merged
merged 7 commits into from
Apr 4, 2023

Conversation

psobolewskiPhD
Copy link
Member

@psobolewskiPhD psobolewskiPhD commented Mar 25, 2023

Description

This fixes issues with Pan/Zoom buttons (layer controls) not working and the spacebar keybinding to activate pan/zoom mode also not working.
In #4894 there was some refactoring which introduced the bugs, see my investigation here: #5654

This PR fixes the typo, lack of proper layer types, and now moves the hold_for_pan_zoom to be a Viewer keybinding. As a result, this keybinding isn't hard-coded anymore, but is now settable in the Preferences > Shortcuts.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

closes #5654

How has this been tested?

Good question! All tests were passing before this PR, despite the fact that things were broken. So I'm open to suggestions how to improve tests so we catch stuff better.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added the qt Relates to qt label Mar 25, 2023
@codecov
Copy link

codecov bot commented Mar 25, 2023

Codecov Report

Merging #5669 (6eaea7c) into main (8a793b9) will increase coverage by 0.00%.
The diff coverage is 94.11%.

@@           Coverage Diff           @@
##             main    #5669   +/-   ##
=======================================
  Coverage   89.85%   89.86%           
=======================================
  Files         611      609    -2     
  Lines       51669    51673    +4     
=======================================
+ Hits        46428    46434    +6     
+ Misses       5241     5239    -2     
Impacted Files Coverage Δ
napari/_qt/layer_controls/qt_shapes_controls.py 88.88% <ø> (ø)
napari/utils/shortcuts.py 100.00% <ø> (ø)
napari/components/_viewer_key_bindings.py 97.33% <81.81%> (-2.67%) ⬇️
...apari/components/_tests/test_viewer_keybindings.py 100.00% <100.00%> (ø)
napari/layers/image/_image_key_bindings.py 78.04% <100.00%> (ø)
napari/layers/points/_points_key_bindings.py 100.00% <100.00%> (ø)
napari/layers/shapes/_shapes_key_bindings.py 98.86% <100.00%> (ø)
napari/layers/surface/_surface_key_bindings.py 93.33% <100.00%> (ø)
napari/layers/tracks/_tracks_key_bindings.py 93.33% <100.00%> (ø)
napari/layers/vectors/_vectors_key_bindings.py 93.33% <100.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Czaki
Copy link
Collaborator

Czaki commented Mar 30, 2023

Could we move pan/zoom from layers to viewer with action like:

def pan_zoom(layerlist: LayerLis):
    selection = layerlist.selection
	if len(selection) != 1:
       yield
       return
    prev = selection[0].mode 
    try:
        selection[0].mode = PanZoom
    finally:
        selection[0].mode = prev

This is dummy code written form memory and may not work.

Such action will allow having one global shortcut for all layers.

@github-actions github-actions bot added the tests Something related to our tests label Mar 30, 2023
@psobolewskiPhD
Copy link
Member Author

Ok, I moved it over to napari/components/_viewer_key_bindings.py
and moved/adjusted the tests.
It's now editable in the Viewer shortcuts.
Personally, I still don't think this should be editable, but just a napari thing, but I get that I'm coming from a different PoV, so sure, why not.

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Configurability doesn't really hurt here I think :)

@Czaki
Copy link
Collaborator

Czaki commented Mar 31, 2023

Until we do not have a mechanism to prevent overwrite in setting built-in shortcuts, then making it configurable is better.

@brisvag brisvag added ready to merge Last chance for comments! Will be merged in ~24h bugfix PR with bugfix labels Apr 3, 2023
@brisvag brisvag merged commit a91fd42 into napari:main Apr 4, 2023
@brisvag brisvag removed the ready to merge Last chance for comments! Will be merged in ~24h label Apr 4, 2023
kcpevey pushed a commit to kcpevey/napari that referenced this pull request Apr 6, 2023
…apari#5669)

# Description

This fixes issues with Pan/Zoom buttons (layer controls) not working and
the spacebar keybinding to activate pan/zoom mode also not working.
In napari#4894 there was some refactoring
which introduced the bugs, see my investigation here:
napari#5654

This PR fixes the typo, lack of proper layer types, and now moves the
`hold_for_pan_zoom` to be a Viewer keybinding. As a result, this
keybinding isn't hard-coded anymore, but is now settable in the
Preferences > Shortcuts.

## Type of change
<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

# References
closes napari#5654

# How has this been tested?
Good question! All tests were passing before this PR, despite the fact
that things were broken. So I'm open to suggestions how to improve tests
so we catch stuff better.


## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] If I included new strings, I have used `trans.` to make them
localizable.
For more information see our [translations
guide](https://napari.org/developers/translations.html).
@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone Apr 9, 2023
@psobolewskiPhD psobolewskiPhD modified the milestones: 0.5.0, 0.4.18 Apr 30, 2023
@Czaki Czaki mentioned this pull request Jun 7, 2023
Czaki pushed a commit that referenced this pull request Jun 19, 2023
…5669)

This fixes issues with Pan/Zoom buttons (layer controls) not working and
the spacebar keybinding to activate pan/zoom mode also not working.
In #4894 there was some refactoring
which introduced the bugs, see my investigation here:
#5654

This PR fixes the typo, lack of proper layer types, and now moves the
`hold_for_pan_zoom` to be a Viewer keybinding. As a result, this
keybinding isn't hard-coded anymore, but is now settable in the
Preferences > Shortcuts.

<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

closes #5654

Good question! All tests were passing before this PR, despite the fact
that things were broken. So I'm open to suggestions how to improve tests
so we catch stuff better.

- [x] My PR is the minimum possible work for the desired functionality
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] If I included new strings, I have used `trans.` to make them
localizable.
For more information see our [translations
guide](https://napari.org/developers/translations.html).
Czaki pushed a commit that referenced this pull request Jun 21, 2023
…5669)

This fixes issues with Pan/Zoom buttons (layer controls) not working and
the spacebar keybinding to activate pan/zoom mode also not working.
In #4894 there was some refactoring
which introduced the bugs, see my investigation here:
#5654

This PR fixes the typo, lack of proper layer types, and now moves the
`hold_for_pan_zoom` to be a Viewer keybinding. As a result, this
keybinding isn't hard-coded anymore, but is now settable in the
Preferences > Shortcuts.

<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

closes #5654

Good question! All tests were passing before this PR, despite the fact
that things were broken. So I'm open to suggestions how to improve tests
so we catch stuff better.

- [x] My PR is the minimum possible work for the desired functionality
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] If I included new strings, I have used `trans.` to make them
localizable.
For more information see our [translations
guide](https://napari.org/developers/translations.html).
Czaki pushed a commit that referenced this pull request Jun 21, 2023
…5669)

This fixes issues with Pan/Zoom buttons (layer controls) not working and
the spacebar keybinding to activate pan/zoom mode also not working.
In #4894 there was some refactoring
which introduced the bugs, see my investigation here:
#5654

This PR fixes the typo, lack of proper layer types, and now moves the
`hold_for_pan_zoom` to be a Viewer keybinding. As a result, this
keybinding isn't hard-coded anymore, but is now settable in the
Preferences > Shortcuts.

<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

closes #5654

Good question! All tests were passing before this PR, despite the fact
that things were broken. So I'm open to suggestions how to improve tests
so we catch stuff better.

- [x] My PR is the minimum possible work for the desired functionality
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] If I included new strings, I have used `trans.` to make them
localizable.
For more information see our [translations
guide](https://napari.org/developers/translations.html).
Czaki pushed a commit that referenced this pull request Jun 21, 2023
…5669)

This fixes issues with Pan/Zoom buttons (layer controls) not working and
the spacebar keybinding to activate pan/zoom mode also not working.
In #4894 there was some refactoring
which introduced the bugs, see my investigation here:
#5654

This PR fixes the typo, lack of proper layer types, and now moves the
`hold_for_pan_zoom` to be a Viewer keybinding. As a result, this
keybinding isn't hard-coded anymore, but is now settable in the
Preferences > Shortcuts.

<!-- Please delete options that are not relevant. -->
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

closes #5654

Good question! All tests were passing before this PR, despite the fact
that things were broken. So I'm open to suggestions how to improve tests
so we catch stuff better.

- [x] My PR is the minimum possible work for the desired functionality
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] If I included new strings, I have used `trans.` to make them
localizable.
For more information see our [translations
guide](https://napari.org/developers/translations.html).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix qt Relates to qt tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Main: KeyError: 'napari:activate_shape_pan_zoom_mode' plus space-bar keybind doesn't work
3 participants