zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard
verified publisher icon zulip

Introduce help context

Open Niloth-p opened this issue 1 year ago • 3 comments

What does this PR do, and why?

Refactors set_footer_text() of core.py Refactors lint_hotkeys heavily, in preparation for contexts. Introduces context to track the current focus. Uses context to generate relevant footer hints. Demonstrates a Contextual Help Menu and "parent contexts".

Commit Grouping

Commits grouped by headings, and by another type Includes references for motivation of some groups.

Footer updates:

  • ui: Make the Help hotkey suggestion in the footer more conventional. (1)
  • refactor: ui/core/boxes: Separate out function to reset footer text. (2)
  • refactor: ui/core: Separate footer events from set_footer_text(). (2)
  • ui: Add state variable to track live footer events. (2)

Rewrite Lint-hotkeys: general improvements (3)

  • lint-hotkeys: Handle the doc file does not exist exception.
  • refactor: lint-hotkeys: Remove the write_hotkeys_file() function.
  • refactor: lint-hotkeys: Make the integer error flag boolean. (edited commit from #1518)
  • refactor: lint-hotkeys: Rewrite function docstrings and comments.
  • refactor: lint-hotkeys: Rename get_hotkeys_file_string().
  • refactor: lint-hotkeys: Rename ambiguous keyword "action" to "batch".
  • refactor: lint-hotkeys: Improve variable naming of generated dict.
  • lint-hotkeys: Refactor the flow by creating new ENTRY_BY_CATEGORIES.
  • refactor: lint-hotkeys: Delay generation of file string when linting.
  • refactor: lint-hotkeys: Split out the help text linting function.
  • refactor: lint-hotkeys: Split out the function that lints categories.
  • lint-hotkeys: Restructure the error messages of lint_help_text().
  • lint-hotkeys: Lint for typos in key_category values. (edited commit from #1518)

Refactor Lint-hotkeys: in preparation for linting contexts (3)

These commits share a common line at the bottom of their commit messages - "Part of adding generic variables in preparation for contexts." Once reviewed, we could squash these commits.

  • refactor: lint-hotkeys: Use help_group instead of HELP_CATEGORIES.
  • refactor: lint-hotkeys: Create generic variable entries_by_group.
  • refactor: lint-hotkeys: Create generic variables for key_category.
  • refactor: lint-hotkeys: Replace 'category(ies)' with 'group(s)'.
  • refactor: lint-hotkeys: Delete constant OUTPUT_FILE_NAME, compute it.
  • refactor: lint-hotkeys: Replace usage of OUTPUT_FILE.

Inculcates the above commits.

  • refactor: lint-hotkeys: Generalize the traversal of key group values.
  • refactor: lint-hotkeys: Use bundled group values.

Adding contexts + linting:

  • keys: Add a contexts field to Key Binding. (3)
  • lint-hotkeys: Add argument to generate a keys file grouped by contexts. (3)

Use contexts in footer hint:

  • keys: Use the help contexts in generating random help tips.
  • keys: Include previously excluded random help hints.
  • ui: Enable footer texts to display contextual help tips. (2)
  • ui/keys: Display the context of the footer hint. (1)

Track context switching

  • contexts/core/ui: Track the currently focused widget in the UI.

Contextual Help Menu

  • core/ui/views/keys: Add a Contextual Help Menu. (4)

Parent Contexts

  • keys/views: Add mapping of contexts to broader contexts they belong to. (3)(4)

Another dimension of grouping of commits:

The commits/groups marked with the suffix number in brackets denotes commits related to the following groups: (1) Footer Hint format (2) commits related to set_footer_text() (3) Linting commits (4) Contextual Help Menu (5) The contexts feature - all commits from the adding contexts part. The commits that do not depend on the contexts are obviously placed at the front, so all the commits after contexts have been introduced are all related to the contexts feature.

References: (corresponding to the numbered groups)

Outstanding Aspects:

Miscellaneous aspects

Feedback needed on all the naming changes. There are a lot.

I've rebased several times to get the order of commits just right. Hope they are sensible. Can freely be squashed after reviewing.

The contexts.py is added in a separate file, although we could put it inside another file. The logic flow is not very smooth (several if-else blocks), due to the differences in cases. Any suggestions on making it smoother would be helpful.

Tests have not been included for the context tracking. Guidance required on framing the tests and the setup.

We primarily need to:

  • test the mapping of focus paths to widgets
  • simulate focus on a widget, test context update

But, I'm having trouble setting up the scenarios.

The Contextual Help Menu is merely added to test the contexts feature. It needs a lot more work before it's ready for a merge.

  • To view only the hints specific to a context, run ZT after setting HEAD to the penultimate commit
  • To view all working hints for a specific context, run ZT with the entire PR.
FAQ and Assumptions

FAQ:

  1. Why bother this much with updating lint-hotkeys?

There was a feature in discussion - Rebinding/Remapping hotkeys, which require strong linting before users are allowed to customize their hotkeys. Thus, refactoring lint-hotkeys is a necessity.

  1. Why these many commits?

Since it's just a linting file, I was hoping we could get by with just the functionality, and not worry about the readability of the commits, as per my question here. But, it turns out linting is quite important for later features, so we do need this to be readable. And since all the changes are done to the same file, we need to do them step by step to ensure readability, unlike changes made to different files which can easily be squashed (eg: see the Parent Contexts commit (the last one) that introduces parent contexts, adds linting and contextual help menu in the same step). Any ideas for squashing while retaining readability are quite welcome. Or if I am holding high standards for what I consider readable, criticism is also welcome.

  1. Why all commits in the same PR?

The earlier commits are motivated by the requirements of the later commits, and their implementation strongly depends (in the case of lint-hotkeys refactoring) on the implementation of the later commits. The later commits can only be built on top of the earlier commits, so even if there were separate PRs, they'd need to be stacked, and merged in order.

Assumptions:

  • We want to generate the contexts.md file just for the devs and not the users. Several commits for lint-hotkeys operate under this assumption. Was unable to add an edited .gitignore with the contexts.md entry.

  • We want to start off with key_contexts instead of key_context. Why not introduce this later? Because there are some obvious discrepancies, e.g., the toggle topics hint would only be able to mapped to either the stream list or the topic list. See the context values for more such cases.

Follow-ups

Follow-ups:

  • [discuss] We currently allow the 'Tab' keypress to override any footer events. This can be controlled using the newly added variable that tracks live footer events.
  • [discuss] Allow opening the Help Menu from even within popups and editors.
  • [discuss] The design/formatting of the Contextual Help Menu
  • [discuss] Do we want to rename the fields key_category/key_context in the key bindings?
  • [discuss] I think that the testing function test_commands_for_random_tips() is quite overloaded. It is non-trivial and tests for several different things with very tight intersectional test cases:
    • the working of "excluded_from_random_tips"
    • usage of contexts
    • single vs multiple contexts
    • absence of context field
    • no tips in given context
    • the returned context value
  • Condition checks for vaildity of specific commands

External discussion & connections

  • [x] Discussed in #zulip-terminal in Context Based Help Tips and Adding context to hotkeys
  • [ ] Fully fixes #
  • [x] Partially fixes issue #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?

  • [x] 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)

Niloth-p avatar Apr 18 '24 16:04 Niloth-p

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.

neiljp avatar Apr 26 '24 00:04 neiljp

Hello @zulip/server-hotkeys, @zulip/server-refactoring members, this pull request was labeled with the "area: hotkeys", "area: refactoring" labels, so you may want to check it out!

zulipbot avatar Jul 18 '24 07:07 zulipbot

Heads up @Niloth-p, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot avatar Aug 20 '24 00:08 zulipbot