Add media support in ZT #1058

Ezio-Sarthak · 2021-06-28T10:12:53Z

Thanks to @preetmishra and @amanagr for their substantial work in #740 and #359, that helped wrapping this PR up with a sober feature version easily.

Partially fixes #764.

Commit flow:

helper: Add download_media() and open_media() to handle media links.
refactor: Use normalized OS-specific file names.
buttons: Use process_media() to handle media via MessageLinkButton.
helper: Show downloading & downloaded update in process_media().
refactor: buttons/helper: Use callbacks to isolate UI-centric parts.
views: Extend PopUpConfirmationView to accept location parameter.
core/helper: Show PopUpConfirmationView before opening any media.

Screenshots/GIFs

zulipbot · 2021-06-28T10:12:58Z

Hello @Ezio-Sarthak, it seems like you have referenced #764 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 #764..

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 It's good to see more work on this 👍

I've not looked at all the tests in detail, but this is some feedback for this version of the PR - have you checked my/others comments from the previous PRs?

There seem to be some changes from the previous iteration in terms of eg. subprocess, so although I've noted some reasons you may want to check how up to date your PR is with that?

neiljp · 2021-07-04T02:08:18Z

zulipterminal/helper.py

+                " The tool ",
+                ("bold", tool),
+                " did not run successfully" ". Exited with ",
+                ("bold", str(exit_status)),


"bold" as a style doesn't exist - we have footer_contrast now.

neiljp · 2021-07-04T02:20:47Z

zulipterminal/helper.py

+    elif LINUX:
+        command = f"xdg-open {media_path}"
+    elif MACOS:
+        command = f"open {media_path}"


These should really be List[str]. There is discussion elsewhere, though I can't seem to dig it up on github right now.

neiljp · 2021-07-04T02:22:04Z

zulipterminal/helper.py

+                    f.write(chunk)
+
+    if WSL:
+        command = f'explorer.exe `wslpath -w "{media_path}"`'  # Needs testing.


Let's leave this out until we know we have a version that works, and we're sure is good, particularly with that quoting.

While we support only these three, we should handle the else case (or equivalent) in any case, so we'll need command to be defined, or have some meaning, and perhaps pop up an 'unsupported' notice or report an error. That means it can apply to the WSL case for now too.

neiljp · 2021-07-04T02:27:41Z

zulipterminal/helper.py

