Added custom override of hotkeys based on zuliprc file #1446

jamesdchu · 2023-12-05T19:35:00Z

What does this PR do, and why?

This PR edits the keys.py file to enable custom hotkey mapping. This means that users can change the zuliprc file to get their desired hotkey mappings.

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes #
[x ] Partially fixes issue Enable remapping hotkeys #1439
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
[x ] Adding automated tests for new behavior (or missing tests)
[ x] Existing automated tests should already cover this (only a refactor of tested code)

neiljp · 2023-12-08T22:48:53Z

@jamesdchu This looks like you're making progress towards this - are you looking for feedback yet?

neiljp

@jamesdchu Thanks for working on this 👍 I left the question re if you were asking for feedback in December, but only recently noticed you'd changed it from a draft to a regular PR before my comment, so here's some feedback in case you had intended that to be the case already :)

You currently have this as a lot of commits, so I've not left comments on individual commits, but rather the total set of changes. See the contributing notes in the README regarding how Zulip in general, and this repo in particular, structure commits and why. This is one reason why the PR is failing the 'isolated commits' phase of the automatic tests (CI).

I gave this a manual test and after adjusting a line in run.py I could get it to run OK, as per one comment inline. That comment discusses adding tests for that run.py part, which suggests one way to separate into two parts (commits) - implementing the functionality, and then integrating it into run.py, each commit with tests (and passing linting). The documentation part could also be separate, particularly since a FAQ entry could be useful too. Beyond that, you could also break the functionality part into stages, with tests added/adjusted as the functionality improves, but we can discuss that later if you're interested :)

neiljp · 2024-01-09T02:41:23Z

README.md

+## Custom Keybindings: keybindings can be customized for user preferences (see elsewhere for default)
+custom_hotkeys=GO_UP:G, GO_DOWN:B


It may be clearer here to have a second line ## line indicating specifically where to find the defaults, and also the symbols themselves.

We likely also want the custom_hotkeys line to be commented out by default, since this represents a departure from the default configuration - based on the introduction to this section.

neiljp · 2024-01-09T02:45:45Z

zulipterminal/cli/run.py

@@ -554,6 +555,15 @@ def print_setting(setting: str, data: SettingData, suffix: str = "") -> None:
        print_setting("color depth setting", zterm["color-depth"])
        print_setting("notify setting", zterm["notify"])

+        if "custom_keybindings" in zterm:


Can we have this section earlier? The loading (and validation) of config is generally above this point.

neiljp · 2024-01-09T02:54:08Z

tests/config/test_keys.py

@@ -112,3 +112,83 @@ def test_updated_urwid_command_map() -> None:
            assert key in keys.keys_for_command(zt_cmd)
        except KeyError:
            pass
+
+
+def test_override_keybindings_valid(mocker: MockerFixture) -> None:


Nit: I'd suggest using a double underscore to separate the function name from the variation you're testing. We possibly don't do this everywhere yet, but it makes it easier to distinguish the two parts :)

neiljp · 2024-01-09T02:57:39Z

zulipterminal/cli/run.py

+            custom_keybindings_str = zterm["custom_keybindings"].value
+            _, key_value_pairs = custom_keybindings_str.split("=")


This doesn't work correctly - maybe you had an older version here?

I tested manually by working around this.

You have tests for the function that generates the overrides, but not for this stage. We have a test file for this, so it would be useful to see some for this new code too.

neiljp · 2024-01-09T03:11:08Z

zulipterminal/cli/run.py

+            # Split each pair and convert to a dictionary
+            custom_keybindings = dict(
+                pair.split(":") for pair in key_value_pairs.split(", ")
+            )


This is a good first version, but it'd be useful to see a little error handling (or more flexibility) here, since this crashes if the configuration is laid out incorrectly - such as being ,-separated, instead of with a space too.

I would understand more if multiple keys could be provided, since you might be separating those without a space, but that seems not to be available at this stage? Of course it's not a problem to skip multiple keys with an initial version :)

neiljp · 2024-01-09T03:30:53Z

tests/config/test_keys.py

