Group message data #1537

Niloth-p · 2024-07-19T07:35:42Z

What does this PR do, and why?

Old version

Refactoring: Group message data in a TypedDict / dataclass.

Group the following into a single piece of data, to simplify passing them as arguments across functions.

message
topic_links
message_links
time_mentions

Motivation: Was reviewing #1529, the redundancy felt awkward, addressed it with this PR.
Was previously discussed in #1455 comments.

Objective: Avoid passing Message Info data to all of its inner popups.

Solution Steps:

Introducing a Stacking of Popups
Extending PopupView to support a second command to allow both types of exiting.
Add a TypedDict to group the content of the message info popup

Why stack popups?
Currently, we create a new popup even when closing the uppermost popups.
With stacking, we can re-use the popups.
This allows us to not pass the entire data for creating the inner popups
to the outer popups.

Support 2 functions.

Exiting the current popup - goes back to the previous popup.
Exiting all popups - clears all popups.

Currently, the only popup that opens other popups is the MsgInfoView.
Affected widgets: EditHistoryView, FullRenderedMsgView, FullRawMsgView.

Hotkey behavior (from the widget):

Press Esc or respective command -> go to MsgInfoView popup
Press i from the widget -> close all popups (EXIT_POPUP command)

By adding new keypress handling to their parent PopupView, we can
pass their respective commands without having to implement custom
keypress handlers in each of the inner popup classes.

External discussion & connections

Discussed in #zulip-terminal in Stacking of Popups (refactoring Message Info popup) #T1537
Fully fixes #
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 Add support for copying code snippets #1455 Render Spoilers in ZT #1529

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

zulipbot · 2024-07-19T07:36:16Z

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

neiljp

@Niloth-p Thanks for looking into this 👍

This is a quick glance over the code to see if this was the direction I expected; I've not looked at the tests as yet.

It certainly does tidy things up, and makes the code easier to read overall, so appears to be heading in a good direction.

However, one aspect this highlights is how this enables dropping of all but the self.message in popups called from the message information popup (eg. topic links). The new self.msg_data appears to be used only to get the self.message and then bring back the 'parent' message information popup when required.
=> If we didn't have to call show_msg_info(self.msg_data), we could avoid passing all this data around 💡

We could discuss how to solve the latter issue, since it would be an even cleaner solution, but as above, this tidying would be a good first step in any case :)

@rsashank I'd be interested to hear your feedback on this as it stands, since you have the related PRs ongoing.

Would this be good merged before or after those?
Do those use only the self.message, or also other data from the message-box in the extra popups?

neiljp · 2024-07-22T05:30:01Z

zulipterminal/ui_tools/messages.py

-                self.message, self.topic_links, self.message_links, self.time_mentions
+                MessageData(
+                    self.message,
+                    self.topic_links,
+                    self.message_links,
+                    self.time_mentions,
+                )


This would be the only point where I'd expect we'd initialize a MessageData, but that doesn't seem to be the case. Can the code be simplified?

neiljp · 2024-07-22T05:32:43Z

zulipterminal/helper.py

+class MessageData:
+    message: Message
+    topic_links: Dict[str, Tuple[str, int, bool]]
+    message_links: Dict[str, Tuple[str, int, bool]]
+    time_mentions: List[Tuple[str, str]]


I was surprised to see the message itself in here, though it is passed around in each location right now, so it seems useful as it stands.

neiljp · 2024-07-22T05:37:05Z

zulipterminal/ui_tools/views.py

-        topic_links: Dict[str, Tuple[str, int, bool]],
-        message_links: Dict[str, Tuple[str, int, bool]],
-        time_mentions: List[Tuple[str, str]],
+        msg_data: MessageData,


I'd prefer a more focused name than MessageData, since it's not entirely clear what else is included; after all, a message (Message) is also 'message data' :)

Also while I know we have it in the code right now, generally it's good to use full versions for variables, eg. favoring message over msg.

Would MessageInfoPopupContent and message_info_content work better?

And the TestMsgInfoView class. Tests updated.