+    error = []
+    try:
+        process = subprocess.run(
+            command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, shell=True


It would be good to avoid shell=True for security reasons (eg. https://docs.python.org/3/library/subprocess.html#security-considerations)

neiljp · 2021-07-04T02:28:24Z

zulipterminal/helper.py

+    with requests.get(media_link, auth=auth, stream=True) as r:
+        r.raise_for_status()
+        with NamedTemporaryFile(
+            mode="wb", delete=False, prefix="zt-", suffix="-{}".format(media_name)


This can now be an f-string.

neiljp · 2021-07-04T02:32:26Z

zulipterminal/helper.py

+
+
+@asynch
+def open_media(controller: Any, command: str, tool: str) -> None:


This is open_media, so it's surprising that the open-command selection is not present here.

It's been a while since I've looked at this code, but it may be clearer not having one function call the other, ie. download_media(url) -> local_path, open_media(local_path) ?

neiljp · 2021-07-04T02:34:59Z

zulipterminal/ui_tools/buttons.py

@@ -370,6 +370,11 @@ def handle_link(self, *_: Any) -> None:
        server_url = self.model.server_url
        if self.link.startswith(urljoin(server_url, "/#narrow/")):
            self.handle_narrow_link()
+        elif self.link.startswith(urljoin(server_url, "/user_uploads/")):
+            # Exit pop-up promptly, let the media download in the background.
+            if isinstance(self.controller.loop.widget, urwid.Overlay):


We should probably refactor this to be a controller method - we already have one case of this style.

neiljp · 2021-07-04T02:39:07Z

zulipterminal/helper.py

@@ -737,7 +737,9 @@ def suppress_output() -> Iterator[None]:


 @asynch
-def process_media(controller: Any, media_link: str) -> None:
+def process_media(
+    controller: Any, media_link: str, callback: Callable[..., None]


callback is a very general name - it's worth clarifying when it's called in the name. This could be after downloading, during downloading...

neiljp · 2021-07-04T02:42:39Z

zulipterminal/helper.py

-                    controller.view.set_footer_text(
-                        [" Downloading ", ("bold", media_name)]
-                    )
+                    callback([" Downloading ", ("bold", media_name)])


This is not coupled to the controller etc, but you need to know that it's an urwid styled-text being passed in. You could specify the parameter in the calling function, and have this just be callback(). A different name would help too - see elsewhere.

Right. I just passed in the params to show the media name as well. But I think showing just Downloading your media ... is good, since I'm prompting the user the filename while opening it in any case 👍

neiljp · 2021-07-04T02:46:15Z

zulipterminal/ui_tools/views.py

-            )
-        )
+        widgets = [question, urwid.Divider(), wrapped_widget]
+        prompt = urwid.LineBox(urwid.ListBox(urwid.SimpleFocusListWalker(widgets)))


A good follow-up to this would be to improve the styling and unify it with the general popup - the border is not that clear compared to those, in retrospect.

prah23

Thanks for bringing this work up to speed @Ezio-Sarthak!

I've tested this on WSL2 (Ubuntu), and it works well for the file formats I've tried: JPEG, PNG, PDF, and MP4. There's a slight issue as it stands, the footer notification for the tool not running successfully is triggered every time I open something. I presume there's a different exit code being returned on Windows or something of the like. You might have to handle that based on the platform.

I notice Neil has also just reviewed, I wanted to quickly put in a suggestion regarding this while you're still working on it so it's easier to accommodate.

prah23 · 2021-07-04T13:53:58Z

zulipterminal/helper.py

+    client = controller.client
+    auth = requests.auth.HTTPBasicAuth(client.email, client.api_key)
+
+    with requests.get(media_link, auth=auth, stream=True) as r:


minor: I understand that the r and f aliases come from the previous PRs but can we use more explicit aliases (maybe file and response)? Since we use variables with explanatory names everywhere else, it would be consistent here.

andersk · 2021-08-09T06:00:14Z

zulipterminal/helper.py

+    if PLATFORM == "WSL":
+        tool = "explorer.exe"
+        # Converting absolute path to WSL path
+        media_path = f'`wslpath "{media_path}"`'


What is interpreting these backticks? What else is it interpreting? Something is definitely wrong here.

Yeah. It is a bad way of nesting bash commands. One should instead use $() I think. As I said in the stream, we'd potentially not need this now.

No, my point is there should not be a Bash. Backticks should not be interpreted here. If they were being interpreted here—even if we’re no longer using them—that indicates a deep problem that we need to investigate, because media_path could itself contain backticks or other characters that we really don’t want to be interpreted.

The same problem applies to $(), "", or any other kind of “quote” construct. The first rule of quoting is that you need to ask, what if the quoting characters are present inside the string you’re quoting?

Makes sense. Thanks for clarifying 👍

Tests added and amendments made by Preet Mishra to make it usable for MessageLinkButton and PopUpConfirmationView later. Tests and code further amended by Sarthak Garg to support using report_* methods for UI calls, and refactoring *_media() methods. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Sarthak Garg <[email protected]>

…des. The return codes for GUI commands, which were to be interpreted as 1 for WSL and 0 for others, were validated as 0 for all before.This commit extracts the return codes to a helper in `platform_code`, such that the OS-specific successful GUI return codes are handy. Tests added.

There is a difference in path types for Windows and Unix systems. Windows uses backward slashes, whereas other systems use forward slashes. Since a media link in WSL needs an absolute path, such normalization is required. This commit extracts the logic to normalize a file path to a helper in `platform_code`. Tests added.

This commit adds a method `process_media` to handle media links, i.e., it Downloads and Shows the media in the default OS-sepecific explorer. Tests added and amended.

Amendment made by Sarthak Garg to use new `report_*` methods for footer notifications. These are to show download status in footer. Co-authored by: Sarthak Garg <[email protected]>

The parameter is useful for prompts that have long descriptions.

Used 'center' location for show_media_confirmation_popup() for better visibilty of the popup message. Test amended.

neiljp · 2022-05-16T21:51:25Z

Merged with slight adjustments in #1223 🎉

Thanks @amanagr, @preetmishra and @Ezio-Sarthak 👍

zulipbot added the size: XL [Automatic label added by zulipbot] label Jun 28, 2021

Ezio-Sarthak force-pushed the add-media-support branch from 21379ec to a09e8fa Compare June 28, 2021 10:15

Ezio-Sarthak added enhancement New feature or request feedback wanted PR needs review PR requires feedback to proceed labels Jun 28, 2021

neiljp reviewed Jul 4, 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 Jul 4, 2021

prah23 suggested changes Jul 4, 2021

View reviewed changes

Ezio-Sarthak force-pushed the add-media-support branch from a09e8fa to 3c0057b Compare August 8, 2021 11:31

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 Aug 8, 2021

andersk reviewed Aug 9, 2021

View reviewed changes

Ezio-Sarthak force-pushed the add-media-support branch 4 times, most recently from 6b5130c to b13cb72 Compare August 12, 2021 16:58

Ezio-Sarthak force-pushed the add-media-support branch 4 times, most recently from eff57ba to 59e3f37 Compare August 21, 2021 06:46

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

amanagr and others added 7 commits April 11, 2022 19:49

buttons: Use process_media() to handle media via MessageLinkButton.

7a26d41

This commit adds a method `process_media` to handle media links, i.e., it Downloads and Shows the media in the default OS-sepecific explorer. Tests added and amended.

helper: Show downloading & downloaded update in process_media().

2d77542

Amendment made by Sarthak Garg to use new `report_*` methods for footer notifications. These are to show download status in footer. Co-authored by: Sarthak Garg <[email protected]>

views: Extend PopUpConfirmationView to accept location parameter.

b6cd40b

The parameter is useful for prompts that have long descriptions.

core/helper: Show PopUpConfirmationView before opening any media.

613a811

Used 'center' location for show_media_confirmation_popup() for better visibilty of the popup message. Test amended.

neiljp force-pushed the add-media-support branch from 59e3f37 to 613a811 Compare April 12, 2022 15:58

neiljp mentioned this pull request Apr 14, 2022

TRACKING: Handle message links #764

Open

4 tasks

neiljp mentioned this pull request May 16, 2022

Media opening (slight reorder & tidy of #1058) #1223

Merged

4 tasks

neiljp added this to the Next Release milestone May 16, 2022

neiljp mentioned this pull request May 16, 2022

TRACKING: Message rendering bugs/enhancements #452

Open

12 tasks

neiljp added the PR replaced by another PR label May 16, 2022

neiljp closed this May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add media support in ZT #1058

Add media support in ZT #1058

Ezio-Sarthak commented Jun 28, 2021 •

edited

Loading

zulipbot commented Jun 28, 2021

neiljp left a comment

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

neiljp Jul 4, 2021

Ezio-Sarthak Aug 8, 2021

neiljp Jul 4, 2021

prah23 left a comment

prah23 Jul 4, 2021

andersk Aug 9, 2021

Ezio-Sarthak Aug 9, 2021

andersk Aug 9, 2021

Ezio-Sarthak Aug 10, 2021

neiljp commented May 16, 2022



		@asynch
		def open_media(controller: Any, command: str, tool: str) -> None:

Add media support in ZT #1058

Add media support in ZT #1058

Conversation

Ezio-Sarthak commented Jun 28, 2021 • edited Loading

zulipbot commented Jun 28, 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

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

prah23 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

neiljp commented May 16, 2022

Ezio-Sarthak commented Jun 28, 2021 •

edited

Loading