Add support for copying code snippets #1455

rsashank · 2023-12-25T20:05:26Z

What does this PR do, and why?

Introduces the ability to copy code snippets, building upon the groundwork laid by @yuktasarode, @kingjuno, and @neiljp in previous pull requests: #1134 #1353.

This PR incorporates feedback from earlier pull requests. It introduces the k keybinding for copying code from a Message Information Popup. This PR also resolves the previous issue of a flashing cursor on the button.

Additionally, it includes a minor refactoring in copy_to_clipboard to address return carriage differences in WSL.

External discussion & connections

Discussed in #zulip-terminal in Copy code snippets #T1353 #T1134 (fix #T1123)
Fully fixes Add support for copying code snippets #1123
Partially fixes issue #
Builds upon previous unmerged work in PR buttons/messages/views: Add support for copying code snippets. #1353
Is a follow-up to work in PR #
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

CopyCodeSnippet.mp4

rsashank · 2023-12-25T21:13:51Z

I removed the expected_code_width parameter from the test_create_code_snippet_buttons test. The expected width of code snippets seems to vary across different versions of Ubuntu and MacOS.

Should I reintroduce the test with platform-specific expected code widths, or is it acceptable to leave it removed?

rsashank · 2023-12-25T21:18:08Z

@zulipbot add "PR needs review"

zulipterminal/core.py

rsashank · 2024-04-14T09:44:56Z

Updated this PR :)
@zulipbot add "PR needs review"

zulipbot · 2024-04-14T09:44:59Z

ERROR: Label "PR needs review" already exists and was thus not added to this pull request.

neiljp

@rsashank Thanks for taking this forward 👍

I know this was a followup on previous work, so some of my comments aren't necessarily directed at your approach.

That said, some commits likely benefit from co-author labelling, which you've handled previously.

I'm not sure where the 'Code snippet' term came from originally, but I wonder if we would be better served by using Code block instead?

For now we may want to hold off on the last commit since

Viewing Actions generally applies to the message itself, not one of the selected buttons
We don't have copy for other actions
This will be cleaner once we have Sushmey's category headings in place

I'd like us to remember to do some refactoring of the chunks of metadata we're passing around everywhere, that we'll add to here ;) Doing that first would simplify this PR, but would also delay it.

zulipterminal/core.py

zulipterminal/ui_tools/views.py

zulipterminal/ui_tools/messages.py

zulipterminal/ui_tools/buttons.py

neiljp · 2024-04-15T00:14:39Z

zulipterminal/ui_tools/buttons.py

+    def copy_to_clipboard(self, *_: Any) -> None:
+        self.controller.copy_to_clipboard(self.copy_code, "Code")


It looks like you changed this compared to the previous PR? ("code" -> "Code"?)

I think it would be clearer to change the clipboard method, since it's using the string passed as output.

If a message has multiple code blocks, it may also be useful to indicate which one is copied!

I've updated this method to get copy_code only on demand as suggested in another comment. I've also made it indicate which code block has been copied.

Is that fine?

neiljp · 2024-04-15T00:22:06Z

zulipterminal/ui_tools/buttons.py

+    def get_code_from_snippet(
+        self, snippet_list: List[Tuple[str, str]]
+    ) -> Tuple[List[Tuple[str, str]], str]:


Following on some other comments, this function may not be necessary in the same way, but:

See my general comment re the naming we've ended up with here, but in regard to this method:

This method returns two things, which the method name does not suggest is the case

The returned items are described in the comment; fine details are good there, but the name should describe what it does as much as possible :)

I've revised this method to focus solely on extracting display_code from snippet_list.

neiljp · 2024-04-15T00:50:05Z

zulipterminal/ui_tools/views.py

+            display_code, copy_code = CodeSnippetButton.get_code_from_snippet(
+                self, snippet_list
+            )


There doesn't appear to be a change here compared to the previous PR, where I made a comment. I'll add some different notes here since I'm looking at it afresh.

Passing in a self here which is completely different from the other self (a different class & hierarchy) is quite a 'code smell' that something is going oddly. It's working OK since the method doesn't even use self! We're using the function before we initialize a button to have an appropriate self, if we needed one.

The other aspect is that we're using this method to get the raw code (copy_code), and then only passing it into the button initializer - which the button could calculate itself once it has the full 'code snippet' data, and only on demand if it is copied.

Similarly, if the button had the code snippet data, it could provide the display_code, or even just the widths - so this logic could be retained inside the button itself.

Got it! That makes sense 👍

I've updated the copy_code to only be extracted from snippet_list on demand. Also updated this function to extract display_code from snippet_list once the Button is initialized.

zulipterminal/ui_tools/messages.py

neiljp · 2024-04-15T01:03:45Z

zulipterminal/ui_tools/messages.py

+        (
+            content,
+            self.message_links,
+            self.code_snippets,
+            self.time_mentions,
+        ) = self.transform_content(self.message["content"], self.model.server_url)


It'd be good to see some tests for what ends up in self.code_snippets, though we don't seem to have tests for the others.

I'm yet to work on adding these tests and was wondering if it would be alright moving this to a separate PR? I'll also include tests for the others as well.

Added a test for self.code_snippets. I'll add tests for the others in a separate PR 👍

Made a PR for tests for the others :)

neiljp · 2024-04-15T01:14:17Z

zulipterminal/ui_tools/messages.py

+                if code_language != "Text only":
+                    metadata["code_snippets"].append((code_language, code_snippet))


Do we have an example of this? There are no tests or comments to explain this.

Also, what happens for this when data-code-language is absent?

I've seen cases where it looks like no language tag was applied and the code is not highlighted (what language should we show?)

However, also cases where the code is highlighted, and the source message has a language tag - but the current code also thinks there is no language present. This suggests another field was used previously?

It's possible the language data was not available, or was tagged elsewhere, in a previous version of the Zulip markup. This was hinted at in zulip/zulip#11618.

When you put text after the 3 backticks (```), it causes the code_language to be "Text only". It seems they intended to avoid including that as a code snippet for some reason.

I'm looking into where the language data was previously tagged.

Niloth-p

Great work, Sashank!
Ran it, and it works fine on Linux.

I noticed a few changes suggested by Neil, that are pending.

Renaming: 'code blocks' instead of 'code snippets'. Which I noticed you've changed in the footer text, but other occurrences remain.
Adding attributions
The ambiguity about the languages part

Follow-up PR: metadata refactor

zulipterminal/core.py

tests/ui_tools/test_popups.py

zulipterminal/ui_tools/messages.py

Niloth-p · 2024-06-03T19:47:31Z

zulipterminal/ui_tools/views.py

+                    len(max(code_snippet_widgets[index].display_code, key=len)),
+                )
+
+            code = caption + str(snip[1] for snip in snippet_list)


NIT: snip? Could a more descriptive variable name be used instead?

Any suggestions in mind? :)

zulipterminal/ui_tools/buttons.py

rsashank · 2024-06-05T08:01:58Z

Thanks for the detailed feedback @Niloth-p!

I've updated the PR. I still need to figure out where the language data was tagged in the previous version of Zulip markup, but I believe everything else should be addressed now. :)

Adjusted clipboard_text by replacing \r\n with \n to accommodate carriage returns in WSL. Ensures consistent newline handling for comparison. This prevents unnecessary "text does not match" warnings for WSL users when copying texts containing newline characters.

This class inherits from urwid.Button and enables the creation of code block buttons that copy code to the clipboard when activated. Co-authored-by: kingjuno <[email protected]> Co-authored-by: yuktasarode <[email protected]>

Updated existing tests. New test added. Co-authored-by: kingjuno <[email protected]> Co-authored-by: yuktasarode <[email protected]>

Introduces support for copying code blocks in the message information popup. Makes use of the CodeBlockButton class and its methods. Tests added. Co-authored-by: kingjuno <[email protected]> Co-authored-by: yuktasarode <[email protected]> Fixes zulip#1123.

zulipbot added size: XL [Automatic label added by zulipbot] good first issue Good for newcomers missing feature: user A missing feature for all users, present in another Zulip client version parity: 4 labels Dec 25, 2023

rsashank force-pushed the codesnippet1123 branch from e806d56 to df786e5 Compare December 25, 2023 20:13

rsashank changed the title ~~Add support for copying code snippets~~ WIP: Add support for copying code snippets Dec 25, 2023

rsashank force-pushed the codesnippet1123 branch 3 times, most recently from d7a1b65 to 44b035b Compare December 25, 2023 21:00

rsashank changed the title ~~WIP: Add support for copying code snippets~~ Add support for copying code snippets Dec 25, 2023

zulipbot added the PR needs review PR requires feedback to proceed label Dec 25, 2023

rsashank force-pushed the codesnippet1123 branch 3 times, most recently from 2b8be1f to a012674 Compare December 25, 2023 21:29

rsashank force-pushed the codesnippet1123 branch 6 times, most recently from 45201c8 to fee1ed8 Compare February 10, 2024 11:18

rsashank force-pushed the codesnippet1123 branch 3 times, most recently from 802ab9c to e7929db Compare April 14, 2024 09:23

rsashank commented Apr 14, 2024

View reviewed changes

zulipterminal/core.py Outdated Show resolved Hide resolved

neiljp reviewed Apr 18, 2024

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Apr 18, 2024

rsashank mentioned this pull request May 29, 2024

messages: Add tests for transform_content. #1507

Open

17 tasks

zormit requested a review from Niloth-p May 29, 2024 19:54

rsashank force-pushed the codesnippet1123 branch 2 times, most recently from a7d74f6 to 7e27991 Compare June 3, 2024 11:23

Niloth-p reviewed Jun 3, 2024

View reviewed changes

Niloth-p removed the PR needs buddy review label Jun 3, 2024

rsashank force-pushed the codesnippet1123 branch 6 times, most recently from a7d3191 to 4268b0b Compare June 5, 2024 07:59

rsashank added the PR needs review PR requires feedback to proceed label Jun 5, 2024

rsashank added the PR needs mentor review label Jun 19, 2024

rsashank force-pushed the codesnippet1123 branch 4 times, most recently from 59194bd to 33bfce1 Compare July 8, 2024 21:59

This was referenced Jul 19, 2024

Group message data #1537

Open

Render Spoilers in ZT #1529

Open

rsashank force-pushed the codesnippet1123 branch 2 times, most recently from b5e97e4 to dfb2e39 Compare September 19, 2024 17:12

rsashank and others added 3 commits September 19, 2024 22:47

buttons: Add CodeBlockButton class.

9677d19

This class inherits from urwid.Button and enables the creation of code block buttons that copy code to the clipboard when activated. Co-authored-by: kingjuno <[email protected]> Co-authored-by: yuktasarode <[email protected]>

messages/views: Add markup to code blocks.

1fece13

Updated existing tests. New test added. Co-authored-by: kingjuno <[email protected]> Co-authored-by: yuktasarode <[email protected]>

rsashank force-pushed the codesnippet1123 branch 2 times, most recently from c46598b to 964c17a Compare September 19, 2024 17:21

rsashank force-pushed the codesnippet1123 branch from 964c17a to d9cf3b8 Compare September 19, 2024 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for copying code snippets #1455

Add support for copying code snippets #1455

rsashank commented Dec 25, 2023 •

edited

Loading

rsashank commented Dec 25, 2023

rsashank commented Dec 25, 2023

rsashank commented Apr 14, 2024

zulipbot commented Apr 14, 2024

neiljp left a comment

neiljp Apr 15, 2024

rsashank May 28, 2024

neiljp Apr 15, 2024

rsashank May 28, 2024

neiljp Apr 15, 2024

rsashank May 28, 2024

neiljp Apr 15, 2024

rsashank May 28, 2024

rsashank May 29, 2024

rsashank May 29, 2024

neiljp Apr 15, 2024

neiljp Apr 18, 2024

rsashank May 28, 2024 •

edited

Loading

Niloth-p left a comment

Niloth-p Jun 3, 2024

rsashank Jun 4, 2024

rsashank commented Jun 5, 2024

		def copy_to_clipboard(self, *_: Any) -> None:
		self.controller.copy_to_clipboard(self.copy_code, "Code")

		if code_language != "Text only":
		metadata["code_snippets"].append((code_language, code_snippet))

Add support for copying code snippets #1455

Are you sure you want to change the base?

Add support for copying code snippets #1455

Conversation

rsashank commented Dec 25, 2023 • edited Loading

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

rsashank commented Dec 25, 2023

rsashank commented Dec 25, 2023

rsashank commented Apr 14, 2024

zulipbot commented Apr 14, 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

Choose a reason for hiding this comment

rsashank May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Niloth-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsashank commented Jun 5, 2024

rsashank commented Dec 25, 2023 •

edited

Loading

rsashank May 28, 2024 •

edited

Loading