Skip to content
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] Add support for viewing a message in the web browser #698

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/hotkeys.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
|New message to a person or group of people|<kbd>x</kbd>|
|Narrow to the stream of the current message|<kbd>s</kbd>|
|Narrow to the topic of the current message|<kbd>S</kbd>|
|View current message in the web browser|<kbd>v</kbd>|
|Narrow to a topic/private-chat, or stream/all-private-messages|<kbd>z</kbd>|
|Add/remove thumbs-up reaction to the current message|<kbd>+</kbd>|
|Add/remove star status of the current message|<kbd>ctrl</kbd> + <kbd>s</kbd> / <kbd>*</kbd>|
Expand Down
16 changes: 16 additions & 0 deletions tests/core/test_core.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
from platform import platform
from typing import Any

Expand Down Expand Up @@ -271,6 +272,21 @@ def test_narrow_to_all_mentions(
msg_ids = {widget.original_widget.message['id'] for widget in widgets}
assert msg_ids == id_list

@pytest.mark.parametrize('server_url', [
'https://chat.zulip.org/',
'https://foo.zulipchat.com/',
])
def test_view_in_browser(self, mocker, controller, message_fixture,
server_url):
# Set DISPLAY environ to be able to run test in Travis
os.environ['DISPLAY'] = ':0'
controller.model.server_url = server_url
mocked_open = mocker.patch(CORE + '.webbrowser.open')

controller.view_in_browser(message_fixture)

mocked_open.assert_called_once_with(controller.url)

def test_main(self, mocker, controller):
controller.view.palette = {
'default': 'theme_properties'
Expand Down
16 changes: 13 additions & 3 deletions tests/ui_tools/test_popups.py
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,17 @@ def test_keypress_exit_popup(self, key, widget_size):
self.msg_info_view.keypress(size, key)
assert self.controller.exit_popup.called

@pytest.mark.parametrize('key', keys_for_command('VIEW_IN_BROWSER'))
def test_keypress_view_in_browser(self, widget_size, message_fixture, key):
size = widget_size(self.msg_info_view)

self.msg_info_view.keypress(size, key)

(self.controller.view_in_browser.
assert_called_once_with(message_fixture))

def test_height_noreactions(self):
expected_height = 3
expected_height = 4
assert self.msg_info_view.height == expected_height

# FIXME This is the same parametrize as MessageBox:test_reactions_view
Expand Down Expand Up @@ -553,8 +562,9 @@ def test_height_reactions(self, message_fixture, to_vary_in_each_message):
self.msg_info_view = MsgInfoView(self.controller, varied_message,
'Message Information', OrderedDict(),
list())
# 9 = 3 labels + 1 blank line + 1 'Reactions' (category) + 4 reactions.
expected_height = 9
# 10 = 4 labels + 1 blank line + 1 'Reactions' (category) + 4
# reactions (excluding 'Message Links').
expected_height = 10
assert self.msg_info_view.height == expected_height

def test_keypress_navigation(self, mocker, widget_size,
Expand Down
5 changes: 5 additions & 0 deletions zulipterminal/config/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ class KeyBinding(TypedDict, total=False):
'help_text': 'Narrow to the topic of the current message',
'key_category': 'msg_actions',
}),
('VIEW_IN_BROWSER', {
'keys': ['v'],
'help_text': 'View current message in the web browser',
'key_category': 'msg_actions',
}),
('TOGGLE_NARROW', {
'keys': ['z'],
'help_text':
Expand Down
15 changes: 14 additions & 1 deletion zulipterminal/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import signal
import sys
import time
import webbrowser
from collections import OrderedDict
from functools import partial
from platform import platform
Expand All @@ -14,8 +15,9 @@

from zulipterminal.api_types import Composition, Message
from zulipterminal.config.themes import ThemeSpec
from zulipterminal.helper import asynch
from zulipterminal.helper import MACOS, WSL, asynch, suppress_output
from zulipterminal.model import Model
from zulipterminal.server_url import near_message_url
from zulipterminal.ui import Screen, View
from zulipterminal.ui_tools.utils import create_msg_box_list
from zulipterminal.ui_tools.views import (
Expand Down Expand Up @@ -364,6 +366,17 @@ def narrow_to_all_mentions(self) -> None:
# (nothing currently requires narrowing around a message id)
self._narrow_to(anchor=None, mentioned=True)

def view_in_browser(self, message: Message) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the other use of this method - but no tests break?

# Truncate extra '/' from the server url.
self.url = near_message_url(self.model.server_url[:-1], message)
Comment on lines +370 to +371
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to use the pre-existing code here, though using urljoin in the new code would mean we don't need to worry about this. I'm guessing the server code avoids this since it knows the url exactly and relies on tests, which we should too really :)

if (not MACOS and not WSL and not os.environ.get('DISPLAY')
and os.environ.get('TERM')):
Comment on lines +372 to +373
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our platform code relies on only one of the platforms being set; we definitely want to improve that handling (see #791), but we should probably be able to rely on the 'other' option being set for now, ie LINUX.

In the more general sense, the webbrowser module can run terminal browsers, so it would be interesting to see how DISPLAY-less systems handle that. Another aspect is whether graphical non-X11 systems set DISPLAY meaningfully?

Did you mean to capitalize the commit title?

# Don't try to open web browser if running without a GUI
return
with suppress_output():
# Suppress anything on stdout or stderr when opening the browser
webbrowser.open(self.url)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the self?


def deregister_client(self) -> None:
queue_id = self.model.queue_id
self.client.deregister(queue_id, 1.0)
Expand Down
20 changes: 20 additions & 0 deletions zulipterminal/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import subprocess
import time
from collections import OrderedDict, defaultdict
from contextlib import contextmanager
from functools import wraps
from itertools import chain, combinations
from re import ASCII, MULTILINE, findall, match
Expand All @@ -14,6 +15,7 @@
Dict,
FrozenSet,
Iterable,
Iterator,
List,
Set,
Tuple,
Expand Down Expand Up @@ -702,3 +704,21 @@ def get_unused_fence(content: str) -> str:
len(max(matches, key=len)) + 1)

return '`' * max_length_fence


@contextmanager
def suppress_output() -> Iterator[None]:
"""Context manager to redirect stdout and stderr to /dev/null.

Adapted from https://stackoverflow.com/a/2323563
"""
out = os.dup(1)
err = os.dup(2)
os.close(1)
os.close(2)
os.open(os.devnull, os.O_RDWR)
try:
yield
finally:
os.dup2(out, 1)
os.dup2(err, 2)
2 changes: 2 additions & 0 deletions zulipterminal/ui_tools/boxes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1551,6 +1551,8 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
self.model.controller.show_msg_info(self.message,
self.message_links,
self.time_mentions)
elif is_command_key('VIEW_IN_BROWSER', key):
self.model.controller.view_in_browser(self.message['id'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails (exception, unsupported environment, etc), could we let the user know?

return key


Expand Down
7 changes: 6 additions & 1 deletion zulipterminal/ui_tools/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1232,7 +1232,10 @@ def __init__(self, controller: Any, msg: Message, title: str,
msg_info = [
('', [('Date & Time', date_and_time),
('Sender', msg['sender_full_name']),
('Sender\'s Email ID', msg['sender_email'])]),
('Sender\'s Email ID', msg['sender_email']),
('View message in the web browser', 'Press {}'.format(
', '.join(map(repr, keys_for_command('VIEW_IN_BROWSER'))))),
]),
]
# Only show the 'Edit History' label for edited messages.
self.show_edit_history_label = (
Expand Down Expand Up @@ -1303,6 +1306,8 @@ def keypress(self, size: urwid_Size, key: str) -> str:
message_links=self.message_links,
time_mentions=self.time_mentions,
)
elif is_command_key('VIEW_IN_BROWSER', key):
self.controller.view_in_browser(self.msg)
return super().keypress(size, key)


Expand Down