[WIP] Handle media links #740

preetmishra · 2020-07-25T18:18:26Z

Thanks to @amanagr for his initial work in #359. 👍

The PR extracts a few common elements from the PR but has changes to make the function compatible with MessageLinkButton.

Commits

The first commit is cherry-picked from Handle internal links #708.
The second commit is from WIP: Support displaying of images in msg in the default viewer. #359 with a few amendments that were necessary to make it work with MessageLinkButton.
The third commit integrates the introduced function in MesssageLinkButton to handle media links.
The fourth commit makes open_media asynch and shows downloading updates in the footer.
The fifth commit is a refactor that extracts PopUpConfirmationView instantiation for its subsequent commit where it is used in open_media.

Potential follow-up

Skip downloading existing media.

I would greatly appreciate any early feedback and bug/caveats report.

Partially fixes #764.

zulipterminal/helper.py

neiljp

@preetmishra Thanks for working on this and pushing this forward 👍 We may be able to merge the core commits quickly and leave the UI tuning for later PRs or merged later from this PR.

zulipterminal/helper.py

tests/helper/test_helper.py

zulipterminal/helper.py

zulipterminal/core.py

zulipterminal/helper.py

Tests added and amendments made by Preet Mishra to make it usable for MessageLinkButton and PopUpConfirmationView later. Co-authored by: Preet Mishra <[email protected]>

Tests amended.

Test amended.

preetmishra · 2020-09-07T15:53:24Z

@andersk, @neiljp Thanks for the review. I have updated the pull request to use the safer NamedTemporaryFile and also incorporated the other in-line suggestions.

Re UI tuning, agreed; the pull request only has a minimal downloading setup. We could follow up on this with an extended view separately.

The parameter is useful for prompts that have long descriptions. Used 'center' location for show_media_confirmation_popup().

zulipbot · 2020-09-11T14:09:28Z

Hello @preetmishra, 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

@preetmishra The latest conflict is small, and I wanted to get this in soon if we can - having just tested it again it seems to work fine, and it'll be great to have merged 👍

It would be good if we could keep the UI out of the helper code, which may require a little restructuring of the control flow but should be fairly straightforward.

@amanagr Could you (or a delegated WSL contributor) check the feasibility of explorer on WSL, as used in this PR?

@andersk Do you have further thoughts or concerns here?

neiljp · 2020-11-23T03:07:47Z

zulipterminal/helper.py

+            error = [
+                ' The command ', ('bold', command), ' did not run successfully'
+                '. Exited with ', ('bold', str(exit_status)),
+            ]
+    except FileNotFoundError:
+        error = [' The command ', ('bold', command), ' could not be found']
+
+    if error:
+        controller.view.set_footer_text(error, duration=3)


Can we keep the UI-centric parts out of helper.py?

neiljp · 2020-11-23T03:19:36Z

zulipterminal/helper.py

+    if LINUX:
+        command = 'xdg-open'
+    elif WSL:
+        command = 'explorer'  # Needs testing.
+    elif MACOS:
+        command = 'open'


minor: It would be useful if the user was able to know what the file was going to be opened with in the popup (see other comment)

neiljp · 2020-11-23T03:26:26Z

zulipterminal/ui_tools/buttons.py

+            if isinstance(self.controller.loop.widget, urwid.Overlay):
+                self.controller.exit_popup()


What does this check avoid? I know you've used it elsewhere, but I don't remember what the conclusion was - can we discuss and then maybe add a comment. Does it belong in exit_popup?

@preetmishra could you elaborate this?

@Ezio-Sarthak The pop-ups open in an urwid Overlay. This check is to make sure exit_popup is only called when an Overlay exist.

Ah I see that now. If I avoid that check, there is some unwanted synchronous behavior in footer and popup, if that makes any sense.

neiljp · 2020-11-23T03:29:30Z

zulipterminal/helper.py