+
+    # Mock the KEY_BINDINGS to use the test copy
+    mocker.patch.object(keys, "KEY_BINDINGS", test_key_bindings)
+
+    # Now this will modify the test copy, not the original
+    keys.override_keybindings(custom_keybindings, test_key_bindings)


Is this mocking necessary?

neiljp · 2024-01-09T03:32:47Z

tests/config/test_keys.py

+    test_key_bindings: Dict[str, keys.KeyBinding] = {
+        "GO_UP": {
+            "keys": ["up", "k"],
+            "help_text": "Go up / Previous message",
+            "key_category": "navigation",
+        },
+        "GO_DOWN": {
+            "keys": ["down", "j"],
+            "help_text": "Go down / Next message",
+            "key_category": "navigation",
+        },
+    }


It seems like the same starting dict is used in all these tests and copy/pasted. Putting that into a fixture local to this file would reduce the duplication.

neiljp · 2024-01-09T03:37:28Z

zulipterminal/config/keys.py

+def override_keybindings(
+    custom_keybindings: Dict[str, str], existing_keybindings: Dict[str, KeyBinding]


This might read clearer with the existing_keybindings first, and the custom ones named slightly differently - they are a different type.

neiljp · 2024-01-09T06:58:57Z

zulipterminal/config/keys.py

+    # Resolve direct swaps
+    for command, new_key in custom_keybindings.items():
+        if command in conflicts:


Tracking possible conflicts is a good idea, and a direct swap should likely always avoid a problem.

However, the problem is more complex. Note that we already have apparent conflicts depending on different scopes. Our lint-hotkeys script works around some, but there are others (eg. i). These are not a problem in practice due to scoping, but this is not sufficiently encoded in the keybindings data right now.

neiljp · 2024-01-09T07:24:19Z

zulipterminal/config/keys.py

+    # Collect requested changes and detect conflicts
+    for command, new_key in custom_keybindings.items():
+        if command not in existing_keybindings:
+            raise InvalidCommand(f"Invalid command {command} in custom keybindings")


Errors from this method should be handled and cleanly expressed to the user in run.py, not just left to escape unhandled.

zulipbot · 2024-04-17T23:34:41Z

Heads up @jamesdchu, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

jamesdchu added 3 commits December 5, 2023 14:32

added over ride hotkeys functionality and unit tests

bedbbc8

clean up

87f99af

documentation

6828a6a

zulipbot added the size: L [Automatic label added by zulipbot] label Dec 5, 2023

jamesdchu added 5 commits December 5, 2023 16:08

fix linting

30ce997

fix linting

3392608

fix linting

ea8707e

delete tests test

30fde09

fix l;int

7e41155

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Dec 5, 2023

jamesdchu added 5 commits December 5, 2023 16:45

fix linting

a2a9f91

linting again

0b136f2

linting again

a045e40

test

a7ad79b

test

728af79

jamesdchu marked this pull request as draft December 5, 2023 22:37

jamesdchu marked this pull request as ready for review December 5, 2023 22:37

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jan 4, 2024

neiljp reviewed Jan 9, 2024

View reviewed changes

zulipbot added the has conflicts label Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added custom override of hotkeys based on zuliprc file #1446

Added custom override of hotkeys based on zuliprc file #1446

jamesdchu commented Dec 5, 2023

neiljp commented Dec 8, 2023

neiljp left a comment

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

neiljp Jan 9, 2024

zulipbot commented Apr 17, 2024

		## Custom Keybindings: keybindings can be customized for user preferences (see elsewhere for default)
		custom_hotkeys=GO_UP:G, GO_DOWN:B

		custom_keybindings_str = zterm["custom_keybindings"].value
		_, key_value_pairs = custom_keybindings_str.split("=")

		def override_keybindings(
		custom_keybindings: Dict[str, str], existing_keybindings: Dict[str, KeyBinding]

Added custom override of hotkeys based on zuliprc file #1446

Are you sure you want to change the base?

Added custom override of hotkeys based on zuliprc file #1446

Conversation

jamesdchu commented Dec 5, 2023

What does this PR do, and why?

External discussion & connections

How did you test this?

neiljp commented Dec 8, 2023

neiljp 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

zulipbot commented Apr 17, 2024