Group message data
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 #
- [x] Merge will enable work on #1455 #1529
How did you test this?
- [x] Manually - Behavioral changes
- [ ] Manually - Visual changes
- [x] Adapting existing automated tests
- [ ] Adding automated tests for new behavior (or missing tests)
- [x] Existing automated tests should already cover this (only a refactor of tested code)
Self-review checklist for each commit
- [x] It is a minimal coherent idea
- [x] It has a commit summary following the documented style (title & body)
- [x] It has a commit summary describing the motivation and reasoning for the change
- [x] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit
Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!
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 :crossed_fingers:
@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.
I'm working on refining these points to help with other PRs.