+                    controller.view.set_footer_text([
+                        ' Downloading ', ('bold', media_name),
+                    ])
+        controller.view.set_footer_text([' Downloaded ', ('bold', media_name)],
+                                        duration=3)


I'm not sure if this will end up needing it's own commit, but perhaps make these callbacks to keep the UI separate in any case?

Now that we have report_* methods, would it be OK to use here?

neiljp · 2020-11-23T03:31:57Z

zulipterminal/core.py

+            'Your requested media has been downloaded at ',
+            ('bold', media_path),
+            '. Do you want to view it?'


Given the length of the download URLs for some files, this might well benefit from more vertical spacing, eg.

Your requested media has been downloaded to: <filename> Do you want the application to open it with <tool>?

neiljp · 2020-11-23T03:39:32Z

zulipterminal/helper.py

@@ -678,7 +678,7 @@ def process_media(controller: Any, media_link: str) -> None:
    elif MACOS:
        command = 'open'

-    open_media(controller, command, media_path)
+    controller.show_media_confirmation_popup(open_media, command, media_path)


I'm guessing you're trying to synchronize the asynch download terminating?

I'm not sure if I fully understood your comment, but I think the callback would be working fine in any case?

neiljp · 2020-11-23T03:43:51Z

zulipterminal/ui_tools/views.py

@@ -1006,9 +1006,13 @@ def __init__(self, controller: Any, title: str) -> None:
        super().__init__(controller, widgets, 'HELP', popup_width, title)


+PopUpConfirmationViewLocation = Literal['top-left', 'center']


I think this comment should go before the previous one, as there is nowhere near enough space in the top-left for showing the download information :)

You mean commit?

amanagr · 2020-11-23T15:40:15Z

(Sorry I no longer have access to WSL)

neiljp · 2020-12-12T07:49:52Z

It may be better to use os.startfile on WSL? This still needs testing/exploration, since potentially the appropriate WSL version has xdg features we can use in any case?

zulipbot · 2021-01-30T20:34:26Z

Heads up @preetmishra, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2022-05-16T21:52:26Z

The followup to this was merged with slight adjustments in #1223 🎉

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

zulipbot added the size: L [Automatic label added by zulipbot] label Jul 25, 2020

preetmishra mentioned this pull request Jul 25, 2020

Handle internal links #708

Merged

preetmishra force-pushed the feat-handle-images branch from d6a8fd6 to 7ba52ef Compare July 31, 2020 14:20

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jul 31, 2020

preetmishra force-pushed the feat-handle-images branch from 7ba52ef to 86b1955 Compare July 31, 2020 14:27

preetmishra added the PR needs review PR requires feedback to proceed label Jul 31, 2020

preetmishra changed the title ~~[WIP] Handle media links~~ Handle media links Jul 31, 2020

preetmishra force-pushed the feat-handle-images branch 2 times, most recently from 83586a7 to 6c60a48 Compare August 6, 2020 17:19

preetmishra commented Aug 6, 2020

View reviewed changes

zulipterminal/helper.py Outdated Show resolved Hide resolved

preetmishra force-pushed the feat-handle-images branch from 6c60a48 to daf0ffe Compare August 6, 2020 17:33

preetmishra marked this pull request as ready for review August 6, 2020 17:37

preetmishra mentioned this pull request Aug 6, 2020

TRACKING: Handle message links #764

Open

4 tasks

preetmishra changed the title ~~Handle media links~~ [AWAITING] Handle media links Aug 9, 2020

preetmishra removed the PR needs review PR requires feedback to proceed label Aug 14, 2020

preetmishra changed the title ~~[AWAITING] Handle media links~~ [WIP] Handle media links Aug 16, 2020

preetmishra force-pushed the feat-handle-images branch 2 times, most recently from dac7a73 to c79d297 Compare August 22, 2020 15:17

preetmishra changed the title ~~[WIP] Handle media links~~ Handle media links Aug 22, 2020

