-
-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Handle media links #740
Changes from all commits
5374679
8b20d3d
9d45337
ef32f4a
dad0837
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
from functools import wraps | ||
from itertools import chain, combinations | ||
from re import ASCII, match | ||
from tempfile import NamedTemporaryFile | ||
from threading import Thread | ||
from typing import ( | ||
Any, Callable, DefaultDict, Dict, FrozenSet, Iterable, List, Set, Tuple, | ||
|
@@ -14,6 +15,7 @@ | |
from urllib.parse import unquote | ||
|
||
import lxml.html | ||
import requests | ||
from typing_extensions import TypedDict | ||
|
||
|
||
|
@@ -647,3 +649,53 @@ def hash_util_decode(string: str) -> str: | |
# Acknowledge custom string replacements in zulip/zulip's | ||
# zerver/lib/url_encoding.py before unquote. | ||
return unquote(string.replace('.', '%')) | ||
|
||
|
||
@asynch | ||
def process_media(controller: Any, media_link: str) -> None: | ||
media_name = media_link.split('/')[-1] | ||
client = controller.client | ||
auth = requests.auth.HTTPBasicAuth(client.email, client.api_key) | ||
|
||
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)) as f: | ||
media_path = f.name | ||
for chunk in r.iter_content(chunk_size=8192): | ||
if chunk: # Filter out keep-alive new chunks. | ||
f.write(chunk) | ||
controller.view.set_footer_text([ | ||
' Downloading ', ('bold', media_name), | ||
]) | ||
controller.view.set_footer_text([' Downloaded ', ('bold', media_name)], | ||
duration=3) | ||
Comment on lines
+668
to
+672
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that we have |
||
|
||
if LINUX: | ||
command = 'xdg-open' | ||
elif WSL: | ||
command = 'explorer' # Needs testing. | ||
elif MACOS: | ||
command = 'open' | ||
Comment on lines
+674
to
+679
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
controller.show_media_confirmation_popup(open_media, command, media_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing you're trying to synchronize the asynch download terminating? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I fully understood your comment, but I think the callback would be working fine in any case? |
||
|
||
|
||
@asynch | ||
def open_media(controller: Any, command: str, media_path: str) -> None: | ||
error = [] | ||
try: | ||
process = subprocess.run([command, media_path], | ||
stdout=subprocess.DEVNULL, | ||
stderr=subprocess.DEVNULL) | ||
exit_status = process.returncode | ||
if exit_status != 0: | ||
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) | ||
Comment on lines
+693
to
+701
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we keep the UI-centric parts out of |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
|
||
from zulipterminal.config.keys import is_command_key, keys_for_command | ||
from zulipterminal.helper import ( | ||
StreamData, edit_mode_captions, hash_util_decode, | ||
StreamData, edit_mode_captions, hash_util_decode, process_media, | ||
) | ||
from zulipterminal.urwid_types import urwid_Size | ||
|
||
|
@@ -300,6 +300,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): | ||
self.controller.exit_popup() | ||
Comment on lines
+305
to
+306
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @preetmishra could you elaborate this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Ezio-Sarthak The pop-ups open in an urwid Overlay. This check is to make sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
process_media(self.controller, self.link) | ||
|
||
@staticmethod | ||
def _decode_stream_data(encoded_stream_data: str) -> DecodedStream: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean commit? |
||
|
||
|
||
class PopUpConfirmationView(urwid.Overlay): | ||
def __init__(self, controller: Any, question: Any, | ||
success_callback: Callable[[], None]): | ||
success_callback: Callable[[], None], | ||
location: PopUpConfirmationViewLocation='top-left'): | ||
self.controller = controller | ||
self.success_callback = success_callback | ||
yes = urwid.Button('Yes', self.exit_popup_yes) | ||
|
@@ -1019,15 +1023,31 @@ def __init__(self, controller: Any, question: Any, | |
'No', 4), None, 'selected') | ||
display_widget = urwid.GridFlow([yes, no], 3, 5, 1, 'center') | ||
wrapped_widget = urwid.WidgetWrap(display_widget) | ||
widgets = [question, urwid.Divider(), wrapped_widget] | ||
prompt = urwid.LineBox( | ||
urwid.ListBox( | ||
urwid.SimpleFocusListWalker( | ||
[question, urwid.Divider(), wrapped_widget] | ||
widgets | ||
))) | ||
urwid.Overlay.__init__(self, prompt, self.controller.view, | ||
align="left", valign="top", | ||
width=self.controller.view.LEFT_WIDTH + 1, | ||
height=8) | ||
|
||
if location == 'top-left': | ||
align = 'left' | ||
valign = 'top' | ||
width = self.controller.view.LEFT_WIDTH + 1 | ||
height = 8 | ||
else: | ||
align = 'center' | ||
valign = 'middle' | ||
|
||
max_cols, max_rows = controller.maximum_popup_dimensions() | ||
# +2 to compensate for the LineBox characters. | ||
width = min(max_cols, | ||
max(question.pack()[0], len('Yes'), len('No'))) + 2 | ||
height = min(max_rows, | ||
sum(widget.rows((width,)) for widget in widgets)) + 2 | ||
|
||
urwid.Overlay.__init__(self, prompt, self.controller.view, align=align, | ||
valign=valign, width=width, height=height) | ||
|
||
def exit_popup_yes(self, args: Any) -> None: | ||
self.success_callback() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the length of the download URLs for some files, this might well benefit from more vertical spacing, eg.