Add manual cycling of random hints in footer #1517

Niloth-p · 2024-06-13T12:04:27Z

What does this PR do, and why?

Enables cycling through random hints in the footer.

Using a hotkey to manually go to next hint
Periodically cycle through footer hints automatically

This is in preparation to #1484, where we add contexts and the footer displays only the context-specific hotkey hints.

Outstanding aspect(s)

If this is good, the next step could involve making the footer's cycle period a user setting.
Deciding the hotkey to go to next hint: I've currently used Tab, but that's subject to change on discussion.
Deciding the default cycle period: I've currently set it to 30 seconds, but we could increase it.

External discussion & connections

Discussed in #zulip-terminal in Adding context to hotkeys, Cycling through footer hints, T1484's review
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

zulipbot · 2024-06-13T13:43:36Z

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

rsashank

LGTM! The use of freezegun is an interesting approach. Tested it manually too, works as intended 👍

Well done, @Niloth-p! I've left a few comments, nothing major though.

rsashank · 2024-06-16T20:35:16Z

docs/hotkeys.md

@@ -13,6 +13,7 @@
 |Redraw screen|<kbd>Ctrl</kbd> + <kbd>l</kbd>|
 |Quit|<kbd>Ctrl</kbd> + <kbd>c</kbd>|
 |Show/hide user information (from users list)|<kbd>i</kbd>|
+|Next footer hint|<kbd>Tab</kbd>|


NIT: Since the footer hints are random, should we rename this to "New footer hint" or "Random footer hint" instead?

That said, "Next footer hint" seems perfectly fine too.

Sure, we could totally go with that. That would be better indeed, at this point.
I was actually thinking of adding functionality to go back to the previous hint as well, but it's too early to decide, I suppose, as it would depend on other features that are yet to be implemented like the hint priority field.

Oh, that makes sense. In that case, it's probably best to keep it as is.

Nah, I've added a new commit updating it already :)
We could change it to "Next" if and when we add the functionality for going to previous hint.

rsashank · 2024-06-18T08:09:51Z

tests/ui/test_ui.py

+            # Trigger the alarm
+            alarm_callback = mocked_alarm.call_args[0][1]
+            alarm_callback(None, None)


If I understand correctly, line 123 is simulating the alarm going off (?) and triggering the callback function. (Correct me if I'm wrong)

Could we make the comment on that line more specific about this to prevent any confusion? We could start the comment with # NOTE: similar to how it's done elsewhere.

Yes, the comment is meant to convey that the code below is to trigger the alarm, i.e directly call the callback function. I wasn't completely sure if it's required, just something that I would personally find convenient, if I was reading the code.

Ok, I'll clarify that, thank you for the feedback. Would "Directly trigger the alarm's callback function" be a better description?

Ah, we do seem to have a few comments with #NOTE: in that file, but generally in the codebase, we don't use it that much. Not sure what the convention is regarding its usage in our repo. There doesn't seem to be a global convention. So, I'll wait to hear from the others regarding that.

Niloth-p

Thank you for the review, Sashank!

Niloth-p · 2024-06-18T10:22:19Z

tests/ui/test_ui.py

+            # Trigger the alarm
+            alarm_callback = mocked_alarm.call_args[0][1]
+            alarm_callback(None, None)


Yes, the comment is meant to convey that the code below is to trigger the alarm, i.e directly call the callback function. I wasn't completely sure if it's required, just something that I would personally find convenient, if I was reading the code.

Ok, I'll clarify that, thank you for the feedback. Would "Directly trigger the alarm's callback function" be a better description?

Ah, we do seem to have a few comments with #NOTE: in that file, but generally in the codebase, we don't use it that much. Not sure what the convention is regarding its usage in our repo. There doesn't seem to be a global convention. So, I'll wait to hear from the others regarding that.

Niloth-p · 2024-06-18T10:27:01Z

docs/hotkeys.md

@@ -13,6 +13,7 @@
 |Redraw screen|<kbd>Ctrl</kbd> + <kbd>l</kbd>|
 |Quit|<kbd>Ctrl</kbd> + <kbd>c</kbd>|
 |Show/hide user information (from users list)|<kbd>i</kbd>|
+|Next footer hint|<kbd>Tab</kbd>|


Sure, we could totally go with that. That would be better indeed, at this point.
I was actually thinking of adding functionality to go back to the previous hint as well, but it's too early to decide, I suppose, as it would depend on other features that are yet to be implemented like the hint priority field.

neiljp · 2024-06-29T06:52:21Z

I left some notes in the channel, but getting the first commit merged with an adjusted key would be great as a first step. The remainder could be useful, particularly since refactoring to make it clearer to understand could benefit if we keep the timed approach, but could also depend on the style of help hints we settle on when they're more contextual.

Added tests. Hotkeys document regenerated.

neiljp · 2024-07-04T22:55:13Z

@Niloth-p Thanks for the quick PR adjust, which I only just noticed. The only change I made was to make it clearer that the 'hint' being referred to is an regard to a hotkey :)

