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

Added custom override of hotkeys based on zuliprc file #1446

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,9 @@ notify=disabled

## Color-depth: set to one of 1 (for monochrome), 16, 256, or 24bit
color-depth=256

## Custom Keybindings: keybindings can be customized for user preferences (see elsewhere for default)
custom_hotkeys=GO_UP:G, GO_DOWN:B
Comment on lines +250 to +251
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

```

> **NOTE:** Most of these configuration settings may be specified on the
Expand Down
80 changes: 80 additions & 0 deletions tests/config/test_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

custom_keybindings = {"GO_UP": "y"}
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",
},
}
Comment on lines +119 to +130
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


# 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)
Comment on lines +131 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this mocking necessary?


assert test_key_bindings["GO_UP"]["keys"] == ["y"]


def test_override_keybindings_invalid_command(mocker: MockerFixture) -> None:
custom_keybindings = {"INVALID_COMMAND": "x"}
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",
},
}
with pytest.raises(keys.InvalidCommand):
keys.override_keybindings(custom_keybindings, test_key_bindings)


def test_override_keybindings_conflict(mocker: MockerFixture) -> None:
custom_keybindings = {"GO_UP": "j"} # 'j' is originally for GO_DOWN
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",
},
}
keys.override_keybindings(custom_keybindings, test_key_bindings)
assert "j" in test_key_bindings["GO_DOWN"]["keys"] # unchanged
assert test_key_bindings["GO_UP"]["keys"] != ["j"] # not updated due to conflict


def test_override_multiple_keybindings_valid(mocker: MockerFixture) -> None:
custom_keybindings = {"GO_UP": "y", "GO_DOWN": "b"} # 'y' and 'b' are unused
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",
},
}
keys.override_keybindings(custom_keybindings, test_key_bindings)
assert test_key_bindings["GO_UP"]["keys"] == ["y"]
assert test_key_bindings["GO_DOWN"]["keys"] == ["b"]
10 changes: 10 additions & 0 deletions zulipterminal/cli/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from urwid import display_common, set_encoding

from zulipterminal.api_types import ServerSettings
from zulipterminal.config.keys import KEY_BINDINGS, override_keybindings
from zulipterminal.config.themes import (
ThemeError,
aliased_themes,
Expand Down Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

custom_keybindings_str = zterm["custom_keybindings"].value
_, key_value_pairs = custom_keybindings_str.split("=")
Comment on lines +559 to +560
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

# Split each pair and convert to a dictionary
custom_keybindings = dict(
pair.split(":") for pair in key_value_pairs.split(", ")
)
Comment on lines +561 to +564
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

override_keybindings(custom_keybindings, KEY_BINDINGS)

### Generate data not output to user, but into Controller
# Generate urwid palette
color_depth_str = zterm["color-depth"].value
Expand Down
43 changes: 43 additions & 0 deletions zulipterminal/config/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,49 @@ def commands_for_random_tips() -> List[KeyBinding]:
]


def override_keybindings(
custom_keybindings: Dict[str, str], existing_keybindings: Dict[str, KeyBinding]
Comment on lines +474 to +475
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

) -> None:
reverse_key_map = {
key: cmd
for cmd, binding in existing_keybindings.items()
for key in binding["keys"]
}
requested_changes = {}
conflicts = {}

# 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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


current_keys = existing_keybindings[command]["keys"]
if new_key not in current_keys:
requested_changes[command] = new_key
if new_key in reverse_key_map and reverse_key_map[new_key] != command:
conflicting_cmd = reverse_key_map[new_key]
conflicts[command] = conflicting_cmd

# Resolve direct swaps
for command, new_key in custom_keybindings.items():
if command in conflicts:
Comment on lines +497 to +499
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

conflicting_cmd = conflicts[command]
if (
conflicting_cmd in custom_keybindings
and custom_keybindings[conflicting_cmd] in current_keys
):
del conflicts[command]
del conflicts[conflicting_cmd]

if conflicts:
# Handle unresolved conflicts, e.g., by warning the user
return

# Apply changes
for command, new_key in requested_changes.items():
existing_keybindings[command]["keys"] = [new_key]


# Refer urwid/command_map.py
# Adds alternate keys for standard urwid navigational commands.
for zt_cmd, urwid_cmd in ZT_TO_URWID_CMD_MAPPING.items():
Expand Down