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

feat: add action keybinding info over tooltip #218

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

dalthviz
Copy link
Contributor

@dalthviz dalthviz commented Aug 14, 2024

Closes #216
Related to napari/napari#7133

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b7cd0d5) to head (3447e25).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #218   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         1871      1875    +4     
=========================================
+ Hits          1871      1875    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -54,6 +56,11 @@ def _on_triggered(self, checked: bool) -> None:
# to raise any exceptions.
self._app.commands.execute_command(self._command_id).result()

def setToolTip(self, tooltip: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

with this pattern, is there any way for someone to opt out of this behavior? For example, if they wish to set their own tooltip (sans suffix). What do you think about simply calling self.setToolTip with the text and the shortcut suffix in __init__ (and then later calls to setToolTip() would override it). ... or some other method that makes it opt-out-able

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, maybe I overdid a little bit here trying to implement a way to always show the keybinding 😅 Will simplify things then 👍

Also, regardless of the simplification. could it make sense to add over the Action definition a field like the one for CommandRule definition for the icon visibility?

icon_visible_in_menu: bool = Field(
True,
description="Whether to show the icon in menus (for backends that support it). "
"If `False`, only the title will be shown. By default, `True`.",
)

So something like keybinding_visible_in_tooltip and make its default value False?

Copy link
Member

Choose a reason for hiding this comment

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

i think i'd prefer not to have that sort of "view-specific" setting in the model itself.

…ge of single key as key combination with Qt6
@dalthviz dalthviz marked this pull request as ready for review August 15, 2024 16:04
@DragaDoncila
Copy link

@tlambert03 I think your comments have been addressed if you wouldn't mind taking another look at this PR?

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

thanks!

@tlambert03 tlambert03 merged commit 694d9d6 into pyapp-kit:main Sep 11, 2024
36 checks passed
@tlambert03 tlambert03 added the enhancement New feature or request label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

feat: include action current keybinding as part of its tooltip
3 participants