feat: Update of shortcuts on menu rebuild #258
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
=======================================
Coverage 99.68% 99.68%
=======================================
Files 31 31
Lines 1877 1895 +18
=======================================
+ Hits 1871 1889 +18
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@tlambert03 pyside2 failures are related to the drop of pyside2 support by pytest-qt. |
can you pick whatever method you want then to fix it? either drop the napari tests, or fix it some other way |
|
Half is fixed in #259. For reusable workflows, I think that I may made similar PR to napari itself. |
# References and relevant issues pyapp-kit/app-model#258 (comment) followup #8067 # Description The `pytest-qt` in version 4.5.0 dropped the support for PySide2. In #8067 I've fixed it on our side, but tests of critical for us pyapp-kit projects are broken. This PR fixes their test that uses https://github.com/pyapp-kit/workflows/blob/main/.github/workflows/test-dependents.yml
| "test.update.keybinding.tooltip", | ||
| KeyBindingRule(primary=KeyCode.KeyL, source=KeyBindingSource.USER), | ||
| ) | ||
| q_action._update_keybinding() |
There was a problem hiding this comment.
I'm a little surprised to see this private method called directly in tests.
Whose job is it ultimately to be calling _update_keybinding()?
There was a problem hiding this comment.
Currently it is called on a rebuild of the menu. app model is not evented. I do not yet identified other places.
But maybe it should be a public method?
| if kb := self._app.keybindings.get_keybinding(self._command_id): | ||
| self.setShortcut(QKeyBindingSequence(kb.keybinding)) | ||
| self._keybinding_tooltip = f"({kb.keybinding.to_text()})" |
There was a problem hiding this comment.
this code is more or less the same as what's happening in the init at line 52. Can we maybe avoid that duplication by having this be the one place where that logic is held, and call it in init?
There was a problem hiding this comment.
The problem is with tooltip. If we call this method from line 52, then, for QCommandRuleAction, the self._tooltip will be undefined on call of _update_keybinding
I think that we may fix this using metaclass and add __post_init__ where _update_keybinding will be called.
Co-authored-by: Talley Lambert <[email protected]>
Co-authored-by: Talley Lambert <[email protected]>
|
Should I elaborate more in some place? |
|
nah, just fell off the radar. i'll have another look tomorrow |
|
ok for a release @Czaki? |
|
yes |
2 participants
This PR adds updating of shortcuts on menu rebuild.
This is required for full migration of the napari into app model