Improve the handling of editor actions as a help category #1494

Niloth-p · 2024-05-09T15:48:25Z

What does this PR do, and why?

Improves the usage of urwid_readline hotkey commands.

Now that all editors support readline shortcuts after the merge of #1492,
all the readline shortcuts have been grouped into their own category.

Add missing key combinations, commands.
Re-categorize, re-order the keybindings.
Improve the help texts.

External discussion & connections

Discussed in #zulip-terminal in Re-categorization of hotkeys and Readline shortcuts in search`
Fully fixes #
Partially fixes issue #
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR boxes/views: Adopt urwid_readline for all editors. #1492
Requires merge of PR #
Merge will enable work on External hotkey commands: tagging, linting and checking sync #1498

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

Visual changes (Outdated)

neiljp

@Niloth-p Thanks for the followup on #1492 👍

This looks mostly OK, though I left a few notes inline.

One aspect to consider for these keys is that they do not derive from any direct use in the code of the commands, and we depend on there being a correlation between them and the readline library. The list is exposed via the library, but we don't make use of it - see eg. https://github.com/rr-/urwid_readline/issues/8

We could potentially do a check to ensure these are available and map correctly, or build some elements at runtime.

It may also be worth somehow tagging the COMMANDs as unused - even if just with a suffix - since then using them in the repo would make it obvious that they are in fact being used :)

zulipterminal/config/keys.py

neiljp · 2024-05-09T16:31:21Z

zulipterminal/config/keys.py

@@ -415,6 +415,7 @@ class KeyBinding(TypedDict):
    "msg_actions": "Message actions",
    "stream_list": "Stream list actions",
    "msg_compose": "Composing a Message",
+    "editor": "Editor actions",


This may change if we split the category, but I'm not sure if editor/input/text/entry are better words. I remember reading about this somewhere on czo semi-recently, but can't seem to dig out a reference.

The web app doesn't have a similar category.

I tried searching CZO.
I could only come up with "Editor Navigation" and "Text Operations", on my own.
Should I keep trying or proceed with these for now?

Generally if there are other changes to be made and you're unsure of a name or text (or something else that doesn't affect the changes to be made too much), it's easier to make that change in an overall update to the PR with a name you select, and highlight that you're unsure in eg. a self-review within the PR (on the line, if you want) before marking it for review. Then the other changes are already addressed, and it's possible the name you select will be fine - or it can be either updated quickly before merging by a committer, or discussed first.

However, since I already was thinking about it...

Since the name here refers to the same type of component, I often prefer to keep the same prefix, rather than use a phrase that can either have the word that refers to the component move around, or be different. That can then be a phrase like Prefix using part 2, or eg. Prefix - part 1 and Prefix - part 2. The latter form may be clearer here, and in terms of the choice of textbox/editor/... itself, we can always update that name later; generally I just aim to be consistent.

The library doesn't support all the features, but https://www.man7.org/linux/man-pages/man3/readline.3.html#EDITING_COMMANDS provides some ideas for naming.

So for the parts we could have

navigation or movement (based on the man page)

manipulation? modification?

neiljp

@Niloth-p I left the feedback here since you were asking about changes specific to the code.

When you split some commands into a new category, their existing description didn't make sense - at least in terms of the 'compose' case I highlighted - without being part of the same change, so it belongs in the same commit. Was that what you meant by the "compose box" change?

The other changes you highlighted in the channel that aren't yet in this PR are incremental improvements to the wording. They are fine in a separate commit in this PR.

I responded to most other changes in the topic, but for the ctrl l case which I hadn't yet done so, let's just keep this consistent with the category section name?

neiljp · 2024-05-09T19:18:52Z

zulipterminal/config/keys.py

@@ -415,6 +415,7 @@ class KeyBinding(TypedDict):
    "msg_actions": "Message actions",
    "stream_list": "Stream list actions",
    "msg_compose": "Composing a Message",
+    "editor": "Editor actions",


