Handling Empty Narrows #1543

Niloth-p · 2024-08-05T09:18:01Z

What does this PR do, and why?

Fixes all the bugs related to empty narrows, and recipient bars when a message is not selected.

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes Crash for user with no subscriptions (eg. new bot) #895
Partially fixes issue Improve handling of narrows (views) with no messages #1506
Builds upon previous unmerged work in PR Sets current narrow marker for empty narrows. #278
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

Solution Space Exploration

Before settling on the dummy message approach, I explored a few other solutions as well:

I explored not switching narrows to empty narrows. We'd need to do some refactoring to get_message_ids_in_current_narrow to allow checking if a future narrow is empty, before switching to it.
And I also explored whether we could expand our system of handling narrows to also cater to narrows without messages, without handling it as an exception case and introducing a dummy message. I tried to move top_search_bar and recipient_header outside of MessageBox to change how they're tightly tied to the focused message, which is why they're not updated currently when in an empty narrow, and the erratic behavior.
Instead of a dummy message, I gave a shot at using a Text widget replacing the entire MessageView, like we do with the PanelSearchBoxes when there are no search hits. But yes, I ran into a few cases with focus issues.
I tried moving the Text element inside, and modifying/turning off the ModListWalker's action each time, to make it behave similar to the SimpleFocusListWalker, but that still wasn't a clean fix.

And other similar tangents, before realising the dummy message approach is the solid one, even if we were to implement any of these in the future, especially the 1st approach, the dummy message should be a backup.

I'm sure a more experienced person would have been able to figure this out much faster than I did, and without all this exploration, so I'd be interested in hearing the thinking process of someone who was able to / can narrow in on (pun intended) the dummy message solution right away.

I also decided against removing the message recipients section for empty narrows, because currently when opening the app, it shows up with "DONT HIDE", so I went with a solution that conforms to that behavior.

Outstanding aspect(s)

I've added the features for the dummy message, before adding the dummy message, so that it starts working as soon as added.
Placed the current narrow state tracking variable in the controller class.
I wasn't able to use even a single line of code as-is from Sets current narrow marker for empty narrows. #278, as this PR is quite different and uses several different decisions, so I just decided to add 278's author as a co-author for all ideas that were inspired from their PR.
I haven't added "bugfix" prefix to the commit messages, should I add it to the commit that wires the dummy message?

Next Steps

Adding tests for empty narrows. None of the commits currently have any new tests.
This introduces a bug. When you have an empty search narrow. And you receive a message event for that narrow, the message does not display until you restart the application, although it adds to the unread count.
I didn't want to hold back on the reviews any longer, so I've pushed the PR and will look into this soon.

Follow-ups

When activating user buttons, check if there exist previous DMs with the user, and if not, just open the compose box and do not switch to that narrow.
When a new message arrives in an empty DM narrow, the recipient header (above the message, not the one in the recipient bar) is not inserted newly. It automatically gets fixed when one more new message arrives. I think it's not a high priority bug, since the recipient bar already matches the recipient header for the DM case.

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

It was previously showing a text "DONT HIDE".

Co-authored-by: Sumanth V Rao <sumanthvrao@gmail.com>

Updated tests.

Co-authored-by: Sumanth V Rao <sumanthvrao@gmail.com>

Message type can only be "stream" or "private", as returned by the API. Simplified the flow to bunch if-else blocks together. Test removed.

Tests updated. Co-authored-by: Sumanth V Rao <sumanthvrao@gmail.com>

neiljp

@Niloth-p I know I replied in the channel, but wanted to follow up here here too.

Broadly this looks great, though there are queries over which specific parts we cover, eg. which keys dummy messages respond to. There are also possibly-missing aspects to consider sooner or later, such as how and when to narrow from a dummy message.

I think each specific feedback comment should be easy to follow, but let me know if not.

I'm increasingly thinking the empty-narrow might be best tracked in the model along with the 'current narrow' and associated data?

I think I addressed your outstanding aspects, but I'd add it only to the last one, and maybe only when it's close to ready to avoid spamming the issue ;) (see a recent czo topic!)

Oh, one thing I did find that I didn't note elsewhere is that the DM area doesn't indicate which recipients you're chatting with. This is a side-effect of how we handle the two sections at the top, but makes me wonder if the current-message-recipients should show the intended narrow/recipients, even if there are none. In a comment I wondered about treating the recipients as blank, but this would be clearer? Re DMs specifically, if I do enter in the user list to someone new, it usefully makes it empty, but as soon as I exit the user it treats it as some kind of generic narrow - x doesn't resume that user (which isn't indicated). I'm not sure what happens in other narrows?

neiljp · 2024-08-07T06:42:13Z

zulipterminal/ui_tools/messages.py

-        if is_command_key("REPLY_MESSAGE", key):
+        is_in_empty_narrow = self.model.controller.is_in_empty_narrow


If using the message as a placeholder, then I understand the need to disable lots of these hotkey behaviors. However, rather than disabling each of them individually, it would be a lot clearer to branch/exit separately, likely first.

Currently you seem to be allowing only the following:

start a stream message

narrow to all messages

Should we allow a new DM message (x) too?

Some actions you're excluding clearly don't make a lot of sense, when they have to act on a specific valid message, but I'm not sure about other cases, specifically narrowing.

Should all narrowing be possible from an empty narrow? Maybe a stream/channel narrow is empty, so we can try an all-messages narrow, but then what if that is itself empty? That suggests we can try different (absolute) narrows from any empty narrow?

How does that work 'without' a message id, ie. an empty narrow and 'fake' message? What does the message id fall back to in that case?

Nothing like this works before this PR, so the simplest solution might be to start from including no narrowing operations, and treat this as a followup after discussion.

neiljp · 2024-08-13T04:53:39Z

zulipterminal/ui_tools/boxes.py

-        self.msg_narrow = urwid.Text("DONT HIDE")
+        self.msg_narrow = urwid.Text("")


As I said in the channel, it's great to remove this line, though I think this was associated with the comment some lines above, which we could also remove? (look through the git log via git blame?)

In terms of this commit, it would be helpful to clarify under which conditions the application would display an empty recipient bar, since I very rarely actually see this odd text. This doesn't need to be via an exact analysis, but notes would be helpful, since looking back to this change in the future would help clarify what happens here.

neiljp · 2024-08-13T05:20:18Z

zulipterminal/ui_tools/boxes.py

-        self.msg_narrow = urwid.Text("DONT HIDE")
+        self.msg_narrow = urwid.Text("")


I was going to suggest taking advantage of this small commit to eg. refactor the attribute naming to be private. However, this is one of those coupled classes/objects, so many of these names are actually public, so this likely would be best settled with a separate refactoring effort.

In the absence of that, you could add comments to clarify each attribute for now; would that help you next time? If so, it'd help others who next read the code :)

neiljp · 2024-08-13T05:27:47Z

zulipterminal/core.py

@@ -598,6 +599,7 @@ def _narrow_to(self, anchor: Optional[int], **narrow: Any) -> None:
        if len(msg_id_list) == 0 or (anchor is not None and anchor not in msg_id_list):
            self.model.get_messages(num_before=30, num_after=10, anchor=anchor)
            msg_id_list = self.model.get_message_ids_in_current_narrow()
+        self.is_in_empty_narrow = bool(len(msg_id_list) == 0)


Was this small commit factored out, so that it would cover Sumanth's original process here? Oh, I think you said that it was where the inspiration came from?

(I'm torn whether this belongs in the model or controller here, but that's probably a refactoring to consider how to structure the whole narrowing behavior)

neiljp · 2024-08-13T06:54:32Z

zulipterminal/ui_tools/messages.py

-        if self.message["type"] == "stream":
+        if self.model.controller.is_in_empty_narrow:
+            return self.empty_narrow_header()
+        elif self.message["type"] == "stream":


This is a partial fix towards the issue? That is, we start to show a different message in the message header and the current-message header? Assuming so, the commit text could indicate that.

Does the new header structure need to follow the form of the others? I know it's rather hacky in places, so it's likely fine for now.

neiljp · 2024-08-13T14:52:29Z

zulipterminal/ui_tools/utils.py

+        dummy_message = {
+            "id": None,
+            "content": (
+                "No search hits" if model.is_search_narrow() else "No messages here"


I couldn't trigger 'No search hits' from here, and trying to do so without the last commit triggered a crash instead.

The 'No messages here' works fine as a placeholder for now, though I'm not sure of the text.

neiljp · 2024-08-13T15:04:09Z

zulipterminal/ui_tools/messages.py

@@ -198,8 +198,23 @@ def private_header(self) -> Any:
        header.markup = title_markup
        return header

+    def empty_narrow_header(self) -> Any:
+        title_markup = ("header", [("general_narrow", "No selected message")])


I'm wondering if it would be clearer to simply leave this blank?

neiljp · 2024-08-13T15:06:27Z

zulipterminal/ui_tools/utils.py

+            else None,
+            "subject": next(
+                (subnarrow[1] for subnarrow in model.narrow if subnarrow[0] == "topic"),
+                "No topics in channel",


This is possible, but difficult to achieve with the current model with Zulip - I think stream events would have to be absent. Did you manually test this?

neiljp · 2024-08-13T15:44:30Z

zulipterminal/ui_tools/utils.py

+    # Add a dummy message if no new or old messages are found
+    if not message_list and not last_msg:
+        message_type = "stream" if model.stream_id is not None else "private"
+        dummy_message = {


While I know that the MessageBox is designed to render a complex data type, I feel we should be able to avoid this kind of hack - this branch already adds some internal knowledge of a 'dummy' type to that class, so a 'make this a dummy' request on initialization might well make this a lot simpler too?

To clarify

I know that the function in this file is used a lot and it makes it simpler to centralize the calling point here

passing in a complex 'fake' message that has to have all the values to render properly seems more complicated than saying 'please make this a dummy with these parameters'?

neiljp · 2024-08-13T15:47:27Z

zulipterminal/ui_tools/boxes.py

-            self.controller.exit_editor_mode()
-            self.controller.search_messages(self.text_box.edit_text)
-            self.controller.view.middle_column.set_focus("body")
+            if self.controller.search_messages(self.text_box.edit_text):
+                self.controller.exit_editor_mode()
+                self.controller.view.middle_column.set_focus("body")
+            else:
+                self.controller.report_error(["No results found."])


I'm not sure if we want to take this approach rather than just have the empty result like the rest of this branch is pursuing, but it's an interesting extra change - I assume this is extra to the original version?

Niloth-p and others added 9 commits August 3, 2024 10:25

boxes: On opening application, display an empty recipient bar.

4992dbe

It was previously showing a text "DONT HIDE".

core: Track if the current narrow is empty.

936cf2f

Co-authored-by: Sumanth V Rao <sumanthvrao@gmail.com>

messages/views: Disable keypresses from dummy messages.

6cac57d

Updated tests.

messages: Customize recipient bar text for dummy messages.

f8503f8

messages: Turn off recipient header for dummy messages.

dcb213a

model: Replace the dummy message on receiving a new message.

0fb5bf0

Co-authored-by: Sumanth V Rao <sumanthvrao@gmail.com>

messages: Remove error handling for unreachable case.

df2feb4

Message type can only be "stream" or "private", as returned by the API. Simplified the flow to bunch if-else blocks together. Test removed.

refactor: messages: Allow datetime field of current message to be empty.

299a1c1

utils/messages: Add a dummy message for empty narrows.

3444552

Tests updated. Co-authored-by: Sumanth V Rao <sumanthvrao@gmail.com>

zulipbot added the size: XL [Automatic label added by zulipbot] label Aug 5, 2024

Niloth-p changed the title ~~Hnadling Empty Narrows~~ Handling Empty Narrows Aug 5, 2024

Niloth-p force-pushed the 1506-empty-narrows/pr branch from 08eccc4 to 24f7e2f Compare August 5, 2024 09:40

core/boxes: Flash footer message on search misses, without narrowing.

2a66297

Niloth-p force-pushed the 1506-empty-narrows/pr branch from 24f7e2f to 2a66297 Compare August 5, 2024 10:00

Niloth-p added bug Something isn't working area: message rendering PR needs buddy review labels Aug 5, 2024

Niloth-p requested a review from rsashank August 5, 2024 10:20

neiljp reviewed Aug 13, 2024

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Aug 13, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling Empty Narrows #1543

Handling Empty Narrows #1543

Niloth-p commented Aug 5, 2024 •

edited

Loading

neiljp left a comment •

edited

Loading

neiljp Aug 7, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

neiljp Aug 13, 2024

		if is_command_key("REPLY_MESSAGE", key):
		is_in_empty_narrow = self.model.controller.is_in_empty_narrow

		self.msg_narrow = urwid.Text("DONT HIDE")
		self.msg_narrow = urwid.Text("")

Handling Empty Narrows #1543

Are you sure you want to change the base?

Handling Empty Narrows #1543

Conversation

Niloth-p commented Aug 5, 2024 • edited Loading

What does this PR do, and why?

External discussion & connections

Solution Space Exploration

Outstanding aspect(s)

Next Steps

Follow-ups

How did you test this?

Self-review checklist for each commit

neiljp 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

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 commented Aug 5, 2024 •

edited

Loading

neiljp left a comment •

edited

Loading