Break down the generalized ENTER command #1497

Niloth-p · 2024-05-14T12:51:47Z

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

Discussed in #zulip-terminal in re-categorize
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
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
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

rsashank · 2024-05-28T08:44:27Z

The intention behind the refactoring is clear. The code looks good to me.

zormit

Looks good to me. I think it achieves the goal of preparing for better contextual help.

A downside of restructuring it like this, is that the code now could accidentally get stale as no one is stopped from using for example ACTIVATE_BUTTON when actually in a context where EXECUTE_SEARCH would be the "correct Enter" variant. A follow-up change could be to not have a single KEY_BINDINGS, but one per category/context. But that's not meant as a request, just a thought that came up. Or add the category in the keys_for_command calls.

Regarding your outstanding items:

Need to consider the placement of the new commands within their categories in the Help Menu.

How would you imagine the placement? Is it related to any of the other open PRs?

Ctrl+M can work as Enter too. This has not yet been addressed.

How did you mean to address this? The fact that ^M works "as Enter" (or rather CR) has to do with ASCII control characters. My understanding is that it's determined on a system or maybe terminal emulator level. My opinion is that, as an application, we should not worry about this. It might even be impossible to tell the difference at this level. As for documenting this, I wouldn't do that either for similar reasons (it's nothing people usually need, and if people know about this, it's rather more widely applicable than just zulip-terminal)

Niloth-p · 2024-05-31T19:07:18Z

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.

Niloth-p · 2024-06-03T08:12:52Z

Linting and tests are passing on my fork here.

neiljp

@Niloth-p It'll be useful to have these split 👍

I left a few points inline. From a code perspective this is fairly straightforward, though the ordering/category looks a bit strange.

@zormit, you wrote:

A downside of restructuring it like this, is that the code now could accidentally get stale as no one is stopped from using for example ACTIVATE_BUTTON when actually in a context where EXECUTE_SEARCH would be the "correct Enter" variant. A follow-up change could be to not have a single KEY_BINDINGS, but one per category/context. But that's not meant as a request, just a thought that came up. Or add the category in the keys_for_command calls.

I'm not sure I follow this; do you mean where enter ends up being present for multiple actions, it could be unclear what should occur if two are present on the same UI object by mistake, or something like that?

neiljp · 2024-06-12T15:57:15Z

zulipterminal/config/keys.py

+    'ACTIVATE_BUTTON': {
+        'keys': ['enter'],
+        'help_text': 'Trigger the selected entry',
+        'key_category': 'general',
+    },


Priority: In the online help and the generated document, this is now at the top of the General section, and help overall, which seems a bit more dominant than I would expect!

Category: This is a very 'general' key, but perhaps really belongs in the same kind of category as the top part of those in 'Navigation', ie. moving around in the UI? That supports splitting that existing section, which I think was discussed already, so we could have eg. 'UI Navigation', or something simpler.

Keys: There's a small section at the end of keys.py which syncs our keys back to urwid (check the git blame/PR). It would be useful to follow up on this commit with a commit/PR which emphasizes that other keys can also activate an item/button/..., which has come up in discussions in the channel previously. For example, space works on buttons, at least as it stands, and possibly others.

=> Maybe:

Split out 'UI Navigation' category first? (or move this addition into Navigation?)

Apply this

Investigate correcting our help to match the urwid defaults, or adjust the urwid keys themselves

In the longer term it would be useful to make this even more specific, since 'trigger' or 'activate' is very general, and it's not obvious just reading this one entry that it is overridden by the other uses of enter, ie. those we're splitting out here, as well as for replying to a message :)

Another aspect of that is my note in the discussion that we don't document the behavior of enter in the user list to reply/compose to a user. That's distinct from this split, and likely depends on a user-list/panel category.

Re. UI Navigation category,
I can't particularly recall discussing "UI Navigation".
Do you mean moving the narrowing commands in #1516?
We haven't used the term "UI Navigation" there yet, just extracting the narrowing commands from it. I'll update the category name.
I think we could do them independently, and in any order? As it would make sense to have ACTIVATE_BUTTON in navigation category even before it's split.
Please let me know if you have more in mind, as I don't think we've discussed this before.

space works on buttons

I totally missed that one.
Thank you for that clear guidance. That really helped.
I've added the space key, updated the display function.
I have tied it to the Urwid mapping even though we do not add any new hotkeys at the moment, for consistency and safety.

document the behavior of enter in the user list to reply/compose to a user. That's distinct from this split, and likely depends on a user-list/panel category.

Yes, I've added that as part of this commit in #1515.

I'm not sure 'UI Navigation' is the best term, but essentially referring to the simple UI movement/trigger commands that are left behind after indeed #1516.

