Repository: zulip/zulip-terminal
Pull request #1497 (2024-05-14 12:51:47)

Break down the generalized ENTER command
### What does this PR do, and why?
Breaks down the broader ENTER command into more specific commands.
- ACTIVATE_BUTTON
- EXECUTE_SEARCH
- NEW_LINE

This would enable using more specific help texts, categories and context.
And clarifies the purposes of the key better.

Updated the usage of each command in the UI tools.
Tests and docs updated.

The NEW_LINE command would also allow us to create a new user setting, where `ENTER` key and `Meta`+`Enter` can be switched between entering new lines and sending messages.

### Outstanding aspect(s)
- Need to consider the placement of the new commands within their categories in the Help Menu.
- `Ctrl`+`M` can work as `Enter` too. This has not yet been addressed.

### External discussion & connections
- [x] Discussed in **#zulip-terminal** in [`re-categorize`](https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Re-Categorization.20of.20Hotkeys/near/1793697)
- [ ] Fully fixes #
- [ ] Partially fixes issue #
- [ ] 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
- [x] Manually - Visual changes
- [ ] Adapting existing automated tests
- [ ] Adding automated tests for new behavior (or missing tests)
- [x] Existing automated tests should already cover this (*only a refactor of tested code*)

### Self-review checklist for each commit
- [x] It is a [minimal coherent idea](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development)
- [x] It has a commit summary following the [documented style](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development) (title & body)
- [x] It has a commit summary describing the  motivation and reasoning for the change
- [x] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [ ] It flows clearly from a previous branch commit, and/or prepares for the next commit

Niloth-p: Thank you for addressing the outstanding items @zormit

> How would you imagine the placement? Is it related to any of the other open PRs?

It's something that Neil suggested in the feedback of #1494. That we could re-order these help entries in the help menu, based on their significance / frequency of use, which is fairly subjective.
Hence, the question to confirm whether the order I've used here is fine, as this was not previously discussed in the stream.
(the order in the help menu is the same as the order in which the key bindings are listed in keys.py)

> How did you mean to address this?

I didn't get till there. Just mentioning that it exists, for the sake of completeness.

> My opinion is that, as an application, we should not worry about this.

Great then.
I wasn't quite aware about the details behind Ctrl M, and wasn't sure if I should spend any time looking into it either.
So, thanks for the explanation!

> A follow-up change could be to not have a single KEY_BINDINGS, but one per category/context. Or add the category in the keys_for_command calls.

