Repository: zulip/zulip-terminal
Pull request #1476 (2024-03-12 15:38:34)

Hotkeys/display
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?
Adds support to control and format the display of hotkey combinations to the user. \
To allow using different types of hotkey displays in different parts of the UI.

- Capitalizes special keys
- Formats 'page up' and 'page down' to 'PgUp' and 'PgDown' respectively

### External discussion & connections
- [ ] Discussed in **#zulip-terminal** in `topic`
- [ ] Fully fixes #
- [x] Partially fixes issue #1327 
- [x] Builds upon previous unmerged work in PR #1354 
- [ ] 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
- [x] Manually - Visual changes
- [x] Adapting existing automated tests
- [x] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (*only a refactor of tested code*)

### Visual changes    
![help_menu](https://github.com/zulip/zulip-terminal/assets/20315308/eedcea99-a584-4266-a486-c1285cce86e0)
![panel](https://github.com/zulip/zulip-terminal/assets/20315308/b8488148-75fc-435d-8a9b-83fd0228cc54)



Niloth-p: @zulipbot add "area: hotkeys" "PR needs review" "feedback wanted" "enhancement"
zulipbot: Hello @zulip/server-hotkeys members, this pull request was labeled with the "area: hotkeys" label, so you may want to check it out!

<!-- areaLabelAddition -->

Niloth-p: @zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"
Niloth-p: @zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"
neiljp: @Niloth-p Thanks for the update - I just pushed back with minor commit text updates, and will merge shortly :tada:

The test ids you've added are fine as they are, though if the parametrize data and the ids run too long then it's generally good to include them inline instead so it's clear how they connect and that they don't get out of sync.

Other than the elements that we've already discussed for following up on this:
- Your new test array is coupled to the current keymap, so changes of that will impact tests and vice versa. This occurs for other tests in this file, so a commit with refactor(s) to tidy that up using a common approach similar to the random tips would be useful.

Patch:
diff --git a/zulipterminal/config/keys.py b/zulipterminal/config/keys.py
index f6f6c09..9e2ace9 100644
--- a/zulipterminal/config/keys.py
+++ b/zulipterminal/config/keys.py
@@ -460,6 +460,44 @@ def primary_key_for_command(command: str) -> str:
     return keys_for_command(command).pop(0)
 
 
+URWID_KEY_TO_DISPLAY_KEY_MAPPING = {
+    "page up": "PgUp",
+    "page down": "PgDn",
+}
+
+
+def display_key_for_urwid_key(urwid_key: str) -> str:
+    """
+    Returns a displayable user-centric format of the urwid key.
+    """
+    for urwid_map_key, display_map_key in URWID_KEY_TO_DISPLAY_KEY_MAPPING.items():
+        if urwid_map_key in urwid_key:
+            urwid_key = urwid_key.replace(urwid_map_key, display_map_key)
+    display_key = [
+        keyboard_key.capitalize()
+        if len(keyboard_key) > 1 and keyboard_key[0].islower()
+        else keyboard_key
+        for keyboard_key in urwid_key.split()
+    ]
+    return " ".join(display_key)
+
+
+def display_keys_for_command(command: str) -> List[str]:
+    """
+    Returns the user-friendly display keys for a given mapped command
+    """
+    return [
+        display_key_for_urwid_key(urwid_key) for urwid_key in keys_for_command(command)
+    ]
+
+
+def primary_display_key_for_command(command: str) -> str:
+    """
+    Primary Display Key is the formatted display version of the primary key
+    """
+    return display_key_for_urwid_key(primary_key_for_command(command))
+
+
 def commands_for_random_tips() -> List[KeyBinding]:
     """
     Return list of commands which may be displayed as a random tip
diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py
index 52e4076..76d0a01 100644
--- a/zulipterminal/helper.py
+++ b/zulipterminal/helper.py
@@ -33,7 +33,7 @@ import requests
 from typing_extensions import Literal, ParamSpec, TypedDict
 
 from zulipterminal.api_types import Composition, EmojiType, Message
-from zulipterminal.config.keys import primary_key_for_command
+from zulipterminal.config.keys import primary_display_key_for_command
 from zulipterminal.config.regexes import (
     REGEX_COLOR_3_DIGIT,
     REGEX_COLOR_6_DIGIT,
@@ -700,7 +700,7 @@ def check_narrow_and_notify(
         and current_narrow != outer_narrow
         and current_narrow != inner_narrow
     ):
-        key = primary_key_for_command("NARROW_MESSAGE_RECIPIENT")
+        key = primary_display_key_for_command("NARROW_MESSAGE_RECIPIENT")
 
         controller.report_success(
             [
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index b7d5287..af0b846 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -56,7 +56,7 @@ from zulipterminal.api_types import (
     UpdateMessageContentEvent,
     UpdateMessagesLocationEvent,
 )
-from zulipterminal.config.keys import primary_key_for_command
+from zulipterminal.config.keys import primary_display_key_for_command
 from zulipterminal.config.symbols import STREAM_TOPIC_SEPARATOR
 from zulipterminal.config.ui_mappings import EDIT_TOPIC_POLICY, ROLE_BY_ID, STATE_ICON
 from zulipterminal.helper import (
@@ -1671,7 +1671,7 @@ class Model:
                 "Press '{}' to close this window."
             )
             notice = notice_template.format(
-                failed_command, primary_key_for_command("GO_BACK")
+                failed_command, primary_display_key_for_command("GO_BACK")
             )
             self.controller.popup_with_message(notice, width=50)
             self.controller.update_screen()
diff --git a/zulipterminal/ui.py b/zulipterminal/ui.py
index 3507f72..b6a5a3d 100644
--- a/zulipterminal/ui.py
+++ b/zulipterminal/ui.py
@@ -9,7 +9,11 @@ from typing import Any, Dict, List, Optional
 
 import urwid
 
-from zulipterminal.config.keys import commands_for_random_tips, is_command_key
+from zulipterminal.config.keys import (
+    commands_for_random_tips,
+    display_key_for_urwid_key,
+    is_command_key,
+)
 from zulipterminal.config.symbols import (
     APPLICATION_TITLE_BAR_LINE,
     AUTOHIDE_TAB_LEFT_ARROW,
@@ -102,10 +106,13 @@ class View(urwid.WidgetWrap):
         if not allowed_commands:
             return ["Help(?): "]
         random_command = random.choice(allowed_commands)
+        random_command_display_keys = ", ".join(
+            [display_key_for_urwid_key(key) for key in random_command["keys"]]
+        )
         return [
             "Help(?): ",
-            ("footer_contrast", " " + ", ".join(random_command["keys"]) + " "),
-            " " + random_command["help_text"],
+            ("footer_contrast", f" {random_command_display_keys} "),
+            f" {random_command['help_text']}",
         ]
 
     @asynch
diff --git a/zulipterminal/ui_tools/boxes.py b/zulipterminal/ui_tools/boxes.py
index 6867d5a..441eaba 100644
--- a/zulipterminal/ui_tools/boxes.py
+++ b/zulipterminal/ui_tools/boxes.py
@@ -15,8 +15,9 @@ from urwid_readline import ReadlineEdit
 
 from zulipterminal.api_types import Composition, PrivateComposition, StreamComposition
 from zulipterminal.config.keys import (
+    display_keys_for_command,
     is_command_key,
-    keys_for_command,
+    primary_display_key_for_command,
     primary_key_for_command,
 )
 from zulipterminal.config.regexes import (
@@ -302,11 +303,11 @@ class WriteBox(urwid.Pile):
             invalid_recipients_error = [
                 "Invalid recipient(s) - " + ", ".join(invalid_recipients),
                 " - Use ",
-                ("footer_contrast", primary_key_for_command("AUTOCOMPLETE")),
+                ("footer_contrast", primary_display_key_for_command("AUTOCOMPLETE")),
                 " or ",
                 (
                     "footer_contrast",
-                    primary_key_for_command("AUTOCOMPLETE_REVERSE"),
+                    primary_display_key_for_command("AUTOCOMPLETE_REVERSE"),
                 ),
                 " to autocomplete.",
             ]
@@ -850,8 +851,10 @@ class WriteBox(urwid.Pile):
                             invalid_stream_error = (
                                 "Invalid stream name."
                                 " Use {} or {} to autocomplete.".format(
-                                    primary_key_for_command("AUTOCOMPLETE"),
-                                    primary_key_for_command("AUTOCOMPLETE_REVERSE"),
+                                    primary_display_key_for_command("AUTOCOMPLETE"),
+                                    primary_display_key_for_command(
+                                        "AUTOCOMPLETE_REVERSE"
+                                    ),
                                 )
                             )
                             self.view.controller.report_error([invalid_stream_error])
@@ -918,7 +921,9 @@ class MessageSearchBox(urwid.Pile):
         super().__init__(self.main_view())
 
     def main_view(self) -> Any:
-        search_text = f"Search [{', '.join(keys_for_command('SEARCH_MESSAGES'))}]: "
+        search_text = (
+            f"Search [{', '.join(display_keys_for_command('SEARCH_MESSAGES'))}]: "
+        )
         self.text_box = ReadlineEdit(f"{search_text} ")
         # Add some text so that when packing,
         # urwid doesn't hide the widget.
@@ -967,7 +972,9 @@ class PanelSearchBox(urwid.Edit):
     ) -> None:
         self.panel_view = panel_view
         self.search_command = search_command
-        self.search_text = f" Search [{', '.join(keys_for_command(search_command))}]: "
+        self.search_text = (
+            f" Search [{', '.join(display_keys_for_command(search_command))}]: "
+        )
         self.search_error = urwid.AttrMap(
             urwid.Text([" ", INVALID_MARKER, " No Results"]), "search_error"
         )
diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py
index 748c86c..9ecb2d3 100644
--- a/zulipterminal/ui_tools/buttons.py
+++ b/zulipterminal/ui_tools/buttons.py
@@ -11,7 +11,11 @@ import urwid
 from typing_extensions import TypedDict
 
 from zulipterminal.api_types import RESOLVED_TOPIC_PREFIX, EditPropagateMode, Message
-from zulipterminal.config.keys import is_command_key, primary_key_for_command
+from zulipterminal.config.keys import (
+    is_command_key,
+    primary_display_key_for_command,
+    primary_key_for_command,
+)
 from zulipterminal.config.regexes import REGEX_INTERNAL_LINK_STREAM_ID
 from zulipterminal.config.symbols import (
     ALL_MESSAGES_MARKER,
@@ -125,7 +129,9 @@ class TopButton(urwid.Button):
 
 class HomeButton(TopButton):
     def __init__(self, *, controller: Any, count: int) -> None:
-        button_text = f"All messages     [{primary_key_for_command('ALL_MESSAGES')}]"
+        button_text = (
+            f"All messages     [{primary_display_key_for_command('ALL_MESSAGES')}]"
+        )
 
         super().__init__(
             controller=controller,
@@ -139,7 +145,7 @@ class HomeButton(TopButton):
 
 class PMButton(TopButton):
     def __init__(self, *, controller: Any, count: int) -> None:
-        button_text = f"Direct messages  [{primary_key_for_command('ALL_PM')}]"
+        button_text = f"Direct messages  [{primary_display_key_for_command('ALL_PM')}]"
 
         super().__init__(
             controller=controller,
@@ -153,7 +159,9 @@ class PMButton(TopButton):
 
 class MentionedButton(TopButton):
     def __init__(self, *, controller: Any, count: int) -> None:
-        button_text = f"Mentions         [{primary_key_for_command('ALL_MENTIONS')}]"
+        button_text = (
+            f"Mentions         [{primary_display_key_for_command('ALL_MENTIONS')}]"
+        )
 
         super().__init__(
             controller=controller,
@@ -167,7 +175,9 @@ class MentionedButton(TopButton):
 
 class StarredButton(TopButton):
     def __init__(self, *, controller: Any, count: int) -> None:
-        button_text = f"Starred messages [{primary_key_for_command('ALL_STARRED')}]"
+        button_text = (
+            f"Starred messages [{primary_display_key_for_command('ALL_STARRED')}]"
+        )
 
         super().__init__(
             controller=controller,
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index 455d6a8..208850d 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -14,8 +14,9 @@ from zulipterminal.api_types import EditPropagateMode, Message
 from zulipterminal.config.keys import (
     HELP_CATEGORIES,
     KEY_BINDINGS,
+    display_key_for_urwid_key,
+    display_keys_for_command,
     is_command_key,
-    keys_for_command,
     primary_key_for_command,
 )
 from zulipterminal.config.markdown_examples import MARKDOWN_ELEMENTS
@@ -1225,9 +1226,14 @@ class HelpView(PopUpView):
                 for binding in KEY_BINDINGS.values()
                 if binding["key_category"] == category
             )
-            key_bindings = []
-            for binding in keys_in_category:
-                key_bindings.append((binding["help_text"], ", ".join(binding["keys"])))
+            key_bindings = [
+                (
+                    binding["help_text"],
+                    ", ".join(map(display_key_for_urwid_key, binding["keys"])),
+                )
+                for binding in keys_in_category
+            ]
+
             help_menu_content.append((HELP_CATEGORIES[category], key_bindings))
 
         popup_width, column_widths = self.calculate_table_widths(
@@ -1379,7 +1385,7 @@ class StreamInfoView(PopUpView):
             if stream["history_public_to_subscribers"]
             else "Not Public to Users"
         )
-        member_keys = ", ".join(map(repr, keys_for_command("STREAM_MEMBERS")))
+        member_keys = ", ".join(map(repr, display_keys_for_command("STREAM_MEMBERS")))
 
         # FIXME: This field was removed from the subscription data in Zulip 7.5 / ZFL226
         #   We should use the new /streams/{stream_id}/email_address endpoint instead
@@ -1387,7 +1393,9 @@ class StreamInfoView(PopUpView):
         if self._stream_email is None:
             stream_copy_text = "< Stream email is unavailable >"
         else:
-            email_keys = ", ".join(map(repr, keys_for_command("COPY_STREAM_EMAIL")))
+            email_keys = ", ".join(
+                map(repr, display_keys_for_command("COPY_STREAM_EMAIL"))
+            )
             stream_copy_text = f"Press {email_keys} to copy Stream email address"
 
         weekly_traffic = stream["stream_weekly_traffic"]
@@ -1562,14 +1570,14 @@ class MsgInfoView(PopUpView):
             msg["timestamp"], show_seconds=True, show_year=True
         )
         view_in_browser_keys = "[{}]".format(
-            ", ".join(map(str, keys_for_command("VIEW_IN_BROWSER")))
+            ", ".join(map(str, display_keys_for_command("VIEW_IN_BROWSER")))
         )
 
         full_rendered_message_keys = "[{}]".format(
-            ", ".join(map(str, keys_for_command("FULL_RENDERED_MESSAGE")))
+            ", ".join(map(str, display_keys_for_command("FULL_RENDERED_MESSAGE")))
         )
         full_raw_message_keys = "[{}]".format(
-            ", ".join(map(str, keys_for_command("FULL_RAW_MESSAGE")))
+            ", ".join(map(str, display_keys_for_command("FULL_RAW_MESSAGE")))
         )
         msg_info = [
             (
@@ -1601,7 +1609,9 @@ class MsgInfoView(PopUpView):
         if self.show_edit_history_label:
             msg_info[0][1][0] = ("Date & Time (Original)", date_and_time)
 
-            keys = "[{}]".format(", ".join(map(str, keys_for_command("EDIT_HISTORY"))))
+            keys = "[{}]".format(
+                ", ".join(map(str, display_keys_for_command("EDIT_HISTORY")))
+            )
             msg_info[1][1].append(("Edit History", keys))
         # Render the category using the existing table methods if links exist.
         if message_links:


Test patch:
diff --git a/tests/config/test_keys.py b/tests/config/test_keys.py
index c080796..752e69a 100644
--- a/tests/config/test_keys.py
+++ b/tests/config/test_keys.py
@@ -1,4 +1,4 @@
-from typing import Any, Dict
+from typing import Any, Dict, List
 
 import pytest
 from pytest_mock import MockerFixture
@@ -112,3 +112,52 @@ def test_updated_urwid_command_map() -> None:
             assert key in keys.keys_for_command(zt_cmd)
         except KeyError:
             pass
+
+
+@pytest.mark.parametrize(
+    "urwid_key, display_key",
+    [
+        ("a", "a"),
+        ("B", "B"),
+        (":", ":"),
+        ("enter", "Enter"),
+        ("meta c", "Meta c"),
+        ("ctrl D", "Ctrl D"),
+        ("page up", "PgUp"),
+        ("ctrl page up", "Ctrl PgUp"),
+    ],
+    ids=[
+        "lowercase_alphabet_key",
+        "uppercase_alphabet_key",
+        "symbol_key",
+        "special_key",
+        "lowercase_alphabet_key_with_modifier_key",
+        "uppercase_alphabet_key_with_modifier_key",
+        "mapped_key",
+        "mapped_key_with_modifier_key",
+    ],
+)
+def test_display_key_for_urwid_key(urwid_key: str, display_key: str) -> None:
+    assert keys.display_key_for_urwid_key(urwid_key) == display_key
+
+
+COMMAND_TO_DISPLAY_KEYS = [
+    ("NEXT_LINE", ["Down", "Ctrl n"]),
+    ("TOGGLE_STAR_STATUS", ["Ctrl s", "*"]),
+    ("ALL_PM", ["P"]),
+]
+
+
+@pytest.mark.parametrize("command, display_keys", COMMAND_TO_DISPLAY_KEYS)
+def test_display_keys_for_command(command: str, display_keys: List[str]) -> None:
+    assert keys.display_keys_for_command(command) == display_keys
+
+
+@pytest.mark.parametrize("command, display_keys", COMMAND_TO_DISPLAY_KEYS)
+def test_primary_display_key_for_command(command: str, display_keys: List[str]) -> None:
+    assert keys.primary_display_key_for_command(command) == display_keys[0]
+
+
+def test_display_keys_for_command_invalid_command(invalid_command: str) -> None:
+    with pytest.raises(keys.InvalidCommand):
+        keys.display_keys_for_command(invalid_command)
diff --git a/tests/helper/test_helper.py b/tests/helper/test_helper.py
index 8a79359..2e32e5c 100644
--- a/tests/helper/test_helper.py
+++ b/tests/helper/test_helper.py
@@ -5,7 +5,7 @@ from pytest import param as case
 from pytest_mock import MockerFixture
 
 from zulipterminal.api_types import Composition
-from zulipterminal.config.keys import primary_key_for_command
+from zulipterminal.config.keys import primary_display_key_for_command
 from zulipterminal.helper import (
     Index,
     canonicalize_color,
@@ -469,7 +469,7 @@ def test_notify_if_message_sent_outside_narrow(
     notify_if_message_sent_outside_narrow(req, controller)
 
     if footer_updated:
-        key = primary_key_for_command("NARROW_MESSAGE_RECIPIENT")
+        key = primary_display_key_for_command("NARROW_MESSAGE_RECIPIENT")
         report_success.assert_called_once_with(
             [
                 "Message is sent outside of current narrow."
diff --git a/tests/ui_tools/test_boxes.py b/tests/ui_tools/test_boxes.py
index dbdd082..8fc0503 100644
--- a/tests/ui_tools/test_boxes.py
+++ b/tests/ui_tools/test_boxes.py
@@ -12,7 +12,11 @@ from zulipterminal.api_types import (
     TYPING_STARTED_WAIT_PERIOD,
     TYPING_STOPPED_WAIT_PERIOD,
 )
-from zulipterminal.config.keys import keys_for_command, primary_key_for_command
+from zulipterminal.config.keys import (
+    keys_for_command,
+    primary_display_key_for_command,
+    primary_key_for_command,
+)
 from zulipterminal.config.symbols import (
     INVALID_MARKER,
     STREAM_MARKER_PRIVATE,
@@ -379,9 +383,12 @@ class TestWriteBox:
         expected_lines = [
             "Invalid recipient(s) - " + invalid_recipients,
             " - Use ",
-            ("footer_contrast", primary_key_for_command("AUTOCOMPLETE")),
+            ("footer_contrast", primary_display_key_for_command("AUTOCOMPLETE")),
             " or ",
-            ("footer_contrast", primary_key_for_command("AUTOCOMPLETE_REVERSE")),
+            (
+                "footer_contrast",
+                primary_display_key_for_command("AUTOCOMPLETE_REVERSE"),
+            ),
             " to autocomplete.",
         ]
 
@@ -1760,8 +1767,8 @@ class TestPanelSearchBox:
 
     @pytest.fixture
     def panel_search_box(self, mocker: MockerFixture) -> PanelSearchBox:
-        # X is the return from keys_for_command("UNTESTED_TOKEN")
-        mocker.patch(MODULE + ".keys_for_command", return_value="X")
+        # X is the return from display_keys_for_command("UNTESTED_TOKEN")
+        mocker.patch(MODULE + ".display_keys_for_command", return_value="X")
         panel_view = mocker.Mock()
         update_func = mocker.Mock()
         return PanelSearchBox(panel_view, "UNTESTED_TOKEN", update_func)