Support 2 functions. - Exiting the current popup - goes back to the previous popup. - Exiting all popups - clears all popups. Why stack popups? Currently, we create a new popup even when closing the uppermost popups. With stacking, we can re-use the popups. This allows us to not pass the entire data for creating the inner popups to the outer popups. Currently, the only popup that opens other popups is the MsgInfoView. Affected widgets: EditHistoryView, FullRenderedMsgView, FullRawMsgView. Updated them to use the newly added functions to exit popups. Hotkey behavior (from the widget): - Press Esc or respective command -> go to MsgInfoView popup - Press i from the widget -> close all popups (EXIT_POPUP command) By adding new keypress handling to their parent PopupView, we can pass their respective commands without having to implement custom keypress handlers. Tests updated. Renamed tests: - test_keypress_show_msg_info -> test_keypress_exit_to_msg_info. - test_keypress_exit_popup -> test_keypress_exit_all_popups.

Now that the inner popups can exit to the outer popup without having to initialize it, we can remove the parameters of MsgInfoView class from its inner popup classes and their respective show_* functions. Tests updated.

Tests updated.

Group the following fields that are shown in a message info popup, into a single piece of data. - topic_links - message_links - time_mentions Tests updated. Fixture added.

Niloth-p · 2024-07-22T19:04:45Z

Great suggestion, @neiljp! I didn't look into that deeply at the time, thank you for giving me hints to the right direction.
Your awesome explanation helped me come up with this update: Stacking of popups. Hope it's a good solution 🤞

rsashank · 2024-07-24T17:34:17Z

@rsashank I'd be interested to hear your feedback on this as it stands, since you have the related PRs ongoing.

Would this be good merged before or after those?

Do those use only the self.message, or also other data from the message-box in the extra popups?

I can rebase that; it shouldn't be a problem if this is merged first. It'll be a bit of work though. :)

Spoilers use other data as well because we're attempting to return to the message information popup when exiting the SpoilerView. However, the stacking of popups should solve that. I'll look into it.

neiljp

@Niloth-p This is now quite a different PR, with a different tilt towards the refactoring!

I'm sure that (layered) popups can be modeled in many different ways, this was a useful observation that turned out nicely - thanks for changing course and following through :)

The changes look broadly good, and generally are grouped well.

Most of my comments are small points, or thoughts on how it could be improved here and there. We could even hold off on the last commit initially, since I think the earlier ones are a good refactoring all on their own 👍
(it might also go well when combined with looking at the group of data that comes out of the soup2markup end of the processing, which may also benefit from refactoring/simplifying?)

neiljp · 2024-08-14T01:03:17Z

tests/ui_tools/test_popups.py

@@ -520,14 +520,14 @@ def test_init(self, msg_box: MessageBox) -> None:
        assert self.full_rendered_message.footer.widget_list == msg_box.footer



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.

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.

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

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

neiljp · 2024-08-14T03:10:22Z

zulipterminal/core.py

    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


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)

neiljp · 2024-08-14T03:13:24Z

tests/ui_tools/test_popups.py

-            topic_links=OrderedDict(),
-            message_links=OrderedDict(),
-            time_mentions=list(),


Nit: Removing dead/unnecessary code is a refactor - it shouldn't change the behavior :)

Sorry, thank you!

neiljp · 2024-08-14T03:15:58Z

zulipterminal/ui_tools/views.py

-                message_links=self.message_links,
-                time_mentions=self.time_mentions,
-            )
+            self.controller.show_edit_history(message=self.msg)


Nit: While self.msg isn't a great name - it'd be fine to refactor with a rename! - note that when there's only one parameter then it's normally fine to not need to specify it by keyword.

neiljp · 2024-08-14T03:21:11Z

zulipterminal/ui_tools/views.py

-            self.controller.show_edit_history(
-                message=self.msg,
-                topic_links=self.topic_links,
-                message_links=self.message_links,
-                time_mentions=self.time_mentions,
-            )


Nit: We removed some of these parameters in a previous commit. I'm aware that extra params can be passed (like we did after that commit), though slightly concerned we didn't get any warning - though we don't have explicit tests for them I guess? In any case it may be clearer commit-wise to pair the calling/definition changes into the same commits.

Nit: Again, a refactor commit :)

