feat: add a way to get a user-facing string representation of keybindings #211

dalthviz · 2024-07-10T18:53:19Z

Hi there, this is an initial attempt to add a way to get a user-facing string representation of keybindings (with optional key symbols and taking into account OS differences) by porting the logic already implemented over napari (napari/napari#5064 and napari/napari#5604).

This is still a work in progress but any early feedback is appreciated!

Closes #89

…th optional key symbols and taking into account OS/platform differences)

codecov · 2024-07-10T18:58:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0e801a9) to head (58b9a23).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #211   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         1814      1851   +37     
=========================================
+ Hits          1814      1851   +37

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

tlambert03

thanks @dalthviz, this would be a nice addition. couple thoughts about where this should live, before we consider the final api

tlambert03 · 2024-07-10T20:41:17Z

src/app_model/types/_keys/_keybindings.py

@@ -19,6 +19,58 @@
 _re_cmd = re.compile(r"cmd[\+|\-]")


+def get_keys_user_facing_representation(


probably don't need a function to re-define these dicts every time a string is generated, since they won't change at all at runtime, the only thing is which dict to pull from... so maybe a module level:

WIN_KEY_SYMBOLS: dict[str, str] = {...} MACOS_KEY_SYMBOLS: dict[str, str] = {**WIN_SYMBOLS, "Ctrl": "⌃", "Alt": "⌥", "Meta": "⌘"} LINUX_KEY_SYMBOLS: dict[str, str] = {**WIN_SYMBOLS, "Meta": "Super"} # same for names

and then wherever you would have previously used this function, just pick a dict

(however, see later comment)

tlambert03 · 2024-07-10T20:43:41Z

src/app_model/types/_keys/_keybindings.py

+        Optionally, the string representation can be constructed with symbols
+        like ↵ for Enter or OS specific ones like ⌘ for Cmd on MacOS.
+        """
+        joinchar, key_symbols, key_names = get_keys_user_facing_representation(os=os)


i think @jni wants more control over the joinchar (see #210) so joinchar should be a parameter with a default fallback if not provided

tlambert03 · 2024-07-10T20:57:03Z

src/app_model/types/_keys/_keybindings.py

+        """
+        joinchar, key_symbols, key_names = get_keys_user_facing_representation(os=os)
+        return joinchar.join(
+            key_symbols.get(x, x) if use_symbols else key_names.get(x, x)


i wonder if much of this PR (including the look into the mapping here with .get(x, x)) should actually be a method on KeyCode. Currently, this method is first casting self.key (which is a KeyCode) to a string, and then using that string to again index into another map. What if did that more directly:

class KeyCode(IntEnum): ... def to_os_symbol(self, os: Optional[OperatingSystem]) -> str: ... def to_os_name(self, os: Optional[OperatingSystem]) -> str: ...

and then put these lookups at a lower level in _key_codes.py?

that would also have the benefit of being able to directly get these symbols at a lower level, given something like KeyCode.Meta ... without first having to put it into a keybinding object

Oh I see, I think that makes sense, will give it a try 👍 also thank you so much for the early feedback!

the layout of _key_codes.py is a little dense at first glance. so let me know if you run into any confusion

Thank you! And actually, checking _key_codes.py, I'm wondering if it could make sense to add the symbols and names mappings to the logic over _build_maps along side the addition there of some new functions like:

def _keycode_to_os_symbol(keystr: str, os: OperatingSystem) -> str:... def _keycode_to_os_name(keystr: str, os: OperatingSystem) -> str:...

Seems like that could be a good place 🤔 What do you think?

yeah i agree, _build_maps is the right place to construct these lookups.

Give that a check and I think the work at #213 partialy captures the parse_platform logic at https://github.com/napari/napari/blob/main/napari/utils/interactions.py#L312 The missing thing I would say is also being able to parse symbols from non modifier keys like enter, arrows, backspace, delete, tab, space, etc. Could it be worthy to further extend the parsing capabilities to any symbol or representation that os_symbol and os_name can return? From a quick check, maybe extending the KEYCODE_FROM_LOWERCASE_STRING dictionary could be a way to achieve this? So something like:

KEYCODE_FROM_LOWERCASE_STRING: Dict[str, KeyCode] = { # two special cases for assigning os-specific strings to the meta key 'win': KeyCode.Meta, 'cmd': KeyCode.Meta, # special cases for symbols and other os-specific strings '↵': KeyCode.Enter, '→': KeyCode.RightArrow, ... }

ah I see... yeah, perhaps. Lets discuss that in another issue though. Seems like it might be a cool but YAGNI feature? do you actively parse strings with key symbols in napari? or was it more of a "why not allow this" sort of feature?

Created #214 👍 and I think that was added to be able to store and comper shortcuts in a canonical/normalized form from a representation of a keybinding like the one you would get from the GUI (like for example the napari shortcuts widget) 🤔 So, to support logic like the following:

https://github.com/napari/napari/blob/5e685a20c9563a42576ead39a4f0286337a52ce6/napari/_qt/widgets/qt_keyboard_settings.py#L403-L428

Ok, plenty to discuss there. I would definitely keep things as KeyCodes and not strings as much as possible. So, rather than using current.text() in that widget, you should take advantage of itemData(), or somehow have the visual representation of the keybinding in the widget not be the actual data that napari uses in the model. (Think of this as a classic model view thing, where the model is the KeyCode, and the view is some string that the user sees)

Yep, that makes sense to me 👍 I think I will mark this as ready for review then. Let me know if further changes are needed here!

…m and add more tests

tlambert03

getting close! i made some refactoring suggestions, and some other minor requests

tlambert03 · 2024-07-12T19:58:23Z

src/app_model/types/_keys/_key_codes.py

@@ -152,6 +156,14 @@ class KeyCode(IntEnum):
    def __str__(self) -> str:
        return keycode_to_string(self)

+    def to_os_symbol(self, os: Optional[OperatingSystem] = None) -> str:


i'm waffling on this name... what about just os_symbol()? and os_name()? preference? or who cares :) ?

also please add docstrings to these two public methods, with a brief clarification on how to_name differs from __str__

src/app_model/types/_keys/_key_codes.py

src/app_model/types/_keys/_keybindings.py

Co-authored-by: Talley Lambert <[email protected]>

… issue_89

tlambert03

thank you @dalthviz, lovely

…tcut` logic (#7113) # References and relevant issues Part of #7059 # Description * Use the work done at pyapp-kit/app-model#211 * Bump app-model constraint to >=0.2.8 * Alternative to PR #7093 (without doing any major refactor to the shortcut widget logic)

dalthviz and others added 3 commits July 10, 2024 13:50

Add a way to get a user facing text representation of keybindings (wi…

87a88fc

…th optional key symbols and taking into account OS/platform differences)

style: [pre-commit.ci] auto fixes [...]

25a443e

Merge branch 'main' into issue_89

e315d1e

dalthviz changed the title ~~[WIP] Add a way to get a user-facing string representation of keybindings~~ [WIP] feat: add a way to get a user-facing string representation of keybindings Jul 10, 2024

dalthviz changed the title ~~[WIP] feat: add a way to get a user-facing string representation of keybindings~~ feat: add a way to get a user-facing string representation of keybindings Jul 10, 2024

dalthviz added 3 commits July 10, 2024 13:59

Typing

37dbf0a

More typing

af50347

Add 'Alt' key usage over test

845fb94

tlambert03 reviewed Jul 10, 2024

View reviewed changes

tlambert03 mentioned this pull request Jul 11, 2024

Consider changing the default keybinding separator on printout to - #210

Open

dalthviz added 2 commits July 12, 2024 10:57

Move mappings and logic to '_key_codes.py', introduce 'joinchar' para…

a9cd05c

…m and add more tests

Merge branch 'main' into issue_89

e0b94ec

dalthviz mentioned this pull request Jul 12, 2024

[WIP] Changes for Shortcut logic migration to app-model napari/napari#7093

Closed

tlambert03 reviewed Jul 12, 2024

View reviewed changes

dalthviz and others added 3 commits July 15, 2024 13:25

Apply suggestions from code review

5160031

Co-authored-by: Talley Lambert <[email protected]>

Update method names and add/improve methods docstrings

eb96af5

Merge branch 'issue_89' of https://github.com/dalthviz/app-model into…

58b9a23

… issue_89

dalthviz mentioned this pull request Jul 17, 2024

Enable parsing symbols and os-specific string representations of keys (besides modifiers) #214

Open

dalthviz marked this pull request as ready for review July 18, 2024 16:39

dalthviz closed this Jul 18, 2024

dalthviz reopened this Jul 18, 2024

tlambert03 reviewed Jul 19, 2024

View reviewed changes

tlambert03 merged commit f1e911b into pyapp-kit:main Jul 19, 2024
66 of 67 checks passed

tlambert03 added the enhancement New feature or request label Jul 19, 2024

dalthviz mentioned this pull request Jul 19, 2024

Use app-model KeyBinding.to_text and KeyCode.os_symbol over Shortcut logic napari/napari#7113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a way to get a user-facing string representation of keybindings #211

feat: add a way to get a user-facing string representation of keybindings #211

dalthviz commented Jul 10, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

tlambert03 left a comment

tlambert03 Jul 10, 2024

tlambert03 Jul 10, 2024

tlambert03 Jul 10, 2024

tlambert03 Jul 10, 2024

dalthviz Jul 11, 2024

tlambert03 Jul 11, 2024

dalthviz Jul 11, 2024

tlambert03 Jul 11, 2024

dalthviz Jul 17, 2024

tlambert03 Jul 17, 2024

dalthviz Jul 17, 2024

tlambert03 Jul 17, 2024

dalthviz Jul 18, 2024

tlambert03 left a comment

tlambert03 Jul 12, 2024

tlambert03 Jul 12, 2024

tlambert03 left a comment

		@@ -19,6 +19,58 @@
		_re_cmd = re.compile(r"cmd[\+\|\-]")


		def get_keys_user_facing_representation(

feat: add a way to get a user-facing string representation of keybindings #211

feat: add a way to get a user-facing string representation of keybindings #211

Conversation

dalthviz commented Jul 10, 2024

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

tlambert03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlambert03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlambert03 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024 •

edited

Loading