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: Add the missed keyReleaseEvent method in QtViewerDockWidget #5746

Merged
merged 1 commit into from
Apr 29, 2023

Conversation

ksofiyuk
Copy link
Contributor

@ksofiyuk ksofiyuk commented Apr 19, 2023

Description

I noticed that callbacks do not get key release events when the focus is not on the canvas but they continue to receive key press events. You can easily reproduce the issue by running examples/custom_key_bindings.py and then clicking, for example, on the layer in the layer list, the hello callback (on w key) no longer receives key release events after that.

The issue is cause by the missed keyReleaseEvent in QtViewerDockWidget.

References

Type of change

  • Bugfix (non-breaking change which fixes an issue)

How has this been tested?

  • I tested it manually

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 Apr 19, 2023
@psobolewskiPhD psobolewskiPhD added the bugfix PR with bugfix label Apr 19, 2023
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #5746 (0ffcddc) into main (95356e4) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #5746   +/-   ##
=======================================
  Coverage   89.83%   89.84%           
=======================================
  Files         608      608           
  Lines       51881    51883    +2     
=======================================
+ Hits        46608    46613    +5     
+ Misses       5273     5270    -3     
Impacted Files Coverage Δ
napari/_qt/widgets/qt_viewer_dock_widget.py 96.06% <50.00%> (-0.53%) ⬇️

... 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.

@alisterburt alisterburt self-requested a review April 22, 2023 10:27
@alisterburt alisterburt added this to the 0.4.18 milestone Apr 22, 2023
Copy link
Contributor

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

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

@ksofiyuk thanks for digging here - I have often wanted this to work but there is some hidden complexity here so I'd like opinions from some others before merge

from skimage import data
import napari

blobs = data.binary_blobs(
    length=128, blob_size_fraction=0.05, n_dim=2, volume_fraction=0.25
).astype(float)

viewer = napari.view_image(blobs, name='blobs')
viewer.add_points([64, 64, 64], size=20)

@viewer.bind_key('Up')
def reject_image(viewer):
    msg = 'this is a bad image'
    viewer.status = msg

@napari.Viewer.bind_key('w')
def hello(viewer):
    viewer.status = 'hello world!'
    yield
    viewer.status = 'goodbye world :('
napari.run()

When the layer list is selected it can be navigated using the Up/Down arrow keys - I think this is default Qt behaviour. In this example, our callback is still run when we press the Up key even if we are trying to navigate the layer list

I also tested the callback attached to 'w' and am happy to report than when editing text fields like the layer name and pressing w this callback is not called so this problem is not everywhere and might be limited to Up/Down on list widgets

Questions:

  • do we think only Up/Down are affected by this?
  • if the above is true, is there an easy way to blacklist callbacks attached to just those keys?

@ksofiyuk
Copy link
Contributor Author

ksofiyuk commented Apr 22, 2023

In this example, our callback is still run when we press the Up key even if we are trying to navigate the layer list

@alisterburt Thank you for being mindful about possible issues. However, this PR doesn't actually affect it because it simply adds the omitted the key release event; the key press event is already there (for years since 2019 judging by the commits history) and the described behavior with the Up key is happening with the current main branch.

All the possible pitfalls that you mentioned should already be present as most callbacks only care about the key press event.

Copy link
Contributor

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

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

interesting - I'm now trying to work out what exactly hasn't been working for me when the canvas is not in focus 😆 thank you for clarifying, in this case I am happy for this to be merged as is

@alisterburt alisterburt requested a review from Czaki April 22, 2023 16:50
@alisterburt alisterburt merged commit 9fb9fbe into napari:main Apr 29, 2023
@Czaki Czaki mentioned this pull request Jun 7, 2023
Czaki pushed a commit that referenced this pull request Jun 19, 2023
…5746)

# Description
I noticed that callbacks do not get key release events when the focus is
not on the canvas but they continue to receive key press events. You can
easily reproduce the issue by running `examples/custom_key_bindings.py`
and then clicking, for example, on the layer in the layer list, the
`hello` callback (on `w` key) no longer receives key release events
after that.

The issue is cause by the missed `keyReleaseEvent` in
`QtViewerDockWidget`.

# References
<!-- What resources, documentation, and guides were used in the creation
of this PR? -->

## Type of change
- [X] Bugfix (non-breaking change which fixes an issue)

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [X] I tested it manually

## Final checklist:
- [X] My PR is the minimum possible work for the desired functionality
- [X] 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
…5746)

# Description
I noticed that callbacks do not get key release events when the focus is
not on the canvas but they continue to receive key press events. You can
easily reproduce the issue by running `examples/custom_key_bindings.py`
and then clicking, for example, on the layer in the layer list, the
`hello` callback (on `w` key) no longer receives key release events
after that.

The issue is cause by the missed `keyReleaseEvent` in
`QtViewerDockWidget`.

# References
<!-- What resources, documentation, and guides were used in the creation
of this PR? -->

## Type of change
- [X] Bugfix (non-breaking change which fixes an issue)

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [X] I tested it manually

## Final checklist:
- [X] My PR is the minimum possible work for the desired functionality
- [X] 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
…5746)

# Description
I noticed that callbacks do not get key release events when the focus is
not on the canvas but they continue to receive key press events. You can
easily reproduce the issue by running `examples/custom_key_bindings.py`
and then clicking, for example, on the layer in the layer list, the
`hello` callback (on `w` key) no longer receives key release events
after that.

The issue is cause by the missed `keyReleaseEvent` in
`QtViewerDockWidget`.

# References
<!-- What resources, documentation, and guides were used in the creation
of this PR? -->

## Type of change
- [X] Bugfix (non-breaking change which fixes an issue)

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [X] I tested it manually

## Final checklist:
- [X] My PR is the minimum possible work for the desired functionality
- [X] 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
…5746)

# Description
I noticed that callbacks do not get key release events when the focus is
not on the canvas but they continue to receive key press events. You can
easily reproduce the issue by running `examples/custom_key_bindings.py`
and then clicking, for example, on the layer in the layer list, the
`hello` callback (on `w` key) no longer receives key release events
after that.

The issue is cause by the missed `keyReleaseEvent` in
`QtViewerDockWidget`.

# References
<!-- What resources, documentation, and guides were used in the creation
of this PR? -->

## Type of change
- [X] Bugfix (non-breaking change which fixes an issue)

# How has this been tested?
<!-- Please describe the tests that you ran to verify your changes. -->
- [X] I tested it manually

## Final checklist:
- [X] My PR is the minimum possible work for the desired functionality
- [X] 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).
@ksofiyuk ksofiyuk deleted the fix_missed_press_release branch January 8, 2024 10:27
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants