Scrollable large single msg view #874

Ezio-Sarthak · 2021-01-15T12:37:54Z

What does the PR do?! :

Allows scrolling for large single messages
Note: currently true for every message
Provides raw and rendered message view to users.

Commit flow:

refactor: views: Use urwid.Frame for PopUpView.
boxes: Add members to MessageBox to fetch header, content, footer.
hotkeys/keys: Add f hotkey for full rendered message popup.
views: Structure FullRenderedMsgView class.
core: Add controller helper to show full rendered message on toggle.
views: Add keypress events for showing/hiding full rendered message.
model: Add support for fetching a raw message.
refactor: boxes: Use new model method to fetch a raw message.
hotkeys/keys: Add r hotkey for full raw message popup view.
views: Structure FullRawMsgView class.
core: Add controller helper to show full raw message on toggle.
views: Add keypress events for showing/hiding full raw message.

Further discussions here on czo :)

Fixes #543.

Screenshots/GIFs:
Full Rendered Message View

Full Raw Message View

neiljp · 2021-02-17T07:29:06Z

@Ezio-Sarthak This has been discussed on the stream? To clarify, I think it would be fine to include the "full view" and "raw view" options in the same popup for now, if we could make it clear how to toggle between them.

Ezio-Sarthak · 2021-03-10T16:30:16Z

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"

zee-bit

Thanks for working on this @Ezio-Sarthak. 👍

It works pretty well functionally. I am sure after some improvements the Tab to switch context in popups would be extended to other popups as well in the future. The title is pretty huge currently and offsets the 'Full message view' text to one side of the header, is there any way to fix this?
Also, the message header inside the popup breaks on to another line, if the topic name is large, it's better to take care of this case as well(maybe use ellipsis?). for e.g, see my message here: #test here

I really like the style of your PR message above. 👍

I've dropped in a few comments in your PR, feel free to discuss further. :)

zee-bit · 2021-03-10T19:21:33Z

docs/hotkeys.md

@@ -55,6 +55,7 @@
 |Add/remove star status of the current message|<kbd>ctrl</kbd> + <kbd>s</kbd> / <kbd>*</kbd>|
 |Show/hide message information|<kbd>i</kbd>|
 |Show/hide edit history (from message information)|<kbd>e</kbd>|
+|View full message from message information box|<kbd>f</kbd>|


Since it's a toggle key, Show/hide would be more consistent here?

zee-bit · 2021-03-10T19:36:21Z

zulipterminal/ui_tools/views.py

+        elif is_command_key('GO_UP', key):
+            key = 'up'
+        elif is_command_key('GO_DOWN', key):
+            key = 'down'
+        elif is_command_key('SCROLL_UP', key):
+            key = 'page up'
+        elif is_command_key('SCROLL_DOWN', key):
+            key = 'page down'
+        elif is_command_key('GO_TO_BOTTOM', key):
+            key = 'end'


Are these lines necessary?

Nope. I realized I need simply PopUpView as base class for content. Thanks :)

zee-bit · 2021-03-10T20:02:05Z

zulipterminal/core.py

+                self, message, message_links, time_mentions,
+                'Full message view (tab: cycle raw/formatted content)'
+            ),


Are we using "view" in any other popup's header name?
'view' sounds good, but seems inconsistent with our other popups, and unnecessarily increases header length.

zee-bit · 2021-03-10T20:09:29Z

zulipterminal/ui_tools/views.py

+                  ('Full message view', 'Press {} to view'
+                   .format(full_msg_keys))]),


I guess we already transitioned to f-strings. It would be better to use that instead of .format() wherever possible. :)

zee-bit · 2021-03-10T20:26:11Z

zulipterminal/ui_tools/views.py

+        response = controller.model.client.get_raw_message(message['id'])
+        _content_raw = urwid.Text('N/A')
+        if response['result'] == 'success':


It's good that you are taking care of the 'N/A' case. 👍
But, it's probably better to expand N/A to something clearer in any case. :)

zulipbot · 2021-03-11T07:33:38Z

Hello @Ezio-Sarthak, it seems like you have referenced #543 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #543..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

neiljp

@Ezio-Sarthak This works well but I'm a little wary of the hooks into the "tabs" which aren't rendered and use of CYCLE_COMPOSE_FOCUS. I'm open to leaving that code in there since it does the switching, but it feels a bit strange, as if you've ripped out some code, which you sort of have?

Perhaps minor, but we might add some kind of check to ensure that we're not fetching the raw message multiple times - which I know the code currently satisfies.

It seems that footlinks don't show in the rendered version?

The rendered text is also indented to the left and right, which looks a little strange perhaps.

It would be good to have i exit the view completely, like we do for edit history, if we're to be consistent.

neiljp · 2021-03-10T17:13:12Z

zulipterminal/ui_tools/views.py

+    @staticmethod
+    def calculate_popup_height(widgets: List[Any],
+                               popup_width: int) -> int:
+        """
+        Returns popup height. The popup height is calculated using urwid's
+        .rows method on every widget.
+        """
+        return sum(widget.rows((popup_width, )) for widget in widgets)


I was confused about this (and possibly other code) being identical to the PopUpView code, but you're re-implementing a different popup type here?

Right. I forgot we could utilize the static method from PopUpView in this. Thanks for pointing out :)

tests/ui_tools/test_popups.py

neiljp · 2021-03-10T17:21:27Z

zulipterminal/core.py

@@ -236,6 +237,19 @@ def show_about(self) -> None:
            'area:help'
        )

+    def show_full_msg(


I don't see a reason to shorten this?

Thanks for pointing that out. Initially, it was show_full_msg_view, but view seemed obvious, so I skipped it. I'll change it to show_full_message 👍

neiljp · 2021-03-10T17:26:45Z

zulipterminal/ui_tools/views.py

+             + self.contents['header'][0].widget_list),
+            self.width
+        )
+        self.height = min(max_rows, height)


The height doesn't try and maximize to the available based on rendered and raw content, eg. if the raw is larger.

True. It would be then good to first show the raw content then? (Since most likely the length of raw content is more than the formatted one).

On a side note, I'm a bit skeptical on whether to change the width/height of the popup upon switching. Is this something we want to achieve?

neiljp · 2021-03-19T21:51:14Z

zulipterminal/ui_tools/views.py

+        response = controller.model.client.get_raw_message(message['id'])
+        _content_raw = 'Raw content unavailable :('
+        if response['result'] == 'success':
+            _content_raw = response['raw_content']


I know we have a pre-existing example of this, but network/zulip error handling shouldn't really be in the UI. We generally handle the rest in wrapped methods in the model. You could add a refactor to wrap the raw call like with other calls to client.

neiljp · 2021-03-19T22:02:07Z

zulipterminal/ui_tools/views.py

+
+        max_cols, max_rows = controller.maximum_popup_dimensions()
+
+        self.width = min(max_cols, self.contents['body'][0].width)


This seems to generate narrow popups, even when the base window is quite wide?
For large messages we want to display as much as we can :)

neiljp · 2021-03-19T22:04:04Z

zulipterminal/ui_tools/views.py

+    def _render_body(self) -> None:
+        content = PopUpView(self.controller,
+                            [self.tab_contents[self.tab_focus]],
+                            'FULL_MSG_VIEW', 64, self.title)


Could you explain the logic here? What makes 64 special? This explains the narrow popup.

I initially thought that 64 is the maximum width 😅. I'll refactor that using max_cols from controller 👍

PopUpView already has both max_rows and max_cols present in its constructor, we don't need to pass those again, right?

What I'm passing is the requested_width (required parameter), as we want to display as much info on that view as we can. In my coming iteration, that 64 would be replaced by max_cols. Hope I'm able to clarify 😅

neiljp · 2021-03-19T22:16:29Z

tests/ui_tools/test_popups.py

+    def mock_external_classes(self, mocker, msg_box, initial_index):
+        self.controller = mocker.Mock()
+        mocker.patch.object(self.controller, 'maximum_popup_dimensions',
+                            return_value=(64, 64))


This seems conveniently similar to the hardcoded "magic" value in the actual code; maybe we should parametrize over things somehow so a test notices, or at least fix that width issue!

(See my other comment)

neiljp · 2021-03-19T22:18:23Z

tests/ui_tools/test_popups.py

+        ]
+    )
+    @pytest.mark.parametrize("tab_key",
+                             keys_for_command("CYCLE_COMPOSE_FOCUS"))


This is confusing to read, and in the code - it's not compose related.

neiljp

@Ezio-Sarthak This is a big PR that's gone through various iterations now - phew!

zulipterminal/model.py

zulipterminal/config/keys.py

zulipterminal/ui_tools/boxes.py

neiljp · 2021-07-22T06:26:33Z

zulipterminal/ui_tools/views.py

+        elif is_command_key("MSG_INFO", key):
+            self.controller.exit_popup()


I can see why this is necessary, but I'm unclear why we don't use it for the edit history popup, since that seems a close equivalent of this one.

There's no test for this?

Ah. This didn't grab my attention before! I didn't expect this to be the case 😅. Thanks for pointing this out! I am actually expecting the opposite. What I expect is exiting of popup using either FULL_RENDERED_MESSAGE (say) or GO_BACK only

EDIT: Doubt cleared :)

Good discussion! Thanks @preetmishra :)

neiljp · 2021-07-22T22:47:11Z

zulipterminal/ui_tools/views.py

+        if response is None:
+            controller.report_error("Error: Couldn't fetch raw message content")


This is going to be a duplicate of the error handling in the model method?

Ah yes. I'll simply return in that case 👍. Thanks for pointing out!

neiljp · 2021-07-22T22:55:14Z

@Ezio-Sarthak I've merged the first feature, but given the report_error issue wanted to briefly pause here. The secondary reason is that the raw content commit is ripe for refactoring the existing code, though we can do that after this PR of course (but shouldn't forget!)

This commit adds a model method that fetches raw message content using its ID. Errors and exceptions are handled accordingly. Tests added.

Ezio-Sarthak · 2021-07-23T06:05:04Z

@neiljp Thanks for your patience on this! I just pushed an updated version. 👍

This commit replaces instances of `get_raw_message` client call with newly added `fetch_raw_message_content` method wherever present. Tests amended.

docs/hotkeys.md regenerated.

This commit adds a popup class with header as message header, body as message (raw) content, and footer as potential footlinks and reactions. This would allow users to see full scrollable (raw) message content. In the rare case of fetch failure, a footer notification is shown with the error and the popup doesn't open in this case. Tests added (FullRawMsgView).

This commit adds keypress events for showing/hiding full raw message view in message information popup. Tests amended.

neiljp · 2021-07-23T06:38:12Z

@Ezio-Sarthak Thanks for the last update with the refactor to use the new model wrapper for the other cases 👍 I'm merging after fixing one small typo in that refactor 🎉
(However, we should add tests to fix that!)

zulipbot added the size: XL [Automatic label added by zulipbot] label Jan 15, 2021

Ezio-Sarthak force-pushed the scroll-large-msg-view branch 5 times, most recently from 05a030c to 7d097eb Compare January 21, 2021 05:20

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 7d097eb to 4553b4e Compare January 23, 2021 06:38

Base automatically changed from master to main January 30, 2021 20:31

zulipbot added the has conflicts label Jan 30, 2021

Ezio-Sarthak mentioned this pull request Feb 15, 2021

Message content copy[/paste] #546

Open

Ezio-Sarthak force-pushed the scroll-large-msg-view branch 2 times, most recently from 867849d to 783b392 Compare February 16, 2021 16:52

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 17, 2021

neiljp removed the has conflicts label Feb 25, 2021

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 783b392 to 46dd6af Compare March 10, 2021 16:15

Ezio-Sarthak changed the title ~~[WIP] Scrollable large single msg view~~ Scrollable large single msg view Mar 10, 2021

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Mar 10, 2021

zee-bit suggested changes Mar 10, 2021

View reviewed changes

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 46dd6af to 68541d9 Compare March 11, 2021 07:33

Ezio-Sarthak force-pushed the scroll-large-msg-view branch 2 times, most recently from b1a437a to 32d2c48 Compare March 18, 2021 14:23

neiljp reviewed Mar 19, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Mar 19, 2021

neiljp added this to the Next Release milestone Mar 19, 2021

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 32d2c48 to b710767 Compare March 20, 2021 11:13

Ezio-Sarthak force-pushed the scroll-large-msg-view branch 2 times, most recently from 41a9ff0 to 8d7fe5c Compare July 19, 2021 03:10

neiljp mentioned this pull request Jul 19, 2021

bugfix: views: Ensure EditModeView works with new frame-based popup. #1090

Merged

4 tasks

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 8d7fe5c to 0a642bd Compare July 19, 2021 07:54

neiljp reviewed Jul 22, 2021

View reviewed changes

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 0a642bd to da9ea59 Compare July 22, 2021 19:05

Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 22, 2021

neiljp reviewed Jul 22, 2021

View reviewed changes

zulipbot added the has conflicts label Jul 22, 2021

model: Add support for fetching a raw message content.

9fc5682

This commit adds a model method that fetches raw message content using its ID. Errors and exceptions are handled accordingly. Tests added.

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from da9ea59 to 9acca87 Compare July 23, 2021 06:00

zulipbot removed the has conflicts label Jul 23, 2021

Ezio-Sarthak force-pushed the scroll-large-msg-view branch from 9acca87 to 73b13af Compare July 23, 2021 06:20

Ezio-Sarthak added 5 commits July 22, 2021 23:23

refactor: boxes: Use new model method to fetch a raw message.

1c6f7e5

This commit replaces instances of `get_raw_message` client call with newly added `fetch_raw_message_content` method wherever present. Tests amended.

hotkeys/keys: Add r hotkey for full raw message popup view.

571d17f

docs/hotkeys.md regenerated.

core: Add controller helper to show full raw message on toggle.

3cd66ac

views: Add keypress events for showing/hiding full raw message.

5c835ca

This commit adds keypress events for showing/hiding full raw message view in message information popup. Tests amended.

neiljp force-pushed the scroll-large-msg-view branch from 73b13af to 5c835ca Compare July 23, 2021 06:28

neiljp merged commit cdd11b0 into zulip:main Jul 23, 2021

neiljp removed the PR needs review PR requires feedback to proceed label Jul 23, 2021

Ezio-Sarthak mentioned this pull request Jul 23, 2021

Condense/collapse large messages (messages crossing screen height) #1094

Open

		('Full message view', 'Press {} to view'
		.format(full_msg_keys))]),


		max_cols, max_rows = controller.maximum_popup_dimensions()

		self.width = min(max_cols, self.contents['body'][0].width)

		elif is_command_key("MSG_INFO", key):
		self.controller.exit_popup()

		if response is None:
		controller.report_error("Error: Couldn't fetch raw message content")

Scrollable large single msg view #874

Scrollable large single msg view #874

Conversation

Ezio-Sarthak commented Jan 15, 2021 • edited Loading

neiljp commented Feb 17, 2021

Ezio-Sarthak commented Mar 10, 2021

zee-bit 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

zulipbot commented Mar 11, 2021

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

Ezio-Sarthak Mar 20, 2021 • 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

Ezio-Sarthak Mar 20, 2021 • 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

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Jul 22, 2021 • 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

neiljp commented Jul 22, 2021

Ezio-Sarthak commented Jul 23, 2021

neiljp commented Jul 23, 2021

Ezio-Sarthak commented Jan 15, 2021 •

edited

Loading

zee-bit left a comment •

edited

Loading

Ezio-Sarthak Mar 20, 2021 •

edited

Loading

Ezio-Sarthak Mar 20, 2021 •

edited

Loading

Ezio-Sarthak Jul 22, 2021 •

edited

Loading