zulipbot added the size: L [Automatic label added by zulipbot] label Jun 13, 2024

Niloth-p force-pushed the 1517-footer-cycling/pr branch from 77bbb59 to f8d1ee5 Compare June 13, 2024 12:12

Niloth-p changed the title ~~Cycle through hints in footer~~ Add manual and automatic cycling of random hints in footer Jun 13, 2024

Niloth-p added PR needs buddy review area: UI General user interface update area: hotkeys labels Jun 13, 2024

rsashank reviewed Jun 18, 2024

View reviewed changes

rsashank removed the PR needs buddy review label Jun 18, 2024

Niloth-p commented Jun 18, 2024

View reviewed changes

Niloth-p added the PR needs mentor review label Jun 18, 2024

Niloth-p requested a review from zormit June 18, 2024 10:35

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

Niloth-p force-pushed the 1517-footer-cycling/pr branch from f9f076a to c74e3bb Compare July 2, 2024 11:38

zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jul 2, 2024

Niloth-p changed the title ~~Add manual and automatic cycling of random hints in footer~~ Add manual cycling of random hints in footer Jul 2, 2024

Niloth-p added PR needs mentor review and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 2, 2024

keys/ui: Add hotkey and command to generate new random footer hint.

0a11a56

Added tests. Hotkeys document regenerated.

neiljp force-pushed the 1517-footer-cycling/pr branch from c74e3bb to 0a11a56 Compare July 4, 2024 22:52

neiljp merged commit 967d762 into zulip:main Jul 4, 2024
21 checks passed

neiljp added this to the Next Release milestone Jul 4, 2024

Niloth-p deleted the 1517-footer-cycling/pr branch July 5, 2024 10:31

Niloth-p added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs mentor review labels Jul 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add manual cycling of random hints in footer #1517

Add manual cycling of random hints in footer #1517

Niloth-p commented Jun 13, 2024 •

edited

Loading

zulipbot commented Jun 13, 2024

rsashank left a comment •

edited

Loading

rsashank Jun 16, 2024

Niloth-p Jun 18, 2024

rsashank Jun 18, 2024

Niloth-p Jun 18, 2024

rsashank Jun 18, 2024

Niloth-p Jun 18, 2024

Niloth-p left a comment

Niloth-p Jun 18, 2024

Niloth-p Jun 18, 2024

neiljp commented Jun 29, 2024

neiljp commented Jul 4, 2024

Add manual cycling of random hints in footer #1517

Add manual cycling of random hints in footer #1517

Conversation

Niloth-p commented Jun 13, 2024 • edited Loading

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Self-review checklist for each commit

zulipbot commented Jun 13, 2024

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

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 commented Jun 29, 2024

neiljp commented Jul 4, 2024

Niloth-p commented Jun 13, 2024 •

edited

Loading

rsashank left a comment •

edited

Loading