Make the ALL_MESSAGES command work globally
What does this PR do, and why?
The keys for 'all messages' (a, Esc) did not work reliably except when a message is selected.
This PR makes its behavior consistent with the other menu button narrows - P, #, f.
This is implemented by first performing a refactor, to start tracking whether the list views are filtered or not. Other approach and implementation details are part of the commit descriptions. Several approaches were attempted before arriving at this one.
Outstanding aspect(s)
- Checking and verifying that I have not missed any edge cases is quite welcome.
Miscellaneous reasoning details:
- Splitting the GO_BACK commands into several commands is done in a separate PR, as the motivation for that PR is quite different, and does not directly tie in with this PR.
- The enum is placed in helper.py to avoid circular import issues.
- Since it's only more stringent conditional checks, I haven't particularly added tests for the 2nd commit.
- I have assigned higher priority to use
Escfor RESET_SEARCH op over the ALL_MESSAGES command, when both are possible, as it is the only key to RESET_SEARCH. And since the focus is on the current panel, it also makes more sense to use that context. - The Emoji Picker's search box doesn't matter to setting the changes for this PR - we don't want to track if it's filtered or not, we could leave the empty_search variable, but for the sake of uniformity and to completely remove all occurrences of empty_search, I have applied the same search_status refactor to it.
External discussion & connections
- [x] Discussed in #zulip-terminal in
#zulip-terminal > Make the ALL_MESSAGES command work everywhere - [x] Fully fixes #1505
- [ ] 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?
- [x] 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
- [x] It is a minimal coherent idea
- [x] It has a commit summary following the documented style (title & body)
- [x] It has a commit summary describing the motivation and reasoning for the change
- [x] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit
Changes
Before:
a key
- If in message view, go to home view centering on that message
Esc key
- If in popup, exit the popup
- If in compose box, close it
- If in message view, reset search, go to home view centering on that message
- If in search box or stream/topic/user list, reset search and list, go to last selected button or the first button in the list
After:
a key
- If in message view, go to home view centering on that message
- (NEW) Else, go to home view's last read message
Esc key
- If in popup, exit the popup
- If in compose box, close it
- If in message view, reset search, go to home view centering on that message
- (Made stringent) If in search box or filtered stream/topic/user list, reset search and list, go to last selected button or the first button in the list
- (NEW) If no search to reset, go to home view's last read message
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!
LGTM, great work @Niloth-p and interesting approach!
Just a few comments for clarification. I've tested it manually, and it works well!
Fixed the bug.
Manual Testing cases:
Pressing Esc from
- menu button
- lists - user list, topic list, stream list
- full list
- filtered list (after a search)
- search boxes
- filter multiple lists, and test Esc resets only that particular filtered list, and does not go to Combined Feed.
Maybe also the empty list functionality, since we're refactoring that.
Should we be adding UI tests for these?