Make the ALL_MESSAGES command work globally #1512

Niloth-p · 2024-06-06T03:18:13Z

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 Esc for 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

Discussed in #zulip-terminal in #zulip-terminal > Make the ALL_MESSAGES command work everywhere
Fully fixes 'All messages' are not consistently accessible compared to P/#/f #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?

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

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

zulipbot · 2024-06-06T14:58:16Z

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!

rsashank · 2024-06-06T17:33:51Z

zulipterminal/helper.py

+class SearchStatus(Enum):
+    DEFAULT = 0
+    FILTERED = 1
+    EMPTY = 2
+


This is a really interesting approach!

I think it would be great to include a docstring here as well, explaining this a bit?

I didn't add any because none of the other classes in helper.py have docstrings.
What aspect do you think this could be more clear about? We could rename it too.

Seems fair. I was just thinking it might be confusing for people looking at the code later, so a docstring would help clarify it. However, now that you mention it, it might not be necessary.

Good naming > docstrings/comments, but it could be helpful if the naming can't be easily improved - and it's good not to block on getting perfect names ;)

zulipterminal/ui_tools/boxes.py

rsashank · 2024-06-06T18:09:08Z

LGTM, great work @Niloth-p and interesting approach!

Just a few comments for clarification. I've tested it manually, and it works well!

Niloth-p

Thank you for the very quick review, @rsashank!

zulipterminal/ui_tools/boxes.py

Niloth-p · 2024-06-07T16:04:03Z

zulipterminal/helper.py

+class SearchStatus(Enum):
+    DEFAULT = 0
+    FILTERED = 1
+    EMPTY = 2
+


I didn't add any because none of the other classes in helper.py have docstrings.
What aspect do you think this could be more clear about? We could rename it too.

zormit

I haven't got very far with the review yet, but I'll post this anyways, so you can engage with it. The only edge case that I could find so far is this (maybe there are similar cases in other places):

in the left column, view the topics of a channel (t)
apply a filter there (q + some text + Enter)
then press Esc

What happens is that both the filter gets dropped and the center column switches to All Messages. I think that's too much at once. I would assume Esc only removes the topic filter at that point (while a, already correctly, switches to All Messages only and doesn't touch that topic filter).

You're saying in the PR description

I have assigned higher priority to use Esc for 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.

And it seems to be about that topic. I haven't investigated the code yet, but it doesn't seem to work as described in the topic search at least. Not sure if that corresponds to RESET_SEARCH or something else.

(i've also left one other tiny comment in the code)

zormit · 2024-06-27T20:29:27Z

zulipterminal/ui_tools/views.py

-            self.empty_search = len(users_display) == 0
+            self.search_status = (
+                SearchStatus.EMPTY if len(users_display) == 0 else SearchStatus.FILTERED
+            )

            # FIXME Update log directly?


I'm wondering if this FIXME is now obsolete. I'm not sure on the idea behind that comment, but going towards an Enum seems to not allow updating log directly anymore anyways.

I'm not certain but imo it should be fine.
This enum merely replaces the boolean that was already being used there, so it shouldn't add any new restrictions?

I assume that the comment (added in this commit) means moving some of the logic from RightColumnView to UsersView which holds log. And this should be fine, as we already support TopicsView and StreamsView with the new enum, instead of the LeftColumnView. So, it would probably require just moving the enum to a different class.

Niloth-p · 2024-06-28T06:36:28Z

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?

The existing empty_search boolean property of views with search boxes is replaced by an enum property search_status that supports the states - DEFAULT, FILTERED and EMPTY. This allows tracking whether the results are filtered or not as well, without introducing a separate boolean property for that. Updated tests.

Added conditional checks to ensure that a search was previously performed before resetting search, using the newly added search_status.

This command worked only when a message was selected, using it as an anchor to fetch messages. Now, it has been made consistent with the other narrow commands, to work from any panel. This could not be implemented in ui.py along with other narrow commands, because of the conflict caused by the Esc key also being assigned to reset search in the side panels (GO_BACK command). When both operations, 1. reset search in the current panel view 2. narrow to all messages are possible, pressing `Esc` is set to trigger only the reset search operation and not the ALL_MESSAGES command. The next keypress of `Esc` will go to the home view once the current panel view is restored to its default state. Fixes zulip#1505.

zormit

This looks good to me, thank you for working on this. I think this feature will improve the UI.

I can confirm the manual tests. I have added a comment about potential architecture refactoring, but I think this would be orthogonal to this PR.

Regarding UI-Tests: If it's easy to add them, I'd say yes, as this will enable said refactoring without worrying too much. But I think we don't have a good test infrastructure for testing this from the "outside perspective"? Would be curious about Neil's perspective on this.

zormit · 2024-08-05T14:17:40Z

zulipterminal/ui_tools/views.py

@@ -436,7 +447,7 @@ def __init__(
            header=self.header_list,
        )
        self.search_lock = threading.Lock()
-        self.empty_search = False


I think it's not on this PR to solve this, but it's easy to notice here: It's unfortunate that there's a lot of duplication between all the different views. A similar logic change repeated three times.

I'm currently not sure what could be a good way to refactor this. Maybe a class that knows the view state, handles the keypresses, and forwards back to the view what should happen based on some interface.

neiljp

@Niloth-p I left a few comments, but I need to get to bed, so will have to look again. I've not manually tested, but the behavior sounds good 👍

neiljp · 2024-08-23T07:18:55Z

zulipterminal/helper.py

+class SearchStatus(Enum):
+    DEFAULT = 0
+    FILTERED = 1
+    EMPTY = 2


The enum is an improvement overall 👍 It acknowledges that there are actually three distinct states for the search status in the code.

Re implementation in this commit structure:

this commit tracks it, but doesn't do anything with the extra state (cf. we track empty so we can display something different)

this commit could migrate to an enum, then extend the enum in another commit, when you need to track the other state?

it's strange in general seeing new asserts for FILTERED, but fewer changes for EMPTY, and none for DEFAULT; doing the split as per the above bullet would make it clearer where new state tracking was added and extra asserts added, vs renaming.

For the enum element names:

DEFAULT is OK, but in terms of a search-status, it's not as clear as it could be if that's search-on or search-off. UNFILTERED is maybe too close to FILTERED, but is closer to the meaning of it?

EMPTY is rather more FILTERED_EMPTY? We could end up in a state where it's "UNFILTERED_EMPTY" (DEFAULT_EMPTY), but I don't think the code covers that case?

Re location, helper.py is OK, though I'd prefer we reduce elements in here since 'helper' is such a generic name :)

neiljp · 2024-08-23T07:20:03Z

zulipterminal/helper.py

+class SearchStatus(Enum):
+    DEFAULT = 0
+    FILTERED = 1
+    EMPTY = 2
+


Good naming > docstrings/comments, but it could be helpful if the naming can't be easily improved - and it's good not to block on getting perfect names ;)