neiljp · 2024-08-14T03:34:23Z

zulipterminal/helper.py

+class MessageInfoPopupContent(TypedDict):
+    topic_links: Dict[str, Tuple[str, int, bool]]
+    message_links: Dict[str, Tuple[str, int, bool]]
+    time_mentions: List[Tuple[str, str]]


This commit is now a much smaller part of this PR - but still helpful, if not perhaps as urgent a commit to merge, tidying-wise 👍

This particular change may be better-placed in messages.py, ie. where all the messages-UI code is located. The connected aspect is that this is mostly data from the result of soup2markup or similar, I believe? This name is an improvement - but I'd likely (also) consider the source of the data, ie. this is indexed/extracted data. That can be focused on before merging though.

neiljp · 2024-08-14T03:39:20Z

zulipterminal/ui_tools/views.py

        self.msg = msg
-        self.topic_links = topic_links
-        self.message_links = message_links
-        self.time_mentions = time_mentions
+        self.topic_links = message_info_content["topic_links"]
+        self.message_links = message_info_content["message_links"]
+        self.time_mentions = message_info_content["time_mentions"]


All the unpacking into separate variables - and more to come! - suggests to me that it may be simpler to unpack 'on demand', and/or use a dataclass to make the syntax much easier to read :)

zulipterminal/ui_tools/views.py

neiljp · 2024-09-25T12:52:46Z

I'm working on refining these points to help with other PRs.

zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 19, 2024

Niloth-p added area: refactoring PR needs buddy review labels Jul 19, 2024

Niloth-p requested a review from rsashank July 19, 2024 07:36

Niloth-p mentioned this pull request Jul 19, 2024

Render Spoilers in ZT #1529

Open

18 tasks

neiljp reviewed Jul 22, 2024

View reviewed changes

Niloth-p force-pushed the group-markup branch 2 times, most recently from 224ee15 to 808e665 Compare July 22, 2024 09:11

Niloth-p added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs buddy review labels Jul 22, 2024

Niloth-p added 5 commits July 23, 2024 00:28

views: Use self to re-use instance variables in MsgInfoView class.

15a7c86

And the TestMsgInfoView class. Tests updated.

views: Delete the calling arguments from MsgInfoView's show_* calls.

9e5036e

Tests updated.

refactor: views/helper/core/messages: Group message info in a TypedDict.

9051d4f

Group the following fields that are shown in a message info popup, into a single piece of data. - topic_links - message_links - time_mentions Tests updated. Fixture added.

Niloth-p force-pushed the group-markup branch from 808e665 to 9051d4f Compare July 22, 2024 19:00

Niloth-p added PR needs buddy review and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 22, 2024

neiljp reviewed Aug 14, 2024

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Aug 14, 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

Group message data #1537

Group message data #1537

Niloth-p commented Jul 19, 2024 •

edited

Loading

zulipbot commented Jul 19, 2024

neiljp left a comment

neiljp Jul 22, 2024

neiljp Jul 22, 2024

neiljp Jul 22, 2024

Niloth-p Jul 22, 2024

Niloth-p commented Jul 22, 2024

rsashank commented Jul 24, 2024

neiljp left a comment

neiljp Aug 14, 2024

Niloth-p Sep 8, 2024

neiljp Sep 25, 2024

Niloth-p Sep 25, 2024

neiljp Aug 14, 2024

neiljp Aug 14, 2024

Niloth-p Sep 8, 2024

neiljp Aug 14, 2024

neiljp Aug 14, 2024

neiljp Aug 14, 2024

neiljp Aug 14, 2024

neiljp commented Sep 25, 2024

		@@ -520,14 +520,14 @@ def test_init(self, msg_box: MessageBox) -> None:
		assert self.full_rendered_message.footer.widget_list == msg_box.footer

Group message data #1537

Are you sure you want to change the base?

Group message data #1537

Conversation

Niloth-p commented Jul 19, 2024 • edited Loading

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

zulipbot commented Jul 19, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p commented Jul 22, 2024

rsashank commented Jul 24, 2024

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

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

neiljp commented Sep 25, 2024

Niloth-p commented Jul 19, 2024 •

edited

Loading