So the main aspect until #1516 is really where we place ACTIVATE_BUTTON for now, which you addressed in the commit to be squashed.

My last point was upon rereading some discussion text, so good to see it in another PR.

zulipterminal/config/keys.py

Niloth-p

@neiljp Thank you for the detailed review and the great explanations!

I hope I've addressed all your suggestions with this update.

I'm not sure I follow this; do you mean where enter ends up being present for multiple actions, it could be unclear what should occur if two are present on the same UI object by mistake, or something like that?

This is my understanding of what @zormit meant,
If we add a new button, and support keypress for it, then the code there can use is_command_key("EXECUTE_SEARCH") instead of is_command_key("ACTIVATE_BUTTON"), and achieve the same functionality.
As in the developer could add a mismatched command that may still work because both of them map to Enter. But, this error would create wrong contextual hints.

Niloth-p · 2024-06-13T06:20:22Z

tests/ui_tools/test_messages.py

@@ -1968,6 +1968,7 @@ def test_footlinks_limit(self, maximum_footlinks, expected_instance):
    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")


Needed to edit this test as mouse_event always calls the primary_key_for_command("ACTIVATE_BUTTON") and does not use the passed key.

zulipterminal/config/keys.py

Niloth-p · 2024-06-13T06:22:35Z

zulipterminal/config/keys.py

+    'ACTIVATE_BUTTON': {
+        'keys': ['enter'],
+        'help_text': 'Trigger the selected entry',
+        'key_category': 'general',
+    },


Re. UI Navigation category,
I can't particularly recall discussing "UI Navigation".
Do you mean moving the narrowing commands in #1516?
We haven't used the term "UI Navigation" there yet, just extracting the narrowing commands from it. I'll update the category name.
I think we could do them independently, and in any order? As it would make sense to have ACTIVATE_BUTTON in navigation category even before it's split.
Please let me know if you have more in mind, as I don't think we've discussed this before.

space works on buttons

I totally missed that one.
Thank you for that clear guidance. That really helped.
I've added the space key, updated the display function.
I have tied it to the Urwid mapping even though we do not add any new hotkeys at the moment, for consistency and safety.

document the behavior of enter in the user list to reply/compose to a user. That's distinct from this split, and likely depends on a user-list/panel category.

Yes, I've added that as part of this commit in #1515.

neiljp

@Niloth-p It looks like most of my points were accounted for already, while you were updating :)

Re the point about potentially incorrect use of Commands that are essentially equivalent in terms of keys, I agree this is a possible challenge. However, the aim is to maintain semantic naming, a little like we do with theme styles which may have the same content but are still named differently. We can examine this again when the contextual part becomes finalized.

zulipterminal/config/keys.py

neiljp · 2024-06-15T21:35:43Z

zulipterminal/config/keys.py

+    'ACTIVATE_BUTTON': {
+        'keys': ['enter'],
+        'help_text': 'Trigger the selected entry',
+        'key_category': 'general',
+    },


I'm not sure 'UI Navigation' is the best term, but essentially referring to the simple UI movement/trigger commands that are left behind after indeed #1516.

So the main aspect until #1516 is really where we place ACTIVATE_BUTTON for now, which you addressed in the commit to be squashed.

My last point was upon rereading some discussion text, so good to see it in another PR.

neiljp · 2024-06-15T22:57:23Z

zulipterminal/config/keys.py

@@ -442,6 +446,7 @@ class KeyBinding(TypedDict):
    "SCROLL_UP": CURSOR_PAGE_UP,
    "SCROLL_DOWN": CURSOR_PAGE_DOWN,
    "GO_TO_BOTTOM": CURSOR_MAX_RIGHT,
+    "ACTIVATE_BUTTON": ACTIVATE,


I'm not sure this is strictly necessary, since the urwid default map has the values we're setting already, but it's fine to include and a good pointer that we may need to return to it in future.

Niloth-p · 2024-06-17T14:28:29Z

Squashed the commits.
Kept the addition of 'space' in a separate commit as recommended, and newly wrote its commit message.

neiljp · 2024-06-22T05:18:26Z

docs/hotkeys.md

@@ -24,7 +24,7 @@
 |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>|
+|Trigger the selected entry|<kbd>Enter</kbd> / |


It looks like we need a special case to handle the documented keys here?

Oops with the squashing. Guess I didn't check the diff, my apologies.

neiljp · 2024-06-22T05:34:32Z

zulipterminal/config/keys.py

@@ -244,16 +249,16 @@ class KeyBinding(TypedDict):
        'excluded_from_random_tips': True,
        'key_category': 'searching',
    },
