Add support for copying raw message. #1470

Sushmey · 2024-02-14T14:17:24Z

What does this PR do, and why?

Adding a key to copy raw message content since manually copying text in a terminal is pretty difficult due to the bars on the sides. Implemented using preexisting copy_to_clipboard function.

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes #
Partially fixes issue Message content copy[/paste] #546
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR views/keys/hotkeys: Add key to copy message content #1365
Requires merge of PR #
Merge will enable work on #

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

Visual changes

neiljp

@Sushmey This looks to work well for the most part 👍

I've left some mostly-minor feedback inline.

An alternative/follow-up to this would be to improve our popups more generally to include general & specific keys, as per #1002, so we could include c in the popup itself.

neiljp · 2024-02-22T01:08:18Z

zulipterminal/ui_tools/views.py

@@ -1939,14 +1939,21 @@ def __init__(

        # Get rendered message header and footer
        msg_box = MessageBox(message, controller.model, None)
+        msg_box.footer = [


This changes the existing behavior, since the footer stores footlinks and reactions in message boxes.

Yes I noticed that but it's still a widget_list that is being returned and since originally footer was an empty list in the FullRawMsgView so the earlier assertion passes so didn't need to change anything in test_init.

Adding two new assertions now to check if the widget_list element is urwid.Text and to check if the text is what it's supposed to be.

neiljp · 2024-02-22T01:11:11Z

zulipterminal/ui_tools/views.py

+        msg_box.footer = [
+            (
+                urwid.Text(
+                    ("msg_bold", "\nPress [c] to copy message to clipboard"),


For specific keys, we always want to avoid hard-coding the key in the text, and rather lookup the key name via it's symbolic name: COPY_MESSAGE.

Yes that makes more sense! Making that change.

neiljp · 2024-02-22T01:14:18Z

tests/ui_tools/test_popups.py

+        "key",
+        {
+            *keys_for_command("COPY_MESSAGE"),
+        },


nit: this fits on one line? It's probably only the extra comma(s) that's forcing black to spread it out.

Agreed. Changed.

neiljp · 2024-02-22T01:19:44Z

zulipterminal/ui_tools/views.py

+        msg_box.footer = [
+            (
+                urwid.Text(
+                    ("msg_bold", "\nPress [c] to copy message to clipboard"),


I'm not sure of the wording here, but including content in the text seems useful, particularly if we keep the footer and header as they are in main.

How's "Press [c] to copy raw message content to clipboard" ?

neiljp · 2024-02-22T01:30:19Z

zulipterminal/ui_tools/views.py

-        response = controller.model.fetch_raw_message_content(message["id"])
-
-        if response is None:
+        self.response = controller.model.fetch_raw_message_content(message["id"])


Since we're using this more widely now, it would be useful to give it a name which is clearer - so that we don't have to return here to look up what it means. That seems to be what you do when you use it on line ~1978?

You could also consider this change to be a refactor prep commit, since you've not updated test_init.

You mean changing the variable name to something like self.raw_message_content?

I'll create a different refactor PR changing the variable name and then rebase this one.

neiljp · 2024-02-22T01:44:51Z

docs/hotkeys.md

@@ -64,6 +64,7 @@
 |View current message in browser (from message information)|<kbd>v</kbd>|
 |Show/hide full rendered message (from message information)|<kbd>f</kbd>|
 |Show/hide full raw message (from message information)|<kbd>r</kbd>|
+|Copy message content to clipboard|<kbd>c</kbd>|


This doesn't have any (from ...) suffix like the others, which could imply that it works just when a message is selected.

Missed that text. Adding (from message information). Or should I add something like (from raw message view)? Although it might be confusing for new users since they might not know where raw message view is.

Using c as the key to copy messages to clipboard. Adding c to be excluded since c is already being used in same group.

Adding a feature to copy the text you see in the raw msg popup. Added hint text in the popup. Updated tests for the same.

zulipbot added the size: M [Automatic label added by zulipbot] label Feb 14, 2024

Sushmey force-pushed the copy-raw-message branch from 6e5d8e5 to a6c7240 Compare February 14, 2024 14:24

neiljp reviewed Feb 22, 2024

View reviewed changes

neiljp added missing feature: user A missing feature for all users, present in another Zulip client PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Feb 22, 2024

keys: Add key to copy to clipboard.

079ef3e

Using c as the key to copy messages to clipboard. Adding c to be excluded since c is already being used in same group.

Sushmey force-pushed the copy-raw-message branch from a6c7240 to c969c1e Compare February 22, 2024 11:00

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Feb 22, 2024

views: Add support for copying raw message.

17a008d

Adding a feature to copy the text you see in the raw msg popup. Added hint text in the popup. Updated tests for the same.

Sushmey force-pushed the copy-raw-message branch from c969c1e to 17a008d Compare February 25, 2024 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for copying raw message. #1470

Add support for copying raw message. #1470

Sushmey commented Feb 14, 2024

neiljp left a comment

neiljp Feb 22, 2024

Sushmey Feb 22, 2024

neiljp Feb 22, 2024

Sushmey Feb 22, 2024

neiljp Feb 22, 2024

Sushmey Feb 22, 2024

neiljp Feb 22, 2024

Sushmey Feb 22, 2024

neiljp Feb 22, 2024

Sushmey Feb 22, 2024

neiljp Feb 22, 2024

Sushmey Feb 22, 2024

Add support for copying raw message. #1470

Are you sure you want to change the base?

Add support for copying raw message. #1470

Conversation

Sushmey commented Feb 14, 2024

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

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

Choose a reason for hiding this comment