Generally if there are other changes to be made and you're unsure of a name or text (or something else that doesn't affect the changes to be made too much), it's easier to make that change in an overall update to the PR with a name you select, and highlight that you're unsure in eg. a self-review within the PR (on the line, if you want) before marking it for review. Then the other changes are already addressed, and it's possible the name you select will be fine - or it can be either updated quickly before merging by a committer, or discussed first.

However, since I already was thinking about it...

Since the name here refers to the same type of component, I often prefer to keep the same prefix, rather than use a phrase that can either have the word that refers to the component move around, or be different. That can then be a phrase like Prefix using part 2, or eg. Prefix - part 1 and Prefix - part 2. The latter form may be clearer here, and in terms of the choice of textbox/editor/... itself, we can always update that name later; generally I just aim to be consistent.

The library doesn't support all the features, but https://www.man7.org/linux/man-pages/man3/readline.3.html#EDITING_COMMANDS provides some ideas for naming.

So for the parts we could have

navigation or movement (based on the man page)

manipulation? modification?

Niloth-p · 2024-05-12T17:23:29Z

Visual Changes

Before re-ordering:
After re-ordering:

Niloth-p · 2024-05-13T20:36:17Z

Updated Help Menu

Updated Hotkeys.md

neiljp · 2024-05-17T04:01:32Z

docs/hotkeys.md

@@ -83,16 +83,16 @@
 |Autocomplete @mentions, #stream_names, :emoji: and topics|<kbd>Ctrl</kbd> + <kbd>f</kbd>|


Thanks for finding these!

I understand what this means, but for future readers of the log, it would be useful to clarify in what way these are missing, in the commit text.

For example, where did you find this information?
Also, are there any you've left out, from that resource? (intentionally?)

It seems this is resolved? 0d1babd now explains that they keys are coming from urwid_readline.

Agreed, though in general I would consider including a reference to that resource, if there is one, eg. a reference URL to the code or docs. It provides context for the status during review, and even if that becomes unreachable later, it can give some structure to look in eg. an updated git repo elsewhere.

In terms of the extra keys, like backspace/delete/left/right, let's discuss this as a possible adjustment after this PR. This is a fine first step, and other help refinements can be incremental after that, if they are a priority.

We're also currently missing documenting ctrl b for left, and we don't have the equivalent of right in readline since ctrl f is used for autocomplete. I mentioned a few points to @zormit related to this. It depends how far into readline support (via urwid-readline) we want to go.

zulipterminal/config/keys.py

docs/hotkeys.md

zulipterminal/config/keys.py

neiljp · 2024-05-17T04:25:35Z

docs/hotkeys.md

+|Undo last action|<kbd>Ctrl</kbd> + <kbd>_</kbd>|
+|Clear text box|<kbd>Ctrl</kbd> + <kbd>l</kbd>|
 |Cut forwards to the end of the line|<kbd>Ctrl</kbd> + <kbd>k</kbd>|
 |Cut backwards to the start of the line|<kbd>Ctrl</kbd> + <kbd>u</kbd>|
 |Cut forwards to the end of the current word|<kbd>Meta</kbd> + <kbd>d</kbd>|
 |Cut backwards to the start of the current word|<kbd>Ctrl</kbd> + <kbd>w</kbd> / <kbd>Meta</kbd> + <kbd>Backspace</kbd>|
 |Cut the current line|<kbd>Meta</kbd> + <kbd>x</kbd>|
 |Paste last cut section|<kbd>Ctrl</kbd> + <kbd>y</kbd>|
-|Undo last action|<kbd>Ctrl</kbd> + <kbd>_</kbd>|
-|Clear text box|<kbd>Ctrl</kbd> + <kbd>l</kbd>|
+|Delete previous character (to left)|<kbd>Ctrl</kbd> + <kbd>h</kbd>|
+|Transpose characters|<kbd>Ctrl</kbd> + <kbd>t</kbd>|


We discussed this reordering, and I agree with it, but I'd look to see a motivation for the change expressed in the commit text.

The current commit title/text looks much more broader than I'd expect, given this change to hotkeys.md, which is also the impact I'd expect on the help menu.

zulipterminal/config/keys.py

neiljp

@Niloth-p This looks like being close to merging, and the improvements since the last iteration look good, including those you found out from elsewhere 👍

There are a few spots where we could adjust things further, or that we could simplify, as per the notes I left inline - which for some reason appear to have been posted as a review separately already.

It's not important to handle this now, but one aspect I'd considered for the future was to handle the readline commands automatically, so setting the 'keys' lists via the dict(s) in urwid-readline. I don't expect we'd need to keep updating them, but it would ensure that they didn't get reset in the help, and were kept synchronized.

neiljp · 2024-05-17T05:27:41Z

zulipterminal/config/keys.py

        'key_category': 'editor_navigation',
    },
    'ONE_WORD_BACKWARD': {
        'keys': ['meta b', 'shift left'],
-        'help_text': 'Jump backward one word',
+        'help_text': 'Jump backward to the start of current or previous word',


I'm not entirely convinced of all of these - they're clearer wording in some cases, but also longer. More compact and concise would make these clearer.

For the navigation text, perhaps we can simplify using the fact that they all start with 'Jump'? That common phrasing suggests a simplification.

The other reason to keep these shorter is that they make the help menu wider or start to wrap, though currently there is one command that is even longer than these.

neiljp · 2024-05-17T05:33:12Z

@zormit I started the original reviewing here, but would welcome any feedback you have :)

zormit

This looks pretty good to me. From the previous discussion, I take it that the commit messages were improved, even though I don't know what it looked like before :)

The only commit that doesn't have a description is the last one (5c14347), and I'm wondering where the specific wordings were discussed and decided. I think it was in the Zulip thread. Is it enough to reference that in the PR description, or should the commit also have that bit of information?

Open points that I see, just to consolidate them, but that do not necessarily need to be covered in this PR:

programmatically synchronizing the readline specific keys with the urwid_readline dependency
tagging COMMANDS as unused (I'm not sure I understand what @neiljp was asking for here)

zormit · 2024-05-20T12:01:18Z

docs/hotkeys.md

@@ -83,16 +83,16 @@
 |Autocomplete @mentions, #stream_names, :emoji: and topics|<kbd>Ctrl</kbd> + <kbd>f</kbd>|


It seems this is resolved? 0d1babd now explains that they keys are coming from urwid_readline.

zulipterminal/config/keys.py

Niloth-p · 2024-05-20T18:34:07Z

@zormit The wording for the final commit was not particularly discussed anywhere beyond the feedback here. So, they're not yet decided.

The tagging of commands has been done as part of #1498 after Neil's recommendation in the feedback for this PR.

The suggestion of programmatic synchronization was made recently, after #1498 was opened, so has not been attempted yet.

rsashank · 2024-05-28T09:08:57Z

LGTM. I've caught up with the previous comments left, and everything looks in order to me. Removing the buddy review tag.

I apologize for the delay in buddy reviews; I was unavailable due to exams when the buddy review tag was added. I'll be quicker with reviews now on.

zormit · 2024-05-29T15:02:22Z

So from my point of view, to finalize this PR you'll need to

make sure the wording is final (this requires a review by/discussion with @neiljp )
update the last commit message (I'm imagining a description how you came up with the changes and why)

(edit: i'll remove "needs mentor review" tag as well, but please let me know if I can help getting this over the line or just re-add it, if needed)

Niloth-p · 2024-05-31T18:43:42Z

Just added a commit description to the last commit, as per feedback.
@zormit This good?

neiljp

I've left a few comments, but I really don't want to dwell on minor details too much and instead get this merged soon.

Thanks to @Niloth-p and for everyone's reviews - I'll likely make a few minor text changes and merge a bit later, but am busy for a bit now.

neiljp · 2024-05-31T19:19:42Z

docs/hotkeys.md

@@ -83,16 +83,16 @@
 |Autocomplete @mentions, #stream_names, :emoji: and topics|<kbd>Ctrl</kbd> + <kbd>f</kbd>|


Agreed, though in general I would consider including a reference to that resource, if there is one, eg. a reference URL to the code or docs. It provides context for the status during review, and even if that becomes unreachable later, it can give some structure to look in eg. an updated git repo elsewhere.

In terms of the extra keys, like backspace/delete/left/right, let's discuss this as a possible adjustment after this PR. This is a fine first step, and other help refinements can be incremental after that, if they are a priority.

We're also currently missing documenting ctrl b for left, and we don't have the equivalent of right in readline since ctrl f is used for autocomplete. I mentioned a few points to @zormit related to this. It depends how far into readline support (via urwid-readline) we want to go.

neiljp · 2024-05-31T20:16:30Z

docs/hotkeys.md

+|Delete character behind cursor|<kbd>Ctrl</kbd> + <kbd>h</kbd>|
+|Swap with previous character|<kbd>Ctrl</kbd> + <kbd>t</kbd>|


My only nit with this change is that the other changes are more consistent with using 'previous', so maybe we just drop the (to left).

Updated key bindings to include undocumented key combinations that are provided by urwid_readline. For reference see: https://github.com/rr-/urwid_readline/blob/master/urwid_readline/readline_edit.py#L88 All undocumented hotkeys and commands provided by urwid_readline are now present, except for: - delete and backspace keys, as they're fairly obvious in modern systems - Ctrl + d hotkey for character deletion (same as delete key) - currently used for sending messages from the compose box - Ctrl + b/f hotkeys for stepping backwards/forwards by one character - Ctrl + f is currently used for autocomplete from the compose box Hotkeys document regenerated.

Now that all editors support readline shortcuts after e32ae3e, they've been moved from the message compose category and grouped into 2 new 'editor' sub-categories: - navigation - text manipulation Hotkeys document regenerated.

The text manipulation hotkeys are re-ordered to place more powerful commands at the beginning of the category in the help menu and the doc. As part of this process, the editor key bindings are grouped by their categories within the source file, improving readability. Hotkeys document regenerated.

- Remove the common prefix "jump [to]", mentioning the targets directly (shortens the length of the help text without a reduction in clarity) - Use "Start" instead of "Beginning" (consistent with existing uses) - Clarify that Shift+Left (and equivalent) includes the current word - Use the simpler "Swap" instead of "Transpose" - Consistently use "Previous" in descriptions Hotkeys document regenerated.

neiljp · 2024-06-01T02:47:59Z

@Niloth-p I just pushed the textual corrections I mentioned, and again after rebasing against a PR I just pushed to allow long URLs in the commit text body, and am merging now - thanks again! 🎉

Please do try a git range-diff against your local branch to see the specific changes; we can discuss that in the project stream or private stream, if that would be helpful more generally.

zulipbot added the size: M [Automatic label added by zulipbot] label May 9, 2024

Niloth-p force-pushed the category/1494-readline/pr branch from 16dbd30 to 3035803 Compare May 9, 2024 15:53

Niloth-p added area: hotkeys PR needs review PR requires feedback to proceed labels May 9, 2024

neiljp reviewed May 9, 2024

View reviewed changes

neiljp added area: popup: help 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 May 9, 2024

neiljp reviewed May 9, 2024

View reviewed changes

Niloth-p force-pushed the category/1494-readline/pr branch from 3035803 to 357bb54 Compare May 12, 2024 16:31

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels May 12, 2024

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

Niloth-p changed the title ~~Make editor actions a help category~~ Improve the handling of editor actions as a help category May 13, 2024

Niloth-p removed the area: hotkeys label May 14, 2024

Niloth-p mentioned this pull request May 14, 2024

External hotkey commands: tagging, linting and checking sync #1498

Open

18 tasks

Niloth-p force-pushed the category/1494-readline/pr branch from 5b275e6 to 7e2290c Compare May 14, 2024 14:52

neiljp reviewed May 17, 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 May 17, 2024

zormit self-requested a review May 17, 2024 15:56

Niloth-p force-pushed the category/1494-readline/pr branch from 7e2290c to 5c14347 Compare May 18, 2024 00:35

Niloth-p removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label May 18, 2024

Niloth-p added the PR needs review PR requires feedback to proceed label May 18, 2024

zormit approved these changes May 20, 2024

View reviewed changes

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

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

Niloth-p force-pushed the category/1494-readline/pr branch 2 times, most recently from aec2558 to 0ba249c Compare May 31, 2024 18:41

Niloth-p mentioned this pull request May 31, 2024

Break down the generalized ENTER command #1497

Merged

18 tasks

neiljp reviewed May 31, 2024

View reviewed changes

neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels May 31, 2024

neiljp force-pushed the category/1494-readline/pr branch from 0ba249c to ba0cf98 Compare June 1, 2024 00:48

neiljp mentioned this pull request Jun 1, 2024

gitlint: Add regex to ignore some long lines. #1509

Merged

18 tasks

Niloth-p added 4 commits May 31, 2024 18:57

keys: Extract editor hotkeys into two new categories.

d3dd70b

Now that all editors support readline shortcuts after e32ae3e, they've been moved from the message compose category and grouped into 2 new 'editor' sub-categories: - navigation - text manipulation Hotkeys document regenerated.

neiljp force-pushed the category/1494-readline/pr branch from ba0cf98 to cd34d57 Compare June 1, 2024 01:57

neiljp merged commit 5fbfe07 into zulip:main Jun 1, 2024
21 checks passed

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

Niloth-p deleted the category/1494-readline/pr branch June 11, 2024 05:01

neiljp mentioned this pull request Jun 24, 2024

TRACKING: Improve hotkey descriptions, categories & accuracy #1524

Open

34 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the handling of editor actions as a help category #1494

Improve the handling of editor actions as a help category #1494

Niloth-p commented May 9, 2024 •

edited

Loading

neiljp left a comment

neiljp May 9, 2024

Niloth-p May 9, 2024

neiljp May 9, 2024

neiljp left a comment

neiljp May 9, 2024

Niloth-p commented May 12, 2024 •

edited

Loading

Niloth-p commented May 13, 2024

neiljp May 17, 2024 •

edited

Loading

zormit May 20, 2024

neiljp May 31, 2024

neiljp May 17, 2024

neiljp left a comment

neiljp May 17, 2024

neiljp commented May 17, 2024

zormit left a comment

zormit May 20, 2024

Niloth-p commented May 20, 2024

rsashank commented May 28, 2024

zormit commented May 29, 2024 •

edited

Loading

Niloth-p commented May 31, 2024

neiljp left a comment

neiljp May 31, 2024

neiljp May 31, 2024

neiljp commented Jun 1, 2024

		@@ -83,16 +83,16 @@
		\|Autocomplete @mentions, #stream_names, :emoji: and topics\|<kbd>Ctrl</kbd> + <kbd>f</kbd>\|

		\|Delete character behind cursor\|<kbd>Ctrl</kbd> + <kbd>h</kbd>\|
		\|Swap with previous character\|<kbd>Ctrl</kbd> + <kbd>t</kbd>\|

Improve the handling of editor actions as a help category #1494

Improve the handling of editor actions as a help category #1494

Conversation

Niloth-p commented May 9, 2024 • edited Loading

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes (Outdated)

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

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p commented May 12, 2024 • edited Loading

Visual Changes

Niloth-p commented May 13, 2024

Updated Help Menu

Updated Hotkeys.md

neiljp May 17, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented May 17, 2024

zormit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p commented May 20, 2024

rsashank commented May 28, 2024

zormit commented May 29, 2024 • edited Loading

Niloth-p commented May 31, 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

neiljp commented Jun 1, 2024

Niloth-p commented May 9, 2024 •

edited

Loading

Niloth-p commented May 12, 2024 •

edited

Loading

neiljp May 17, 2024 •

edited

Loading

zormit commented May 29, 2024 •

edited

Loading