zulipbot added the size: XL [Automatic label added by zulipbot] label Jun 6, 2024

Niloth-p force-pushed the 1512-global-all-msgs/pr branch from 489b5e2 to d3297e2 Compare June 6, 2024 03:19

Niloth-p added bug Something isn't working area: hotkeys PR needs review PR requires feedback to proceed area: refactoring PR needs buddy review PR needs mentor review labels Jun 6, 2024

rsashank reviewed Jun 6, 2024

View reviewed changes

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

Niloth-p commented Jun 7, 2024

View reviewed changes

Niloth-p removed the PR needs review PR requires feedback to proceed label Jun 10, 2024

Niloth-p requested a review from zormit June 10, 2024 04:12

Niloth-p force-pushed the 1512-global-all-msgs/pr branch from d3297e2 to ac14d88 Compare June 26, 2024 01:10

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Jun 26, 2024

zormit reviewed Jun 27, 2024

View reviewed changes

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 28, 2024

Niloth-p force-pushed the 1512-global-all-msgs/pr branch 2 times, most recently from a055908 to 1e57902 Compare June 28, 2024 06:05

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 Jun 28, 2024

Niloth-p added 3 commits July 18, 2024 20:58

views/boxes: Reset search only after verifying search status.

4f15cac

Added conditional checks to ensure that a search was previously performed before resetting search, using the newly added search_status.

Niloth-p force-pushed the 1512-global-all-msgs/pr branch from 1e57902 to 2525a79 Compare July 18, 2024 15:30

Niloth-p requested a review from zormit July 18, 2024 15:31

zormit approved these changes Aug 5, 2024

View reviewed changes

zormit added PR needs review PR requires feedback to proceed and removed PR needs mentor review labels Aug 5, 2024

Niloth-p requested a review from neiljp August 7, 2024 02:29

neiljp reviewed Aug 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the ALL_MESSAGES command work globally #1512

Make the ALL_MESSAGES command work globally #1512

Niloth-p commented Jun 6, 2024 •

edited

Loading

zulipbot commented Jun 6, 2024

rsashank Jun 6, 2024

Niloth-p Jun 7, 2024

rsashank Jun 13, 2024

neiljp Aug 23, 2024

rsashank commented Jun 6, 2024

Niloth-p left a comment

Niloth-p Jun 7, 2024

zormit left a comment •

edited

Loading

zormit Jun 27, 2024

Niloth-p Jun 28, 2024

Niloth-p commented Jun 28, 2024 •

edited

Loading

zormit left a comment

zormit Aug 5, 2024

neiljp left a comment

neiljp Aug 23, 2024

neiljp Aug 23, 2024

Make the ALL_MESSAGES command work globally #1512

Are you sure you want to change the base?

Make the ALL_MESSAGES command work globally #1512

Conversation

Niloth-p commented Jun 6, 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

Changes

Before:

After:

zulipbot commented Jun 6, 2024

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

rsashank commented Jun 6, 2024

Niloth-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zormit 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

Niloth-p commented Jun 28, 2024 • edited Loading

Manual Testing cases:

zormit left a 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

Niloth-p commented Jun 6, 2024 •

edited

Loading

zormit left a comment •

edited

Loading

Niloth-p commented Jun 28, 2024 •

edited

Loading