preetmishra added the PR needs review PR requires feedback to proceed label Aug 22, 2020

neiljp reviewed Aug 29, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 29, 2020

andersk requested changes Aug 29, 2020

View reviewed changes

zulipterminal/helper.py Outdated Show resolved Hide resolved

preetmishra changed the title ~~Handle media links~~ [WIP] Handle media links Aug 30, 2020

amanagr and others added 4 commits September 7, 2020 20:38

helper: Add process_media() and open_media() to handle media links.

5374679

Tests added and amendments made by Preet Mishra to make it usable for MessageLinkButton and PopUpConfirmationView later. Co-authored by: Preet Mishra <[email protected]>

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

8b20d3d

Tests amended.

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

9d45337

core/helper: Show PopUpConfirmationView before opening any media.

ef32f4a

Test amended.

preetmishra force-pushed the feat-handle-images branch from c79d297 to ef32f4a Compare September 7, 2020 15:43

preetmishra changed the title ~~[WIP] Handle media links~~ Handle media links Sep 7, 2020

preetmishra added the PR needs review PR requires feedback to proceed label Sep 7, 2020

core/views: Extend PopUpConfirmationView to accept location parameter.

dad0837

The parameter is useful for prompts that have long descriptions. Used 'center' location for show_media_confirmation_popup().

neiljp reviewed Nov 23, 2020

View reviewed changes

neiljp mentioned this pull request Nov 23, 2020

Opens message links popup menu #424

Closed

preetmishra changed the title ~~Handle media links~~ [WIP] Handle media links Nov 23, 2020

preetmishra removed the PR needs review PR requires feedback to proceed label Jan 17, 2021

neiljp added this to the Release after next milestone Jan 28, 2021

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

zulipbot added the has conflicts label Jan 30, 2021

Ezio-Sarthak mentioned this pull request Jun 28, 2021

Add media support in ZT #1058

Closed

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

neiljp mentioned this pull request May 16, 2022

TRACKING: Message rendering bugs/enhancements #452

Open

12 tasks

neiljp closed this May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Handle media links #740

[WIP] Handle media links #740

preetmishra commented Jul 25, 2020 •

edited

Loading

neiljp left a comment

preetmishra commented Sep 7, 2020

zulipbot commented Sep 11, 2020

neiljp left a comment

neiljp Nov 23, 2020

neiljp Nov 23, 2020

neiljp Nov 23, 2020

Ezio-Sarthak Jun 28, 2021

preetmishra Jun 28, 2021

Ezio-Sarthak Jun 28, 2021

neiljp Nov 23, 2020

Ezio-Sarthak Jun 22, 2021

neiljp Nov 23, 2020

neiljp Nov 23, 2020

Ezio-Sarthak Jun 22, 2021

neiljp Nov 23, 2020

Ezio-Sarthak Jun 28, 2021

amanagr commented Nov 23, 2020

neiljp commented Dec 12, 2020

zulipbot commented Jan 30, 2021

neiljp commented May 16, 2022

		if isinstance(self.controller.loop.widget, urwid.Overlay):
		self.controller.exit_popup()

		@@ -1006,9 +1006,13 @@ def __init__(self, controller: Any, title: str) -> None:
		super().__init__(controller, widgets, 'HELP', popup_width, title)


		PopUpConfirmationViewLocation = Literal['top-left', 'center']

[WIP] Handle media links #740

[WIP] Handle media links #740

Conversation

preetmishra commented Jul 25, 2020 • edited Loading

Commits

Potential follow-up

neiljp left a comment

Choose a reason for hiding this comment

preetmishra commented Sep 7, 2020

zulipbot commented Sep 11, 2020

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

Choose a reason for hiding this comment

amanagr commented Nov 23, 2020

neiljp commented Dec 12, 2020

zulipbot commented Jan 30, 2021

neiljp commented May 16, 2022

preetmishra commented Jul 25, 2020 •

edited

Loading