-
-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce help context #1484
base: main
Are you sure you want to change the base?
Introduce help context #1484
Conversation
5b38003
to
0a1909c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 :)
zulipterminal/config/keys.py
Outdated
}, | ||
'CYCLE_COMPOSE_FOCUS': { | ||
'keys': ['tab'], | ||
'help_text': 'Cycle through recipient and content boxes', | ||
'key_category': 'msg_compose', | ||
'key_context': 'write_box', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
016ddf0
to
e4d57a3
Compare
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, great PR @Niloth-p! It adds a much-needed functionality and is definitely the holy grail of your GSoC proposal. It’s fantastic to see it in action!
I left some comments, mostly minor changes and NITs. Functionally, it LGTM—I played around with it a bit and found that it recommends Esc to clear the search bar when you're in the users list, even if there's no search active. There might be a few outliers like that. Is it feasible to fix those on an individual scale? (That's the only one I’ve found so far, but I’ll let you know if I come across any others.)
tools/lint-hotkeys
Outdated
@@ -26,6 +26,23 @@ HELP_TEXT_STYLE = re.compile(r"^[a-zA-Z /()',&@#:_-]*$") | |||
KEYS_TO_EXCLUDE = ["q", "e", "m", "r", "Esc"] | |||
|
|||
|
|||
def read_help_categories() -> Dict[str, List[Tuple[str, List[str]]]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I feel we can squash this and the previous commit. The previous commit seems redundant since you're refactoring the flow and removing the function anyways.
refactor: lint-hotkeys: Improve variable naming of generated dict.
lint-hotkeys: Refactor the flow by creating new ENTRY_BY_CATEGORIES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting, but we're not actually removing the function, it's just moved to the top to be placed before creating the new ENTRY_BY_CATEGORIES.
Changed the brackets used for the hotkey. Removed the colon.
Added function reset_footer_text() to reduce overloading of set_footer_text(), and updated its every usage. Updated tests.
By adding functions - set_footer_text_for_event() - set_footer_text_for_event_duration() and updating their usage. Updated tests.
To reduce redundancy in error messages for scenarios with several linting errors, collect all errors before printing them with the hint to resolve the errors. Also add a \n at the end of error messages.
Part of adding generic variables in preparation for contexts.
Part of adding generic variables in preparation for contexts.
Replace 'key_category' with 'key_group' and 'key_group_value'. Add type KeyGroup to enforce typing. Part of adding generic variables in preparation for contexts.
This commit only updates the user-facing strings and variables, it does not update the occurrences in the comments or docstrings. Part of adding generic variables in preparation for contexts.
Part of adding generic variables in preparation for contexts.
Use CATEGORY_OUTPUT_FILE instead of the module constant OUTPUT_FILE. Use the smaller case 'output file' instead, in docstrings. Use existing_file as the name of the file object. Part of adding generic variables in preparation for contexts.
Convert string entries of `key_group` into lists, to later support the list of strings `key_contexts` with the same code.
Add a TypedDict bundling the values associated with category/context, and a dictionary mapping the group to its bundle. Pass group argument to all functions, to use the respective 'bundled' values associated with that group.
Add a dict of supported hotkey contexts for key bindings. Used a dict, instead of a list to facilitate the mapping of internal names to user-friendly names. Added TODO comments marking key bindings that require condition checks. Tests updated. Linting added.
Tests added and updated.
Hotkeys that used to be excluded because they were too specific, can now be useful as hints in particular contexts.
- Add a context property. - Update footer text on context changes, if a footer event is not already in progress. Tests added.
Return a valid display context name from commands_for_random_tips(), and use in the footer. Tests added and updated.
using a customized urwid.MainLoop. Added a new file contexts.py that implements the MainLoop that will track the context through the focus path. Developer overview document regenerated. Test updated.
Assigned an arbitrary hotkey. Will be overloaded onto the HELP command. Help Menus do not currently work from within editors or popups. Hotkeys document regenerated.
e4d57a3
to
e79f68b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the testing it out and I appreciate the great suggestions, @rsashank!
recommends Esc to clear the search bar when you're in the users list, even if there's no search active
Yes, good spot. Those need some extra logic to handle. There are at least 3 key bindings of that sort that need condition checks, and for all of them, I'd attached a comment # TODO: condition check required
at the bottom of their key bindings, until we come up with a smooth solution for handling them.
But, what I failed to consider was that since the comment is just for the devs, from the user POV, the hints would just seem bad hints.
So, I've currently turned off those key bindings from being used in random hints until the condition checks are added. Hope that's a good way of handling it.
Please do share any other observations as you come across them :)
Updates to the code:
- Addressed Sashank's feedback, by adding the changes to the earlier commits and updating them gradually in sync with the changes in the later commits.
- Rebased onto main, and added context field for the new OPEN_EXTERNAL_EDITOR command.
@@ -104,13 +104,13 @@ def get_random_help(self) -> List[Any]: | |||
# Get random allowed hotkey (ie. eligible for being displayed as a tip) | |||
allowed_commands = commands_for_random_tips() | |||
if not allowed_commands: | |||
return ["Help(?): "] | |||
return ["Help[?] "] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideas on how we can further highlight the differentation of the Help and its hotkey from the random hint are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a space in the first instance? Then it'd be similar to eg. the Search [<key>]
formatting we have right now. My other concern when running it is that we end up with
Help[?] OTHERKEY, KEY HELP TEXT FOR HINT
In that, the hint looks as close to the hint description as to the help hint, so we could add an extra space?
However, without that latter extra space, I'd still consider it an improvement, since the :
is confusing by suggesting the Help is the hint, I think?
In the longer-term I mentioned highlighting hotkeys consistently, which might help here, but that's beyond the remit of this commit I think :)
zulipterminal/config/keys.py
Outdated
if len(random_tips) == 0: | ||
return commands_for_random_tips("general") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case has not been addressed in my update either. I'm not sure when we would have no hotkeys under the (now) "global" context. We will always have at least the Ctrl C hotkey, so it should be fine?
GroupedHelpEntries = Dict[str, List[Tuple[str, List[str]]]] | ||
|
||
|
||
def read_help_groups(key_group: KeyGroup) -> GroupedHelpEntries: | ||
""" | ||
Generate a dict from KEYS_FILE | ||
key: help category | ||
value: a list of help texts and key combinations, for each key binding in that help category | ||
key: help category/context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the word "groups" for the variables.
But, mention category/context in all the docstrings.
I hope everyone's fine with calling categories & contexts with the common name groups 🤞, praying for fewer merge conflicts 😅
tools/lint-hotkeys
Outdated
@@ -26,6 +26,23 @@ HELP_TEXT_STYLE = re.compile(r"^[a-zA-Z /()',&@#:_-]*$") | |||
KEYS_TO_EXCLUDE = ["q", "e", "m", "r", "Esc"] | |||
|
|||
|
|||
def read_help_categories() -> Dict[str, List[Tuple[str, List[str]]]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting, but we're not actually removing the function, it's just moved to the top to be placed before creating the new ENTRY_BY_CATEGORIES.
Update the contextual help menu to use the parent contexts as well. Add linting for parent contexts mapping.
e79f68b
to
3513d73
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Niloth-p I'm not sure if this was 34 commits before, but it certainly makes it challenging to review as it is - it can be intimidating just to see there are 34, at least ;) The commits aren't difficult to read in most cases, which helps 👍
I started focusing on the early commits, since they have potential for simple refactoring too (and that bug?). However, as per your current PR intro you then have over 10 commits for tidying the linter, presumably from the other previous PR, before then extending that to work to prepare for the contextual help.
So, I've focused this review on those earlier commits, then mostly towards the end, since the early part may be merged sooner and the latter might be simplified. Getting some commits merged will make this easier to handle!
The approach you're using for setting context does seem to work, but I see I mentioned the class variables idea before. What do you think?
@@ -104,13 +104,13 @@ def get_random_help(self) -> List[Any]: | |||
# Get random allowed hotkey (ie. eligible for being displayed as a tip) | |||
allowed_commands = commands_for_random_tips() | |||
if not allowed_commands: | |||
return ["Help(?): "] | |||
return ["Help[?] "] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a space in the first instance? Then it'd be similar to eg. the Search [<key>]
formatting we have right now. My other concern when running it is that we end up with
Help[?] OTHERKEY, KEY HELP TEXT FOR HINT
In that, the hint looks as close to the hint description as to the help hint, so we could add an extra space?
However, without that latter extra space, I'd still consider it an improvement, since the :
is confusing by suggesting the Help is the hint, I think?
In the longer-term I mentioned highlighting hotkeys consistently, which might help here, but that's beyond the remit of this commit I think :)
"Help(?): ", | ||
"Help[?] ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this method, it would be useful to refactor this common text at the same time?
zulipterminal/ui.py
Outdated
if duration is not None: | ||
assert duration > 0 | ||
time.sleep(duration) | ||
self.reset_footer_text() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This assert didn't make it across the refactor.
zulipterminal/ui.py
Outdated
@asynch | ||
def set_footer_text( | ||
self, | ||
text: List[Any], | ||
style: str = "footer", | ||
duration: Optional[float] = None, | ||
) -> None: | ||
def set_footer_text(self, text: List[Any], style: str = "footer") -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this function need to be @asynch
now? Would delegating that simplify the problem we had?
zulipterminal/ui.py
Outdated
def set_footer_text_for_event(self, text: List[Any], style: str = "footer") -> None: | ||
self.set_footer_text(text, style) | ||
|
||
def set_footer_text_for_event_duration( | ||
self, text: List[Any], duration: float, style: str = "footer" | ||
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reading this, it's unclear to me what the 'event' refers to:
- there's no context for why (most) calls now use a
set_footer_text_for_event*
variant - you add a non-'event' variant in the contextual-help commits (much) later
- if there will be another variant name-wise, this would be a better rename much closer to that addition, otherwise it looks confusing among these commits
- there are other calling points that don't use these names; should they? (see another comment)
class FocusTrackingMainLoop(urwid.MainLoop): | ||
def __init__( | ||
self, | ||
widget: urwid.Widget, | ||
palette: ThemeSpec, | ||
screen: Optional[urwid.BaseScreen], | ||
) -> None: | ||
super().__init__(widget, palette, screen) | ||
self.previous_focus_path = None | ||
self.view = widget | ||
|
||
def process_input(self, input: List[str]) -> None: | ||
super().process_input(input) | ||
self.track_focus_change() | ||
|
||
def track_focus_change(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem straightforward to track focus-change events via a derived class here, to ensure it is called after each input event 👍
However, while it may be due to the other complexity, I'm not clear if we need the rest of the code to explicitly be in the loop class?
PARENT_CONTEXTS: Dict[str, List[str]] = { | ||
"global": [], | ||
"general": ["global"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this commit is interesting, but it seems to make the context help popup rather more confusing, perhaps due to the large number of general global keys.
Another point after looking at the 'path' code, is whether this would lose too much by being dynamic using the class constants? eg. if the path found various known terms, it could fetch help from them all?
def __init__(self, controller: Any, title: str) -> None: | ||
def __init__( | ||
self, controller: Any, title: str, context: Optional[str] = None | ||
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails to toggle properly with contextual help.
def set_footer_text_for_event_duration( | ||
self, text: List[Any], duration: float, style: str = "footer" | ||
) -> None: | ||
self.set_footer_text_for_event(text, style) | ||
time.sleep(duration) | ||
self.reset_footer_text() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not asynch, so blocks for that duration. This breaks existing behavior. For example, try editing an old message that you cannot - the UI just stops updating, before catching up.
We shouldn't make everything asynch, as that puts everything in a thread and makes it a lot more complex, which was one benefit to the existing approach. However, this needs resolving.
("footer_contrast", f" {random_command_display_keys} "), | ||
f" {random_command['help_text']}", | ||
f" ({tip_context}) ", | ||
( | ||
"footer_contrast", | ||
f" {random_command['help_text']} ", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows the information quite well at this stage 👍
Later, I can see we'll want to iterate on the layout - there's a similar issue as with the Help [?]
point, but also that the context/keys/descriptions 'bounce around' spacing-wise.
What does this PR do, and why?
Refactors
set_footer_text()
of core.pyRefactors
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:
Rewrite Lint-hotkeys: general improvements (3)
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.
Inculcates the above commits.
Adding contexts + linting:
Use contexts in footer hint:
Track context switching
Contextual Help Menu
Parent Contexts
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)
Improved UI design of footer hint
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:
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.
FAQ and Assumptions
FAQ:
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.
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.
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:
key_category
/key_context
in the key bindings?test_commands_for_random_tips()
is quite overloaded. It is non-trivial and tests for several different things with very tight intersectional test cases:External discussion & connections
Context Based Help Tips
andAdding context to hotkeys
How did you test this?