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.

Sign up for GitHub

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

Group message data #1537

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+127 −266
Open

Group message data #1537

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15a7c86
views: Use `self` to re-use instance variables in MsgInfoView class.
Niloth-p Jul 22, 2024
c734d2a
core/views: Stack inner popups, add support for exiting all popups.
Niloth-p Jul 22, 2024
e316327
views/core: Delete all message info popup parameters from inner popups.
Niloth-p Jul 22, 2024
9e5036e
views: Delete the calling arguments from MsgInfoView's show_* calls.
Niloth-p Jul 22, 2024
9051d4f
refactor: views/helper/core/messages: Group message info in a TypedDict.
Niloth-p Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 29 additions & 74 deletions tests/ui_tools/test_popups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,14 +520,14 @@ def test_init(self, msg_box: MessageBox) -> None:
assert self.full_rendered_message.footer.widget_list == msg_box.footer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re the commit text for the 'stack' change, my only nit would be re the end of the 'why stack popups' that, we don't need to pass the entire data around... unless we need it :) None of these now do, but as per discussion there are upcoming PRs that will do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, is that so? I can't seem to recall that discussion.
The change to the commit text sounds better anyways.
Glad to know that the rest is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've not rechecked, but was likely referring to Sashank's comment here, where he is expecting this will be necessary for spoilers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that's covered by this PR. Won't need to pass around any data for that.

@pytest.mark.parametrize("key", keys_for_command("MSG_INFO"))
def test_keypress_exit_popup(
def test_keypress_exit_all_popups(
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
size = widget_size(self.full_rendered_message)

self.full_rendered_message.keypress(size, key)

assert self.controller.exit_popup.called
assert self.controller.exit_all_popups.called

def test_keypress_exit_popup_invalid_key(
self, widget_size: Callable[[Widget], urwid_Size]
Expand All @@ -546,19 +546,14 @@ def test_keypress_exit_popup_invalid_key(
*keys_for_command("EXIT_POPUP"),
},
)
def test_keypress_show_msg_info(
def test_keypress_exit_to_msg_info(
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
size = widget_size(self.full_rendered_message)

self.full_rendered_message.keypress(size, key)

self.controller.show_msg_info.assert_called_once_with(
msg=self.message,
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)
assert self.controller.exit_popup.called


class TestFullRawMsgView:
Expand Down Expand Up @@ -596,14 +591,14 @@ def test_init(self, msg_box: MessageBox) -> None:
assert self.full_raw_message.footer.widget_list == msg_box.footer

@pytest.mark.parametrize("key", keys_for_command("MSG_INFO"))
def test_keypress_exit_popup(
def test_keypress_exit_all_popups(
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
size = widget_size(self.full_raw_message)

self.full_raw_message.keypress(size, key)

assert self.controller.exit_popup.called
assert self.controller.exit_all_popups.called

def test_keypress_exit_popup_invalid_key(
self, widget_size: Callable[[Widget], urwid_Size]
Expand All @@ -622,19 +617,14 @@ def test_keypress_exit_popup_invalid_key(
*keys_for_command("EXIT_POPUP"),
},
)
def test_keypress_show_msg_info(
def test_keypress_exit_to_msg_info(
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
size = widget_size(self.full_raw_message)

self.full_raw_message.keypress(size, key)

self.controller.show_msg_info.assert_called_once_with(
msg=self.message,
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)
assert self.controller.exit_popup.called


class TestEditHistoryView:
Expand Down Expand Up @@ -671,14 +661,14 @@ def test_init(self) -> None:
)

@pytest.mark.parametrize("key", keys_for_command("MSG_INFO"))
def test_keypress_exit_popup(
def test_keypress_exit_all_popups(
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
size = widget_size(self.edit_history_view)

self.edit_history_view.keypress(size, key)

assert self.controller.exit_popup.called
assert self.controller.exit_all_popups.called

def test_keypress_exit_popup_invalid_key(
self, widget_size: Callable[[Widget], urwid_Size]
Expand All @@ -700,12 +690,7 @@ def test_keypress_show_msg_info(

self.edit_history_view.keypress(size, key)

self.controller.show_msg_info.assert_called_once_with(
msg=self.message,
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)
assert self.controller.exit_popup.called

@pytest.mark.parametrize(
"snapshot",
Expand Down Expand Up @@ -975,27 +960,28 @@ def mock_external_classes(
"Tue Mar 13 10:55:22",
"Tue Mar 13 10:55:37",
]
self.message = message_fixture
self.msg_info_view = MsgInfoView(
self.controller,
message_fixture,
self.message,
"Message Information",
OrderedDict(),
OrderedDict(),
list(),
)

def test_init(self, message_fixture: Message) -> None:
assert self.msg_info_view.msg == message_fixture
def test_init(self) -> None:
assert self.msg_info_view.msg == self.message
assert self.msg_info_view.topic_links == OrderedDict()
assert self.msg_info_view.message_links == OrderedDict()
assert self.msg_info_view.time_mentions == list()

def test_pop_up_info_order(self, message_fixture: Message) -> None:
def test_pop_up_info_order(self) -> None:
topic_links = OrderedDict([("https://bar.com", ("topic", 1, True))])
message_links = OrderedDict([("image.jpg", ("image", 1, True))])
msg_info_view = MsgInfoView(
self.controller,
message_fixture,
self.message,
title="Message Information",
topic_links=topic_links,
message_links=message_links,
Expand Down Expand Up @@ -1029,7 +1015,6 @@ def test_keypress_any_key(
)
def test_keypress_edit_history(
self,
message_fixture: Message,
key: str,
widget_size: Callable[[Widget], urwid_Size],
realm_allow_edit_history: bool,
Expand All @@ -1041,21 +1026,13 @@ def test_keypress_edit_history(
self.controller.model.initial_data = {
"realm_allow_edit_history": realm_allow_edit_history,
}
msg_info_view = MsgInfoView(
self.controller,
message_fixture,
title="Message Information",
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)
size = widget_size(msg_info_view)
size = widget_size(self.msg_info_view)

msg_info_view.keypress(size, key)
self.msg_info_view.keypress(size, key)

if msg_info_view.show_edit_history_label:
if self.msg_info_view.show_edit_history_label:
self.controller.show_edit_history.assert_called_once_with(
message=message_fixture,
message=self.message,
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
Expand All @@ -1065,51 +1042,29 @@ def test_keypress_edit_history(

@pytest.mark.parametrize("key", keys_for_command("FULL_RENDERED_MESSAGE"))
def test_keypress_full_rendered_message(
self,
message_fixture: Message,
key: str,
widget_size: Callable[[Widget], urwid_Size],
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
msg_info_view = MsgInfoView(
self.controller,
message_fixture,
title="Message Information",
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)
size = widget_size(msg_info_view)
size = widget_size(self.msg_info_view)

msg_info_view.keypress(size, key)
self.msg_info_view.keypress(size, key)

self.controller.show_full_rendered_message.assert_called_once_with(
message=message_fixture,
message=self.message,
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)

@pytest.mark.parametrize("key", keys_for_command("FULL_RAW_MESSAGE"))
def test_keypress_full_raw_message(
self,
message_fixture: Message,
key: str,
widget_size: Callable[[Widget], urwid_Size],
self, key: str, widget_size: Callable[[Widget], urwid_Size]
) -> None:
msg_info_view = MsgInfoView(
self.controller,
message_fixture,
title="Message Information",
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
)
size = widget_size(msg_info_view)
size = widget_size(self.msg_info_view)

msg_info_view.keypress(size, key)
self.msg_info_view.keypress(size, key)

self.controller.show_full_raw_message.assert_called_once_with(
message=message_fixture,
message=self.message,
topic_links=OrderedDict(),
message_links=OrderedDict(),
time_mentions=list(),
Expand Down
7 changes: 7 additions & 0 deletions zulipterminal/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def __init__(
self.notify_enabled = notify
self.maximum_footlinks = maximum_footlinks
self.editor_command = editor_command
self.popup_stack: List[urwid.Widget] = []

self.debug_path = debug_path

Expand Down Expand Up @@ -226,6 +227,8 @@ def clamp(n: int, minn: int, maxn: int) -> int:
return max_popup_cols, max_popup_rows

def show_pop_up(self, to_show: Any, style: str) -> None:
self.popup_stack.append(self.loop.widget)

text = urwid.Text(to_show.title, align="center")
title_map = urwid.AttrMap(urwid.Filler(text), style)
title_box_adapter = urwid.BoxAdapter(title_map, height=1)
Expand All @@ -249,6 +252,10 @@ def is_any_popup_open(self) -> bool:
return isinstance(self.loop.widget, urwid.Overlay)

def exit_popup(self) -> None:
self.loop.widget = self.popup_stack.pop()

def exit_all_popups(self) -> None:
self.popup_stack.clear()
self.loop.widget = self.view
Comment on lines 254 to 259
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good, though slightly differently it might be more resilient?

  • we could be more defensive with the current design (error-resilient); if there's a show/exit mismatch coding error elsewhere, the stack could be empty and tries to pop/clear
  • we'll always have self.view accessible as the base self.loop.widget, so we could store only popups in the stack?

I'm not sure how feasible it is with all the extra code involved in popups, but it would be useful to add some tests involving show/exit/exit_all, and maybe also is_any_popup_open?
(we could add some of these first, and then this subsequent partially-a-refactor commit would indicate some things hadn't changed, by some tests not changing, but still passing)


def show_help(self) -> None:
Expand Down
Loading