+    'EXECUTE_SEARCH': {
+        'keys': ['enter'],
+        'help_text': 'Execute the search and go to search results',


Nit: We can address this on merging, but 'execute' is likely not the best user-facing language.

Perhaps "Submit search and browse results"?

I don't have the web app to compare to right now (as we discussed in the call).

I checked the list here. They don't seem to use an equivalent for the search op. But, I did find an equivalent for ACTIVATE_BUTTON - "Use Enter to engage with elements".

I have updated the EXECUTE_SEARCH description to match your suggestion, and left the ACTIVATE_BUTTON as-is.

neiljp · 2024-06-22T05:41:18Z

@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.

Breaks down the ENTER command into more specific commands. - ACTIVATE_BUTTON - EXECUTE_SEARCH - NEW_LINE Tests and docs updated.

For consistency with Urwid's ACTIVATE command. Urwid's command map is updated to link Urwid's ACTIVATE with our ACTIVATE_BUTTON command. Although they currently use the same keys, they are synced to ensure future compatibility. Hotkeys document regenerated. Tests updated.

neiljp · 2024-06-23T01:17:33Z

@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 👍

Merging now 🎉

zulipbot added the size: L [Automatic label added by zulipbot] label May 14, 2024

Niloth-p added PR needs review PR requires feedback to proceed PR needs buddy review PR needs mentor review area: popup: help labels May 14, 2024

zormit requested review from zormit and removed request for zormit May 24, 2024 14:45

rsashank removed the PR needs buddy review label May 28, 2024

zormit self-requested a review May 29, 2024 15:04

zormit approved these changes May 29, 2024

View reviewed changes

zormit removed the PR needs mentor review label May 29, 2024

Niloth-p force-pushed the category/1497-break-enter/pr branch from 80ea619 to f84e1ca Compare June 3, 2024 07:52

Niloth-p force-pushed the category/1497-break-enter/pr branch from f84e1ca to 64f5c89 Compare June 3, 2024 11:52

Niloth-p mentioned this pull request Jun 6, 2024

Break down the GO_BACK command into separate commands #1514

Merged

18 tasks

neiljp reviewed Jun 12, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 12, 2024

Niloth-p commented Jun 13, 2024

View reviewed changes

Niloth-p added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 13, 2024

neiljp reviewed Jun 15, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 15, 2024

Niloth-p force-pushed the category/1497-break-enter/pr branch from 00da8d4 to f441bac Compare June 16, 2024 09:13

Niloth-p added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 16, 2024

neiljp reviewed Jun 22, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 22, 2024

Niloth-p added 2 commits June 23, 2024 00:50

keys/ui_tools: Break down the generalized ENTER command.

65e5668

Breaks down the ENTER command into more specific commands. - ACTIVATE_BUTTON - EXECUTE_SEARCH - NEW_LINE Tests and docs updated.

Niloth-p force-pushed the category/1497-break-enter/pr branch from f441bac to f423e94 Compare June 22, 2024 19:23

Niloth-p added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 22, 2024

neiljp merged commit 28d5f6a into zulip:main Jun 23, 2024
21 checks passed

neiljp added this to the Next Release milestone Jun 23, 2024

neiljp removed the PR needs review PR requires feedback to proceed label Jun 23, 2024

This was referenced Jun 23, 2024

Group narrowing commands and improve descriptions #1516

Merged

TRACKING: Improve hotkey descriptions, categories & accuracy #1524

Open

Niloth-p deleted the category/1497-break-enter/pr branch June 24, 2024 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break down the generalized ENTER command #1497

Break down the generalized ENTER command #1497

Niloth-p commented May 14, 2024

rsashank commented May 28, 2024

zormit left a comment

Niloth-p commented May 31, 2024

Niloth-p commented Jun 3, 2024

neiljp left a comment

neiljp Jun 12, 2024

Niloth-p Jun 13, 2024

neiljp Jun 15, 2024

Niloth-p left a comment

Niloth-p Jun 13, 2024

Niloth-p Jun 13, 2024

neiljp left a comment

neiljp Jun 15, 2024

neiljp Jun 15, 2024

Niloth-p commented Jun 17, 2024

neiljp Jun 22, 2024

Niloth-p Jun 22, 2024

neiljp Jun 22, 2024

Niloth-p Jun 22, 2024

neiljp commented Jun 22, 2024

neiljp commented Jun 23, 2024

Break down the generalized ENTER command #1497

Break down the generalized ENTER command #1497

Conversation

Niloth-p commented May 14, 2024

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Self-review checklist for each commit

rsashank commented May 28, 2024

zormit left a comment

Choose a reason for hiding this comment

Niloth-p commented May 31, 2024

Niloth-p commented Jun 3, 2024

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

Niloth-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Niloth-p commented Jun 17, 2024

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

neiljp commented Jun 22, 2024

neiljp commented Jun 23, 2024