Ah, great point, thank you for the suggestions!
We should probably have a discussion finalizing the approach and the order of merging, as this connects to a bunch of open PRs and discussed work related to categories/contexts.
zormit: @Niloth-p Can you please rebase this to the latest `main`? (It basically needs a regeneration of `docs/hotkeys.md` due to recent merges)
Niloth-p: Linting and tests are passing on my fork [here](https://github.com/Niloth-p/zulip-terminal/actions/runs/9346473251).
Niloth-p: Squashed the commits.
Kept the addition of 'space' in a separate commit as recommended, and newly wrote its commit message.
neiljp: @Niloth-p To clarify, this looks good to go except for space-handling in the hotkeys doc translation, so that looks like it'll need to be addressed if we're going to include that in this PR.
neiljp: @Niloth-p Thanks for the adjustment - good to know it was the simple change you had previously that already fixed this and was just dropped by mistake :+1:

Merging now :tada:

Patch:
diff --git a/docs/hotkeys.md b/docs/hotkeys.md
index 0d1ef3a..1272bfc 100644
--- a/docs/hotkeys.md
+++ b/docs/hotkeys.md
@@ -25,13 +25,13 @@
 |Scroll up|<kbd>PgUp</kbd> / <kbd>K</kbd>|
 |Scroll down|<kbd>PgDn</kbd> / <kbd>J</kbd>|
 |Go to bottom / Last message|<kbd>End</kbd> / <kbd>G</kbd>|
+|Trigger the selected entry|<kbd>Enter</kbd> / <kbd>Space</kbd>|
 |Narrow to all messages|<kbd>a</kbd> / <kbd>Esc</kbd>|
 |Narrow to all direct messages|<kbd>P</kbd>|
 |Narrow to all starred messages|<kbd>f</kbd>|
 |Narrow to messages in which you're mentioned|<kbd>#</kbd>|
 |Next unread topic|<kbd>n</kbd>|
 |Next unread direct message|<kbd>p</kbd>|
-|Perform current action|<kbd>Enter</kbd>|
 
 ## Searching
 |Command|Key Combination|
@@ -41,6 +41,7 @@
 |Search streams|<kbd>q</kbd>|
 |Search topics in a stream|<kbd>q</kbd>|
 |Search emojis from emoji picker|<kbd>p</kbd>|
+|Submit search and browse results|<kbd>Enter</kbd>|
 
 ## Message actions
 |Command|Key Combination|
@@ -84,6 +85,7 @@
 |Autocomplete @mentions, #stream_names, :emoji: and topics|<kbd>Ctrl</kbd> + <kbd>f</kbd>|
 |Cycle through autocomplete suggestions in reverse|<kbd>Ctrl</kbd> + <kbd>r</kbd>|
 |Narrow to compose box message recipient|<kbd>Meta</kbd> + <kbd>.</kbd>|
+|Insert new line|<kbd>Enter</kbd>|
 
 ## Editor: Navigation
 |Command|Key Combination|
diff --git a/zulipterminal/config/keys.py b/zulipterminal/config/keys.py
index e8d415e..3306f68 100644
--- a/zulipterminal/config/keys.py
+++ b/zulipterminal/config/keys.py
@@ -6,6 +6,7 @@ from typing import Dict, List
 
 from typing_extensions import NotRequired, TypedDict
 from urwid.command_map import (
+    ACTIVATE,
     CURSOR_DOWN,
     CURSOR_LEFT,
     CURSOR_MAX_RIGHT,
@@ -96,6 +97,11 @@ KEY_BINDINGS: Dict[str, KeyBinding] = {
         'help_text': 'Go to bottom / Last message',
         'key_category': 'navigation',
     },
+    'ACTIVATE_BUTTON': {
+        'keys': ['enter', ' '],
+        'help_text': 'Trigger the selected entry',
+        'key_category': 'navigation',
+    },
     'REPLY_MESSAGE': {
         'keys': ['r', 'enter'],
         'help_text': 'Reply to the current message',
@@ -249,16 +255,16 @@ KEY_BINDINGS: Dict[str, KeyBinding] = {
         'excluded_from_random_tips': True,
         'key_category': 'searching',
     },
+    'EXECUTE_SEARCH': {
+        'keys': ['enter'],
+        'help_text': 'Submit search and browse results',
+        'key_category': 'searching',
+    },
     'TOGGLE_MUTE_STREAM': {
         'keys': ['m'],
         'help_text': 'Mute/unmute streams',
         'key_category': 'stream_list',
     },
-    'ENTER': {
-        'keys': ['enter'],
-        'help_text': 'Perform current action',
-        'key_category': 'navigation',
-    },
     'THUMBS_UP': {
         'keys': ['+'],
         'help_text': 'Toggle thumbs-up reaction to the current message',
@@ -405,6 +411,14 @@ KEY_BINDINGS: Dict[str, KeyBinding] = {
         'help_text': 'Swap with previous character',
         'key_category': 'editor_text_manipulation',
     },
+    'NEW_LINE': {
+        # urwid_readline's command
+        # This obvious hotkey is added to clarify against 'enter' to send
+        # and to differentiate from other hotkeys using 'enter'.
+        'keys': ['enter'],
+        'help_text': 'Insert new line',
+        'key_category': 'msg_compose',
+    },
     'FULL_RENDERED_MESSAGE': {
         'keys': ['f'],
         'help_text': 'Show/hide full rendered message (from message information)',
@@ -437,6 +451,7 @@ ZT_TO_URWID_CMD_MAPPING = {
     "SCROLL_UP": CURSOR_PAGE_UP,
     "SCROLL_DOWN": CURSOR_PAGE_DOWN,
     "GO_TO_BOTTOM": CURSOR_MAX_RIGHT,
+    "ACTIVATE_BUTTON": ACTIVATE,
 }
 
 
@@ -482,6 +497,9 @@ def display_key_for_urwid_key(urwid_key: str) -> str:
     """
     Returns a displayable user-centric format of the urwid key.
     """
+    if urwid_key == " ":
+        return "Space"
+
     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)
diff --git a/zulipterminal/ui_tools/boxes.py b/zulipterminal/ui_tools/boxes.py
index f6d3294..91e2f80 100644
--- a/zulipterminal/ui_tools/boxes.py
+++ b/zulipterminal/ui_tools/boxes.py
@@ -947,14 +947,14 @@ class MessageSearchBox(urwid.Pile):
 
     def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
         if (
-            is_command_key("ENTER", key) and self.text_box.edit_text == ""
+            is_command_key("EXECUTE_SEARCH", key) and self.text_box.edit_text == ""
         ) or is_command_key("GO_BACK", key):
             self.text_box.set_edit_text("")
             self.controller.exit_editor_mode()
             self.controller.view.middle_column.set_focus("body")
             return key
 
-        elif is_command_key("ENTER", key):
+        elif is_command_key("EXECUTE_SEARCH", key):
             self.controller.exit_editor_mode()
             self.controller.search_messages(self.text_box.edit_text)
             self.controller.view.middle_column.set_focus("body")
@@ -1003,7 +1003,7 @@ class PanelSearchBox(ReadlineEdit):
 
     def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
         if (
-            is_command_key("ENTER", key) and self.get_edit_text() == ""
+            is_command_key("EXECUTE_SEARCH", key) and self.get_edit_text() == ""
         ) or is_command_key("GO_BACK", key):
             self.panel_view.view.controller.exit_editor_mode()
             self.reset_search_text()
@@ -1011,7 +1011,7 @@ class PanelSearchBox(ReadlineEdit):
             # Don't call 'Esc' when inside a popup search-box.
             if not self.panel_view.view.controller.is_any_popup_open():
                 self.panel_view.keypress(size, primary_key_for_command("GO_BACK"))
-        elif is_command_key("ENTER", key) and not self.panel_view.empty_search:
+        elif is_command_key("EXECUTE_SEARCH", key) and not self.panel_view.empty_search:
             self.panel_view.view.controller.exit_editor_mode()
             self.set_caption([("filter_results", " Search Results "), " "])
             self.panel_view.set_focus("body")
diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py
index 9ecb2d3..91c428a 100644
--- a/zulipterminal/ui_tools/buttons.py
+++ b/zulipterminal/ui_tools/buttons.py
@@ -120,7 +120,7 @@ class TopButton(urwid.Button):
         self.show_function()
 
     def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
-        if is_command_key("ENTER", key):
+        if is_command_key("ACTIVATE_BUTTON", key):
             self.activate(key)
             return None
         else:  # This is in the else clause, to avoid multiple activation
@@ -417,7 +417,7 @@ class EmojiButton(TopButton):
         self, size: urwid_Size, event: str, button: int, col: int, row: int, focus: int
     ) -> bool:
         if event == "mouse press" and button == 1:
-            self.keypress(size, primary_key_for_command("ENTER"))
+            self.keypress(size, primary_key_for_command("ACTIVATE_BUTTON"))
             return True
         return super().mouse_event(size, event, button, col, row, focus)
 
diff --git a/zulipterminal/ui_tools/messages.py b/zulipterminal/ui_tools/messages.py
index 3ddfa10..b8552fd 100644
--- a/zulipterminal/ui_tools/messages.py
+++ b/zulipterminal/ui_tools/messages.py
@@ -898,7 +898,7 @@ class MessageBox(urwid.Pile):
         if event == "mouse press" and button == 1:
             if self.model.controller.is_in_editor_mode():
                 return True
-            self.keypress(size, primary_key_for_command("ENTER"))
+            self.keypress(size, primary_key_for_command("ACTIVATE_BUTTON"))
             return True
 
         return super().mouse_event(size, event, button, col, row, focus)
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index 068ebad..9247b05 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -1758,7 +1758,11 @@ class EditModeView(PopUpView):
         for mode in EDIT_MODE_CAPTIONS:
             self.add_radio_button(mode)
         super().__init__(
-            controller, self.widgets, "ENTER", 62, "Topic edit propagation mode"
+            controller,
+            self.widgets,
+            "ACTIVATE_BUTTON",
+            62,
+            "Topic edit propagation mode",
         )
         # Set cursor to marked checkbox.
         for i in range(len(self.widgets)):


Test patch:
diff --git a/tests/ui_tools/test_boxes.py b/tests/ui_tools/test_boxes.py
index 8fc0503..b6c763c 100644
--- a/tests/ui_tools/test_boxes.py
+++ b/tests/ui_tools/test_boxes.py
@@ -1819,7 +1819,7 @@ class TestPanelSearchBox:
     @pytest.mark.parametrize(
         "log, expect_body_focus_set", [([], False), (["SOMETHING"], True)]
     )
-    @pytest.mark.parametrize("enter_key", keys_for_command("ENTER"))
+    @pytest.mark.parametrize("enter_key", keys_for_command("EXECUTE_SEARCH"))
     def test_keypress_ENTER(
         self,
         panel_search_box: PanelSearchBox,
diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py
index 3705bba..adc2faa 100644
--- a/tests/ui_tools/test_buttons.py
+++ b/tests/ui_tools/test_buttons.py
@@ -262,7 +262,7 @@ class TestStreamButton:
 
 class TestUserButton:
     # FIXME Place this in a general test of a derived class?
-    @pytest.mark.parametrize("enter_key", keys_for_command("ENTER"))
+    @pytest.mark.parametrize("enter_key", keys_for_command("ACTIVATE_BUTTON"))
     def test_activate_called_once_on_keypress(
         self,
         mocker: MockerFixture,
@@ -358,7 +358,7 @@ class TestEmojiButton:
         assert emoji_button.emoji_name == emoji_unit[0]
         assert emoji_button.reaction_count == count
 
-    @pytest.mark.parametrize("key", keys_for_command("ENTER"))
+    @pytest.mark.parametrize("key", keys_for_command("ACTIVATE_BUTTON"))
     @pytest.mark.parametrize(
         "emoji, has_user_reacted, is_selected_final, expected_reaction_count",
         [
diff --git a/tests/ui_tools/test_messages.py b/tests/ui_tools/test_messages.py
index 8deb4eb..08ba7d0 100644
--- a/tests/ui_tools/test_messages.py
+++ b/tests/ui_tools/test_messages.py
@@ -7,7 +7,7 @@ from bs4 import BeautifulSoup
 from pytest import param as case
 from urwid import Columns, Divider, Padding, Text
 
-from zulipterminal.config.keys import keys_for_command
+from zulipterminal.config.keys import keys_for_command, primary_key_for_command
 from zulipterminal.config.symbols import (
     ALL_MESSAGES_MARKER,
     DIRECT_MESSAGE_MARKER,
@@ -1961,11 +1961,14 @@ class TestMessageBox:
         assert isinstance(footlinks, expected_instance)
 
     @pytest.mark.parametrize(
-        "key", keys_for_command("ENTER"), ids=lambda param: f"left_click-key:{param}"
+        "key",
+        keys_for_command("ACTIVATE_BUTTON"),
+        ids=lambda param: f"left_click-key:{param}",
     )
     def test_mouse_event_left_click(
         self, mocker, msg_box, key, widget_size, compose_box_is_open
     ):
+        expected_keypress = primary_key_for_command("ACTIVATE_BUTTON")
         size = widget_size(msg_box)
         col = 1
         row = 1
@@ -1979,4 +1982,4 @@ class TestMessageBox:
         if compose_box_is_open:
             msg_box.keypress.assert_not_called()
         else:
-            msg_box.keypress.assert_called_once_with(size, key)
+            msg_box.keypress.assert_called_once_with(size, expected_keypress)
diff --git a/tests/ui_tools/test_popups.py b/tests/ui_tools/test_popups.py
index 0c98808..efa06d0 100644
--- a/tests/ui_tools/test_popups.py
+++ b/tests/ui_tools/test_popups.py
@@ -865,7 +865,7 @@ class TestEditModeView:
             (2, "change_all"),
         ],
     )
-    @pytest.mark.parametrize("key", keys_for_command("ENTER"))
+    @pytest.mark.parametrize("key", keys_for_command("ACTIVATE_BUTTON"))
     def test_select_edit_mode(
         self,
         edit_mode_view: EditModeView,
@@ -1558,7 +1558,7 @@ class TestStreamInfoView:
         self.stream_info_view.keypress(size, key)
         assert self.controller.exit_popup.called
 
-    @pytest.mark.parametrize("key", (*keys_for_command("ENTER"), " "))
+    @pytest.mark.parametrize("key", (*keys_for_command("ACTIVATE_BUTTON"), " "))
     def test_checkbox_toggle_mute_stream(
         self, key: str, widget_size: Callable[[Widget], urwid_Size]
     ) -> None:
@@ -1571,7 +1571,7 @@ class TestStreamInfoView:
 
         toggle_mute_status.assert_called_once_with(stream_id)
 
-    @pytest.mark.parametrize("key", (*keys_for_command("ENTER"), " "))
+    @pytest.mark.parametrize("key", (*keys_for_command("ACTIVATE_BUTTON"), " "))
     def test_checkbox_toggle_pin_stream(
         self, key: str, widget_size: Callable[[Widget], urwid_Size]
     ) -> None:
@@ -1584,7 +1584,7 @@ class TestStreamInfoView:
 
         toggle_pin_status.assert_called_once_with(stream_id)
 
-    @pytest.mark.parametrize("key", (*keys_for_command("ENTER"), " "))
+    @pytest.mark.parametrize("key", (*keys_for_command("ACTIVATE_BUTTON"), " "))
     def test_checkbox_toggle_visual_notification(
         self, key: str, widget_size: Callable[[Widget], urwid_Size]
     ) -> None: