Introduce help context #1484

Niloth-p · 2024-04-18T16:56:05Z

What does this PR do, and why?

Introduces context to track the current focus.
Uses context to generate relevant footer hints.

Adds missing tests and new tests.

External discussion & connections

Discussed in #zulip-terminal in Context Based Help Tips and Adding context to hotkeys
Fully fixes #
Partially fixes issue Make help hints context-dependent #515
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)

Tests updated.

Hotkeys that used to be excluded from general help hints, can be useful as hints in particular contexts.

Tests updated.

- Add a context property. - Update footer text on context changes, if a footer event is not already in progress. Tests added.

Tests updated.

Set context to "editor" only if it is not already set to more specific editor contexts. Reset context to "" on exiting any editor. Tests added and updated.

Tests added.

Check if the current focus is on - the menu view -> set context to 'general' - the stream list - the topic list Tests added.

Tests updated.

Tests added.

Add checks for clashing keys in the same context. Added new `context` argument. Generates a new file `hotkeys-by-context.md`. Refactored all the functions to take any field as an input, either category / context, by passing newly created variables. Added FileNotFoundError exception. Removed `Esc` from `ALL_MESSAGES` to pass linting.

Niloth-p

There's a bunch of unfinished work here, and some significant changes to be made. I've mentioned the main ones in the CZO discussion.
I wanted to first ensure that I'm going in the right direction, so I've opened it up for review already.

Niloth-p · 2024-04-24T20:12:38Z

zulipterminal/config/keys.py

@@ -220,7 +220,7 @@ class KeyBinding(TypedDict):
        'key_context': 'stream_view',
    },
    'ALL_MESSAGES': {
-        'keys': ['a', 'esc'],
+        'keys': ['a'],


This is a temporary change. To make the linting tests pass.
As both the Esc key mappings clash in the 'general' context.

I could add an exception instead too, if necessary later.
Or I could move the ALL_MESSAGES command to the "message_view" context instead until that bug is resolved.

This was discussed in the stream recently, I believe? :)

Your last suggestion reflects the current reality, so might be the short-term solution, with a comment re the bug/issue/discussion.

Niloth-p · 2024-04-24T20:26:25Z

tools/lint-hotkeys

I know the changes to this file would be pretty difficult to read and verify.
Do I need to break it down into layers?
Tested manually for the following cases:

[x] no hotkeys-by-context.md file
[x] mismatching existing hotkeys-by-context.md file
[x] matching existing hotkeys-by-context.md file

[x] no hotkeys.md file
[x] mismatching existing hotkeys.md file
[x] matching existing hotkeys.md file

[x] no arguments
[x] context argument
[x] fix argument
[x] both arguments

It would definitely help to split this up, since this is multiple aspects:

linting in any new ways, or relaxing linting as we move forward

adding the new doc generation, if we end up going in that direction

However, as you say it would also be easier to verify, such as if you eg. refactor existing names first.

neiljp

@Niloth-p There's a lot here!

I did give this a manual test, and many parts definitely seem to be working. I mainly moved around areas of the UI, and since some hotkeys (including just ?) effectively show a new random hint, it allows some degree of sampling of them.

I've not looked at the hotkeys assignment doc, since keys.py works fairly well in the first instance, and I'd started reading that before you added the feature :)

I've only left inline comments until prior to the specific context setting. That's partly since I'm wondering if there may be an alternative approach rather than setting it explicitly in many places. For example, you've already set the context depending on a focus path in a few places instead, and for popups this could be a class variable that is accessed by generic popup show code?

One helpful change would be to address the 'cycling' issue sooner rather than later, but even before or part of that we can improve the design slightly. The original design was to indicate that ? is general help, and show a random help next to it, but currently we have the help key right up next to Help and in parentheses rather than square brackets (which we now tend to use). There's also a colon rather than an indication that the general help is distinct from the hint. As an extension specific to this change, it'd be great to show the relevance of the hint - does it work in compose, stream list, selected message, etc. I could tell that it was displaying reasonable hints based on the context, but showing the context itself would be helpful for those less familiar :)

neiljp · 2024-04-19T06:05:33Z

zulipterminal/config/keys.py

    'SEARCH_STREAMS': {
        'keys': ['q'],
        'help_text': 'Search Streams',
        'key_category': 'searching',
+        'key_context': 'general',
    },
    'SEARCH_TOPICS': {
        'keys': ['q'],
        'help_text': 'Search topics in a stream',
        'key_category': 'searching',
+        'key_context': 'topic_view',


I think you mentioned this, but there is nuance with these two, since they depend on the state?

Yes, they both would actually belong in a general context, but need to apply a check (based on the state) to determine which one is applicable at the moment.

neiljp · 2024-04-19T06:13:35Z

zulipterminal/config/keys.py

    },
    'REPLY_MESSAGE': {
        'keys': ['r', 'enter'],
        'help_text': 'Reply to the current message',
        'key_category': 'msg_actions',
+        'key_context': 'message_view',


The _view doesn't appear to add much descriptively, or for other context names?

This relates to a message being selected/focused, right?

True.
I hadn't considered just not using any suffix, was instead trying to choose a good suffix.
Good idea!

Yes, it does. I'll add that as a comment to the HELP_CONTEXTS dict.

neiljp · 2024-04-19T06:16:09Z

zulipterminal/config/keys.py

    },
    'CYCLE_COMPOSE_FOCUS': {
        'keys': ['tab'],
        'help_text': 'Cycle through recipient and content boxes',
        'key_category': 'msg_compose',
+        'key_context': 'write_box',


As per another comment, using a user-facing name would be preferable for contexts.

This could be similar to the key_category, eg composing_message? Keeping names easily searchable (distinct from others) could be useful, so the same as key_category could be challenging if we want that.

neiljp · 2024-04-19T06:18:52Z

zulipterminal/config/keys.py

    },
    'AUTOCOMPLETE': {
        'keys': ['ctrl f'],
        'help_text': ('Autocomplete @mentions, #stream_names, :emoji:'
                      ' and topics'),
        'key_category': 'msg_compose',
+        'key_context': 'editor',


If this has the same help text, it only applies to the message composition content, though we do support autocomplete in the stream/topic/recipient boxes for composing too - but only in the matching boxes, and no @mentions or #streams with prefixes, and never emoji.

This may come down to my understanding your use of editor, and the overrides or subsets.

neiljp · 2024-04-24T20:48:30Z

zulipterminal/config/keys.py

@@ -220,7 +220,7 @@ class KeyBinding(TypedDict):
        'key_context': 'stream_view',
    },
    'ALL_MESSAGES': {
-        'keys': ['a', 'esc'],
+        'keys': ['a'],


This was discussed in the stream recently, I believe? :)

Your last suggestion reflects the current reality, so might be the short-term solution, with a comment re the bug/issue/discussion.

neiljp · 2024-04-24T22:37:21Z

zulipterminal/ui.py

        # Avoid updating repeatedly (then pausing and showing default text)
        # This is simple, though doesn't avoid starting one thread for each call
+        if context_change and self._is_footer_event_running:
+            return
        if text_list == self._w.footer.text:
            return


The existing comment doesn't apply to your new code? Though see my other response, since I suspect it could do, depending if you refactor first and how you rewrite.

neiljp · 2024-04-24T22:44:28Z

zulipterminal/ui.py

+    @asynch
+    def _update_context(self, context_value: str) -> None:
+        self._context = context_value
+        self.set_footer_text(context_change=True)
+
+    context = property(lambda self: self._context, _update_context)


Does this need to be in a new thread?

neiljp · 2024-04-24T22:48:18Z

zulipterminal/ui.py

+        if context_change and self._is_footer_event_running:
+            return


The idea here is that we don't interrupt a running event, ie. timed-duration message, with a context change?

Paraphrasing the logic: if there's no context change (ie. previous to this commit), or a footer event isn't running, we continue with the previous behavior. That seems good, though can we use the new state boolean you added in the previous commit to simplify the existing logic at that point? That would make the previous commit a useful refactor, and this a more clear standalone change that only adds the context_change part.

neiljp · 2024-04-24T23:11:55Z

tests/ui/test_ui.py

+        if event_running:
+            view.frame.footer.set_text.assert_not_called()
+            view.controller.update_screen.assert_not_called()
+        else:
+            view.frame.footer.set_text.assert_called_once_with(["some help text"])
+            view.controller.update_screen.assert_called_once_with()


With this split, it's pretty clear that this should be two separate tests.

It's occasionally reasonable to have these conditionals for complex multi-parameter inputs, but you only have a boolean here :)

One alternative is to simplify the mock asserts to be a bool; it depends how important the parameters to the call being asserted are.

neiljp · 2024-04-25T22:08:20Z

zulipterminal/ui.py

        text_list: Optional[List[Any]] = None,
        style: str = "footer",
        duration: Optional[float] = None,
+        context_change: Optional[bool] = False,


This has become an increasingly complex (threaded!) function, which stores various state in the UI and the view, and has different calling styles depending upon the usage.

After this commit, I believe we'll support calling this with:

no parameters -> reset to show random value

specific fixed text/style (eg. autocomplete) -> show text

specific fixed text/style with a duration -> show text, then later call with no parameters

