Skip to content
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

Handling Empty Narrows #1543

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
11 changes: 11 additions & 0 deletions tests/ui_tools/test_messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def test_init(self, mocker, message_type, set_fields):
display_recipient=[
{"id": 7, "email": "[email protected]", "full_name": "Boo is awesome"}
],
id=457823,
stream_id=5,
subject="hi",
sender_email="[email protected]",
Expand All @@ -71,6 +72,7 @@ def test_private_message_to_self(self, mocker):
display_recipient=[
{"full_name": "Foo Foo", "email": "[email protected]", "id": None}
],
id=457823,
sender_id=9,
content="<p> self message. </p>",
sender_full_name="Foo Foo",
Expand All @@ -82,6 +84,7 @@ def test_private_message_to_self(self, mocker):
MODULE + ".MessageBox._is_private_message_to_self", return_value=True
)
mocker.patch.object(MessageBox, "main_view")

msg_box = MessageBox(message, self.model, None)

assert msg_box.recipient_emails == ["[email protected]"]
Expand Down Expand Up @@ -753,6 +756,7 @@ def test_main_view(self, mocker, message, last_message):
"color": "#bd6",
},
}
self.model.controller.is_in_empty_narrow = False
MessageBox(message, self.model, last_message)

@pytest.mark.parametrize(
Expand Down Expand Up @@ -1149,8 +1153,11 @@ def test_main_view_compact_output(
):
message_fixture.update({"id": 4})
varied_message = dict(message_fixture, **to_vary_in_each_message)
self.model.controller.is_in_empty_narrow = False
msg_box = MessageBox(varied_message, self.model, varied_message)

view_components = msg_box.main_view()

assert len(view_components) == 1
assert isinstance(view_components[0], Padding)

Expand All @@ -1160,7 +1167,9 @@ def test_main_view_generates_EDITED_label(
messages = messages_successful_response["messages"]
for message in messages:
self.model.index["edited_messages"].add(message["id"])
self.model.controller.is_in_empty_narrow = False
msg_box = MessageBox(message, self.model, message)

view_components = msg_box.main_view()

label = view_components[0].original_widget.contents[0]
Expand All @@ -1186,6 +1195,7 @@ def test_update_message_author_status(
):
message = message_fixture
last_msg = dict(message, **to_vary_in_last_message)
self.model.controller.is_in_empty_narrow = False

msg_box = MessageBox(message, self.model, last_msg)

Expand Down Expand Up @@ -1389,6 +1399,7 @@ def test_keypress_EDIT_MESSAGE(
to_vary_in_each_message["subject"] = ""
varied_message = dict(message_fixture, **to_vary_in_each_message)
message_type = varied_message["type"]
self.model.controller.is_in_empty_narrow = False
msg_box = MessageBox(varied_message, self.model, message_fixture)
size = widget_size(msg_box)
msg_box.model.user_id = 1
Expand Down
6 changes: 4 additions & 2 deletions zulipterminal/ui_tools/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ def __init__(self, message: Message, model: "Model", last_message: Any) -> None:
self.stream_id = self.message["stream_id"]
self.topic_name = self.message["subject"]
else:
self.email = self.message["sender_email"]
self.user_id = self.message["sender_id"]
# Get sender details only for non-empty narrows
if self.message.get("id") is not None:
self.email = self.message["sender_email"]
self.user_id = self.message["sender_id"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do use this message data later, but apparently not these attributes - even though we test for them! In any case, based on usage, it seems like they should be set independently of the message type if we are to use them? So that may be a useful part of the earlier refactor.


if self._is_private_message_to_self():
recipient = self.message["display_recipient"][0]
Expand Down
40 changes: 40 additions & 0 deletions zulipterminal/ui_tools/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,46 @@ def create_msg_box_list(
focus_msg = None
last_msg = last_message
muted_msgs = 0 # No of messages that are muted.

# 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 = {
Comment on lines +33 to +36
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'?

"id": None,
"content": (
"No search hits" if model.is_search_narrow() else "No messages here"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
"is_me_message": False,
"flags": ["read"],
"reactions": [],
"type": message_type,
"stream_id": model.stream_id if message_type == "stream" else None,
"stream_name": model.stream_dict[model.stream_id]["name"]
if message_type == "stream"
else None,
"subject": next(
(subnarrow[1] for subnarrow in model.narrow if subnarrow[0] == "topic"),
"No topics in channel",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

)
if message_type == "stream"
else None,
"display_recipient": (
model.stream_dict[model.stream_id]["name"]
if message_type == "stream"
else [
{
"email": model.user_id_email_dict[recipient_id],
"full_name": model.user_dict[
model.user_id_email_dict[recipient_id]
]["full_name"],
"id": recipient_id,
}
for recipient_id in model.recipients
]
),
}
message_list.append(dummy_message)

for msg in message_list:
if is_unsubscribed_message(msg, model):
continue
Expand Down