diff --git a/rtv/__main__.py b/rtv/__main__.py index 994db2c..0280a4f 100644 --- a/rtv/__main__.py +++ b/rtv/__main__.py @@ -23,13 +23,21 @@ except ImportError: sys.exit('Fatal Error: Your python distribution appears to be missing ' '_curses.so.\nWas it compiled without support for curses?') +# If we want to override the $BROWSER variable that the python webbrowser +# references, it needs to be done before the webbrowser module is imported +# for the first time. +webbrowser_import_warning = ('webbrowser' in sys.modules) +RTV_BROWSER, BROWSER = os.environ.get('RTV_BROWSER'), os.environ.get('BROWSER') +if RTV_BROWSER: + os.environ['BROWSER'] = RTV_BROWSER + from . import docs from . import packages from .packages import praw from .config import Config, copy_default_config, copy_default_mailcap from .oauth import OAuthHelper from .terminal import Terminal -from .objects import curses_session, Color +from .objects import curses_session, Color, patch_webbrowser from .subreddit_page import SubredditPage from .exceptions import ConfigError from .__version__ import __version__ @@ -110,8 +118,8 @@ def main(): ('$XDG_CONFIG_HOME', os.getenv('XDG_CONFIG_HOME')), ('$RTV_EDITOR', os.getenv('RTV_EDITOR')), ('$RTV_URLVIEWER', os.getenv('RTV_URLVIEWER')), - ('$RTV_BROWSER', os.getenv('RTV_BROWSER')), - ('$BROWSER', os.getenv('BROWSER')), + ('$RTV_BROWSER', RTV_BROWSER), + ('$BROWSER', BROWSER), ('$PAGER', os.getenv('PAGER')), ('$VISUAL', os.getenv('VISUAL')), ('$EDITOR', os.getenv('EDITOR'))] @@ -139,6 +147,8 @@ def main(): warnings.warn(text) config['ascii'] = True + _logger.info('RTV module path: %s', os.path.abspath(__file__)) + # Check the praw version if packages.__praw_bundled__: _logger.info('Using packaged PRAW distribution, ' @@ -147,6 +157,12 @@ def main(): _logger.info('Packaged PRAW not found, falling back to system ' 'installed version %s', praw.__version__) + # Update the webbrowser module's default behavior + patch_webbrowser() + if webbrowser_import_warning: + _logger.warning('webbrowser module was unexpectedly imported before' + '$BROWSER could be overwritten') + # Construct the reddit user agent user_agent = docs.AGENT.format(version=__version__) diff --git a/rtv/mime_parsers.py b/rtv/mime_parsers.py index d04b2fc..e70515d 100644 --- a/rtv/mime_parsers.py +++ b/rtv/mime_parsers.py @@ -164,7 +164,9 @@ class ImgurApiMIMEParser(BaseMIMEParser): https://apidocs.imgur.com """ CLIENT_ID = None - pattern = re.compile(r'https?://(w+\.)?(m\.)?imgur\.com/((?Pa|album|gallery)/)?(?P[a-zA-Z0-9]+)$') + pattern = re.compile( + r'https?://(w+\.)?(m\.)?imgur\.com/' + r'((?Pa|album|gallery)/)?(?P[a-zA-Z0-9]+)$') @classmethod def get_mimetype(cls, url): diff --git a/rtv/objects.py b/rtv/objects.py index 48f0074..d58b37d 100644 --- a/rtv/objects.py +++ b/rtv/objects.py @@ -3,12 +3,14 @@ from __future__ import unicode_literals import re import os +import sys import time import signal import inspect import weakref import logging import threading +import webbrowser import curses import curses.ascii from contextlib import contextmanager @@ -23,6 +25,31 @@ from .packages import praw _logger = logging.getLogger(__name__) +def patch_webbrowser(): + """ + Patch webbrowser on macOS to support setting BROWSER=firefox, + BROWSER=chrome, etc.. + + https://bugs.python.org/issue31348 + """ + + if sys.platform != 'darwin' or 'BROWSER' not in os.environ: + return + + # This is a copy of what's at the end of webbrowser.py, except that + # it adds MacOSXOSAScript entries instead of GenericBrowser entries. + _userchoices = os.environ["BROWSER"].split(os.pathsep) + for cmdline in reversed(_userchoices): + if cmdline in ('safari', 'firefox', 'chrome', 'default'): + browser = webbrowser.MacOSXOSAScript(cmdline) + try: + webbrowser.register(cmdline, None, browser, update_tryorder=-1) + except TypeError: + # 3.7 nightly build changed the method signature + # pylint: disable=unexpected-keyword-arg + webbrowser.register(cmdline, None, browser, preferred=True) + + @contextmanager def curses_session(): """ diff --git a/rtv/terminal.py b/rtv/terminal.py index eadec67..0ebfa08 100644 --- a/rtv/terminal.py +++ b/rtv/terminal.py @@ -13,12 +13,11 @@ import webbrowser import subprocess import curses.ascii from curses import textpad +from multiprocessing import Process from contextlib import contextmanager from tempfile import NamedTemporaryFile import six -#pylint: disable=import-error -from six.moves.urllib.parse import quote from kitchen.text.display import textual_width_chop from . import exceptions @@ -464,11 +463,12 @@ class Terminal(object): python webbrowser will try to determine the default to use based on your system. - For browsers requiring an X display, we call - webbrowser.open_new_tab(url) and redirect stdout/stderr to devnull. - This is a workaround to stop firefox from spewing warning messages to - the console. See http://bugs.python.org/issue22277 for a better - description of the problem. + For browsers requiring an X display, we open a new subprocess and + redirect stdout/stderr to devnull. This is a workaround to stop + BackgroundBrowsers (e.g. xdg-open, any BROWSER command ending in "&"), + from spewing warning messages to the console. See + http://bugs.python.org/issue22277 for a better description of the + problem. For console browsers (e.g. w3m), RTV will suspend and display the browser window within the same terminal. This mode is triggered either @@ -479,40 +479,38 @@ class Terminal(object): headless There may be other cases where console browsers are opened (xdg-open?) - but are not detected here. + but are not detected here. These cases are still unhandled and will + probably be broken if we incorrectly assume that self.display=True. """ if self.display: - # Note that we need to sanitize the url before inserting it into - # the python code to prevent injection attacks. - command = ( - "import webbrowser\n" - "from six.moves.urllib.parse import unquote\n" - "webbrowser.open_new_tab(unquote('%s'))" % quote(url)) - args = [sys.executable, '-c', command] - with self.loader('Opening page in a new window'), \ - open(os.devnull, 'ab+', 0) as null: - p = subprocess.Popen(args, stdout=null, stderr=null) - # Give the browser 5 seconds to open a new tab. Because the + with self.loader('Opening page in a new window'): + + def open_url_silent(url): + # This used to be done using subprocess.Popen(). + # It was switched to multiprocessing.Process so that we + # can re-use the webbrowser instance that has been patched + # by RTV. It's also safer because it doesn't inject + # python code through the command line. + null = open(os.devnull, 'ab+', 0) + sys.stdout, sys.stderr = null, null + webbrowser.open_new_tab(url) + + p = Process(target=open_url_silent, args=(url,)) + p.start() + # Give the browser 7 seconds to open a new tab. Because the # display is set, calling webbrowser should be non-blocking. # If it blocks or returns an error, something went wrong. try: - start = time.time() - while time.time() - start < 10: - code = p.poll() - if code == 0: - break # Success - elif code is not None: - raise exceptions.BrowserError( - 'Program exited with status=%s' % code) - time.sleep(0.01) - else: + p.join(7) + if p.is_alive(): raise exceptions.BrowserError( - 'Timeout opening browser') + 'Timeout waiting for browser to open') finally: - # Can't check the loader exception because the oauth module - # supersedes this loader and we need to always kill the - # process if escape is pressed + # This will be hit on the browser timeout, but also if the + # user presses the ESC key. We always want to kill the + # webbrowser process if it hasn't opened the tab and + # terminated by now. try: p.terminate() except OSError: @@ -777,7 +775,7 @@ class Terminal(object): out = '\n'.join(stack) return out - + def clear_screen(self): """ In the beginning this always called touchwin(). However, a bug @@ -787,14 +785,14 @@ class Terminal(object): this in their tmux.conf or .bashrc file which can cause issues. Using clearok() instead seems to fix the problem, with the trade off of slightly more expensive screen refreshes. - + Update: It was discovered that using clearok() introduced a separate bug for urxvt users in which their screen flashed when scrolling. Heuristics were added to make it work with as many configurations as possible. It's still not perfect (e.g. urxvt + xterm-256color) will screen flash, but it should work in all cases if the user sets their TERM correctly. - + Reference: https://github.com/michael-lazar/rtv/issues/343 https://github.com/michael-lazar/rtv/issues/323 diff --git a/scripts/inspect_webbrowser.py b/scripts/inspect_webbrowser.py new file mode 100755 index 0000000..35ef49a --- /dev/null +++ b/scripts/inspect_webbrowser.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python + +""" +Utility script used to examine the python webbrowser module with different OSs. +""" + +import os + +os.environ['BROWSER'] = 'firefox' + +# If we want to override the $BROWSER variable that the python webbrowser +# references, it needs to be done before the webbrowser module is imported +# for the first time. +RTV_BROWSER, BROWSER = os.environ.get('RTV_BROWSER'), os.environ.get('BROWSER') +if RTV_BROWSER: + os.environ['BROWSER'] = RTV_BROWSER + +print('RTV_BROWSER=%s' % RTV_BROWSER) +print('BROWSER=%s' % BROWSER) + +import webbrowser + +print('webbrowser._browsers:') +for key, val in webbrowser._browsers.items(): + print(' %s: %s' % (key, val)) + +print('webbrowser._tryorder:') +for name in webbrowser._tryorder: + print(' %s' % name) + +webbrowser.open_new_tab('https://www.python.org') diff --git a/tests/test_objects.py b/tests/test_objects.py index e3d8213..411fd7c 100644 --- a/tests/test_objects.py +++ b/tests/test_objects.py @@ -5,19 +5,48 @@ import time import curses from collections import OrderedDict +import os import six import pytest import requests +from six.moves import reload_module from rtv import exceptions from rtv.objects import Color, Controller, Navigator, Command, KeyMap, \ - curses_session + curses_session, patch_webbrowser try: from unittest import mock except ImportError: import mock +# webbrowser's command to check if a file exists is different for py2/py3 +if six.PY3: + mock_isfile = mock.patch('shutil.which', return_value=None) +else: + mock_isfile = mock.patch('os.path.isfile', return_value=None) + + +@mock.patch.dict(os.environ, {'BROWSER': 'safari'}) +@mock.patch('sys.platform', 'darwin') +@mock_isfile +def test_patch_webbrowser(*_): + + # Make sure that webbrowser re-generates the browser list using the + # mocked environment + import webbrowser + webbrowser = reload_module(webbrowser) + + # By default, we expect that BROWSER will be loaded as a generic browser + # This is because "safari" is not a valid script in the system PATH + assert isinstance(webbrowser.get(), webbrowser.GenericBrowser) + + # After patching, the default webbrowser should now be interpreted as an + # OSAScript browser + patch_webbrowser() + assert isinstance(webbrowser.get(), webbrowser.MacOSXOSAScript) + assert webbrowser._tryorder[0] == 'safari' + @pytest.mark.parametrize('use_ascii', [True, False]) def test_objects_load_screen(terminal, stdscr, use_ascii): diff --git a/tests/test_terminal.py b/tests/test_terminal.py index 8b341ec..15fd42e 100644 --- a/tests/test_terminal.py +++ b/tests/test_terminal.py @@ -10,7 +10,7 @@ import six import pytest from rtv.docs import HELP, COMMENT_EDIT_FILE -from rtv.exceptions import TemporaryFileError +from rtv.exceptions import TemporaryFileError, BrowserError try: from unittest import mock @@ -464,22 +464,38 @@ def test_open_link_subprocess(terminal): assert get_error() -def test_open_browser(terminal): - - url = 'http://www.test.com' +def test_open_browser_display(terminal): terminal._display = True - with mock.patch('subprocess.Popen', autospec=True) as Popen: - Popen.return_value.poll.return_value = 0 - terminal.open_browser(url) - assert Popen.called + with mock.patch('webbrowser.open_new_tab', autospec=True) as open_new_tab: + terminal.open_browser('http://www.test.com') + + # open_new_tab() will be executed in the child process so we can't + # directly check if the was called from here or not. + # open_new_tab.assert_called_with('http://www.test.com') + + # Shouldn't suspend curses assert not curses.endwin.called assert not curses.doupdate.called + +def test_open_browser_display_no_response(terminal): + + terminal._display = True + with mock.patch('rtv.terminal.Process', autospec=True) as Process: + Process.return_value.is_alive.return_value = 1 + terminal.open_browser('http://www.test.com') + assert isinstance(terminal.loader.exception, BrowserError) + + +def test_open_browser_no_display(terminal): + terminal._display = False with mock.patch('webbrowser.open_new_tab', autospec=True) as open_new_tab: - terminal.open_browser(url) - open_new_tab.assert_called_with(url) + terminal.open_browser('http://www.test.com') + open_new_tab.assert_called_with('http://www.test.com') + + # Should suspend curses to give control of the terminal to the browser assert curses.endwin.called assert curses.doupdate.called