context change -> show different random value?

However, we can't generally mix and match all these parameters, or at least the new one? It's also unclear by the name, that no parameters does a 'reset' to show the default/random value.

For now, the best change would be to ensure using assert(s) that all calls have the correct combination of parameters, and extend it to incorporate context_change in this commit.

For the future this may warrant refactoring to explicitly cover the different uses and state for this.

neiljp · 2024-04-26T00:14:09Z

I know you have lots of ideas and are getting code out well, but I'd suggest being cautious regarding pushing too far in one direction. It can be better to have work in another area to work on in parallel, rather than needing to heavily prune/drop/adjust a single direction if work in that one area takes a different path.

keys: Add a context field to Key Binding.

b56958b

zulipbot added the size: XL [Automatic label added by zulipbot] label Apr 18, 2024

Niloth-p added 10 commits April 22, 2024 23:32

keys: Add a dict of hotkey contexts.

9266ddc

keys: Use the help context in generating random help tips.

2ffd1d7

Tests updated.

keys: Include previously excluded random help hints.

3f5bf3e

Hotkeys that used to be excluded from general help hints, can be useful as hints in particular contexts.

ui: Add state variable to track live footer events.

90e3b61

Tests updated.

ui: Enable footer texts to display contextual help tips.

533fdb4

- Add a context property. - Update footer text on context changes, if a footer event is not already in progress. Tests added.

conftest: Add a fixture to mock help context.

c31f25b

boxes: Update context on opening compose box.

a90685b

Tests updated.

views: Update context on accessing message search editor.

5976cb6

Tests updated.

core/views/boxes: Update context on entering/exiting editors.

5ae87bc

Set context to "editor" only if it is not already set to more specific editor contexts. Reset context to "" on exiting any editor. Tests added and updated.

views: Update context on reaching user list with right arrow key.

c6b4579

Tests added.

Niloth-p force-pushed the 1484-introduce-help-context branch from b75f6f5 to 04c2f4f Compare April 23, 2024 19:26

Niloth-p added 8 commits April 24, 2024 21:16

views: Update context on pressing left key from middle column.

a115ad5

Check if the current focus is on - the menu view -> set context to 'general' - the stream list - the topic list Tests added.

buttons: Update context on toggling topics list.

eee99fb

Tests updated.

ui: Assign focused view as context when reset to empty string.

68f8072

Tests added.

core: Reset context on exiting popups.

10492f3

Tests added.

core: Update context on opening stream and msg info popups.

1049fd5

Tests added.

ui: Update context when menu buttons are used.

1c762a4

Tests added.

views: Update context on entering message view using arrow keys.

2718d67

Tests added.

views: Update context on moving between menu buttons and stream list.

5cd1582

Tests added.

Niloth-p force-pushed the 1484-introduce-help-context branch from 04c2f4f to 5b38003 Compare April 24, 2024 17:17

Niloth-p force-pushed the 1484-introduce-help-context branch from 5b38003 to 0a1909c Compare April 24, 2024 17:34

Niloth-p marked this pull request as ready for review April 24, 2024 19:18

Niloth-p commented Apr 24, 2024

View reviewed changes

neiljp reviewed Apr 25, 2024

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Apr 26, 2024

Niloth-p mentioned this pull request May 4, 2024

boxes/views: Adopt urwid_readline for all editors. #1492

Merged

18 tasks

Niloth-p mentioned this pull request May 9, 2024

Minor improvements in lint hotkeys #1482

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce help context #1484

Introduce help context #1484

Niloth-p commented Apr 18, 2024

Niloth-p left a comment

Niloth-p Apr 24, 2024

neiljp Apr 24, 2024

Niloth-p Apr 24, 2024

neiljp Apr 26, 2024

neiljp left a comment

neiljp Apr 19, 2024

Niloth-p Apr 26, 2024

neiljp Apr 19, 2024

Niloth-p Apr 26, 2024 •

edited

neiljp Apr 19, 2024

neiljp Apr 19, 2024

neiljp Apr 24, 2024

neiljp Apr 24, 2024

neiljp Apr 24, 2024

neiljp Apr 24, 2024

neiljp Apr 24, 2024

neiljp Apr 25, 2024

neiljp commented Apr 26, 2024

Introduce help context #1484

Are you sure you want to change the base?

Introduce help context #1484

Conversation

Niloth-p commented Apr 18, 2024

What does this PR do, and why?

External discussion & connections

How did you test this?

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

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

Choose a reason for hiding this comment

Niloth-p Apr 26, 2024 • edited

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

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

Choose a reason for hiding this comment

neiljp commented Apr 26, 2024

Niloth-p Apr 26, 2024 •

edited