buttons/messages/views: Add support for copying code snippets. #1353

yuktasarode · 2023-03-24T10:22:26Z

What does this PR do?
This PR aims to add support for copying code snippets and is based off on the work done by @kingjuno and @neiljp in the PR #1134. This PR mainly fixes the tests and linting issues and bring the PR to a merge-able state.

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

This may fix the core elements of #1123.

neiljp

@yuktasarode Thanks for tidying this up further! 👍

It's good to see this passing linting and tests :) It's also working well still, though I've not examined the functionality in great detail yet.

I've left some comments inline, but you may want to first rebase against main, since I recently merged a PR regarding how the 'Action' keys are shown and grouped, which affects this UI.

My note in the other PR (#1134 (comment)) mention a few other changes; I'm not sure if you're already looking at those aspects, but I've left some details below.

I'd suggest the next step to make this easier to work on and read would be to split this into multiple commits. You'll want to confirm how the code links together yourself, but my take would be that we're generating additional data via the message, which will need to go into a button, both of which are combined into the end result - so 3 commits might be a good starting point.

Moving the behavior into the CodeSnippetButton may be another good step, or at least inheriting via urwid.Button like the MessageLinkButton does in the first instance, since I believe it should achieve most of what the custom button does already?

My note in the other PR (#1134 (comment)) also notes regarding adding tests for each commit/change; if you wish to start on that, that's fine, but I expect it'll be a lot easier to keep track of when the commit size is smaller, and you can separate which code change/addition is related to which test change/addition.

neiljp · 2023-03-25T18:01:14Z

zulipterminal/ui_tools/views.py

+            urwid.connect_signal(button, "click", self.copy_to_clipboard, copy_code)
+            display_attr = None if index % 2 else "popup_contrast"


It would be cleaner if we could have this kind of behavior, and that in a few other methods below, defined in the CodeSnippetButton itself instead, like we do with other buttons - I can't see a reason not to do this.

Note how the create_link_button function is simpler compared to this one.

The CodeSnippetButton does very little that's unique right now - it's very close to being a very simple Button.

zulipterminal/ui_tools/views.py

zulipterminal/ui_tools/messages.py

neiljp · 2023-03-25T18:10:01Z

zulipterminal/ui_tools/buttons.py

@@ -647,6 +647,40 @@ def handle_narrow_link(self) -> None:
                self.controller.exit_popup()


+class CodeSnippetButton(urwid.Text):


As mentioned elsewhere, this works, but it would be good to migrate to a button with the specific functionality we want.

I did trying inherting urwid.Button and Moving the behavior into the CodeSnippetButton but I could only move the method get_code_from_snippet() from views.py to buttons.py, since all other methods require the instance of controller to be taken into account. Kindly let me know anyother way to do this.

Right now, all the basic Button functionality currently in CodeSnippetButton except for get_code_from_snippet is as per the default in urwid.Button (you can compare yourself, to wimp.py in urwid).

Look at the initializer for MessageLinkButton - that is how we can set up a controller via the button. We may also want to store the code content (snippet information) in there, for example.

Right now you're using the class name to call a method in a class, and explicitly passing the self value. Normally that'd be a bad idea and often cause an error, but you're not using self in get_code_from_snippet here! Instead, we should be creating CodeSnippetButtons with the relevant data, then using their properties as we need them.

I understand you've inherited this PR, but this (and the UI, using urwid) is using OOP in Python, so you may need to learn more about this if you've not worked on this before.

neiljp · 2023-04-04T20:03:31Z

@yuktasarode Is this ready for review? If so, please change the labels as per my message in the GSoC 2023 topic in zulip-terminal (ie. remove needing update and add needs review).

yuktasarode · 2023-04-06T07:18:12Z

@zulipbot add "PR needs review"

yuktasarode · 2023-04-06T07:18:37Z

@zulipbot remove "PR awaiting update"

neiljp · 2023-04-08T23:04:58Z

@yuktasarode Could you please rebase this after the recent changes we had in main, and further explore how you might simplify and initialize the snippet button like we do with the MessageLinkButton? (given my comment above)

You're welcome to take up another issue also, of course 👍

Made the button inhert urwid.button and added supporting methods for creating the code snippet button.

Tests updated.

Inherited urwid.button class in create_code_snippet_buttons method. Get the code snippets and copy it on button press. Tests added for create_code_snippet_buttons.

neiljp

@yuktasarode You didn't note this for review, but I saw you had pushed. This looks much closer than before 🎉

neiljp · 2023-04-22T18:23:39Z

zulipterminal/ui_tools/views.py

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


Generally the button vs view separation looks much better, but this stands out. The button should be able to calculate this internally :)

Can you move the initialization of the button up here? It gets the display and copy_code, but that is associated with code related to the rendering. So you should be able to pass in the language/snippet and have a button function provide the data you need externally. It should be able to set the caption itself :)

neiljp · 2023-04-22T18:28:40Z

zulipterminal/ui_tools/buttons.py

+        super().__init__("")
+
+        self.update_widget(display_code, display_attr)
+        urwid.connect_signal(self, "click", self.copy_to_clipboard)


This makes it work for clicks, but not for the keypress. See eg. the StreamButton.

neiljp · 2023-04-22T18:30:06Z

zulipterminal/ui_tools/buttons.py

+        # Set cursor position next to len(caption) to avoid the cursor.
+        icon = urwid.SelectableIcon(display_code, cursor_position=len(display_code) + 1)
+        self._w = urwid.AttrMap(icon, display_attr, focus_map="selected")


This doesn't appear to be working for this button. I see a flashing cursor on the button while testing.

zulipbot added the size: XL [Automatic label added by zulipbot] label Mar 24, 2023

neiljp reviewed Mar 25, 2023

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 Mar 25, 2023

neiljp mentioned this pull request Mar 25, 2023

Add support for copying code snippets #1134

Open

4 tasks

yuktasarode force-pushed the review-1134 branch 2 times, most recently from 86b195c to e94f337 Compare April 4, 2023 06:35

zulipbot added the PR needs review PR requires feedback to proceed label Apr 6, 2023

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Apr 6, 2023

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 Apr 12, 2023

yuktasarode added 3 commits April 14, 2023 00:48

buttons: Added CodeSnippetButton class.

c5b80eb

Made the button inhert urwid.button and added supporting methods for creating the code snippet button.

messages/views: Add markup to code snippets.

a52d4fc

Tests updated.

core/views: Populate code snippet button with data.

6d20d74

Inherited urwid.button class in create_code_snippet_buttons method. Get the code snippets and copy it on button press. Tests added for create_code_snippet_buttons.

yuktasarode force-pushed the review-1134 branch from e94f337 to 6d20d74 Compare April 15, 2023 03:51

neiljp reviewed Apr 22, 2023

View reviewed changes

rsashank mentioned this pull request Dec 25, 2023

Add support for copying code snippets #1455

Open

18 tasks

neiljp added PR replaced by another PR and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buttons/messages/views: Add support for copying code snippets. #1353

buttons/messages/views: Add support for copying code snippets. #1353

yuktasarode commented Mar 24, 2023 •

edited by neiljp

Loading

neiljp left a comment •

edited

Loading

neiljp Mar 25, 2023

neiljp Mar 25, 2023

yuktasarode Apr 6, 2023

neiljp Apr 8, 2023

neiljp commented Apr 4, 2023

yuktasarode commented Apr 6, 2023

yuktasarode commented Apr 6, 2023

neiljp commented Apr 8, 2023

neiljp left a comment

neiljp Apr 22, 2023

neiljp Apr 22, 2023

neiljp Apr 22, 2023

		urwid.connect_signal(button, "click", self.copy_to_clipboard, copy_code)
		display_attr = None if index % 2 else "popup_contrast"

		@@ -647,6 +647,40 @@ def handle_narrow_link(self) -> None:
		self.controller.exit_popup()


		class CodeSnippetButton(urwid.Text):

buttons/messages/views: Add support for copying code snippets. #1353

Are you sure you want to change the base?

buttons/messages/views: Add support for copying code snippets. #1353

Conversation

yuktasarode commented Mar 24, 2023 • edited by neiljp Loading

neiljp left a comment • edited Loading

Choose a reason for hiding this comment

neiljp Mar 25, 2023

Choose a reason for hiding this comment

neiljp Mar 25, 2023

Choose a reason for hiding this comment

yuktasarode Apr 6, 2023

Choose a reason for hiding this comment

neiljp Apr 8, 2023

Choose a reason for hiding this comment

neiljp commented Apr 4, 2023

yuktasarode commented Apr 6, 2023

yuktasarode commented Apr 6, 2023

neiljp commented Apr 8, 2023

neiljp left a comment

Choose a reason for hiding this comment

neiljp Apr 22, 2023

Choose a reason for hiding this comment

neiljp Apr 22, 2023

Choose a reason for hiding this comment

neiljp Apr 22, 2023

Choose a reason for hiding this comment

yuktasarode commented Mar 24, 2023 •

edited by neiljp

Loading

neiljp left a comment •

edited

Loading