From 5499aadffbb1f89a897f552f4d5c3b5f46c5033d Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 00:22:05 -0400 Subject: [PATCH] Adding patch for webbrowser on macOS --- rtv/__main__.py | 9 ++++++- rtv/objects.py | 22 +++++++++++++++++ rtv/terminal.py | 55 +++++++++++++++++++++---------------------- tests/test_objects.py | 26 +++++++++++++++++++- 4 files changed, 82 insertions(+), 30 deletions(-) diff --git a/rtv/__main__.py b/rtv/__main__.py index 05f806d..0280a4f 100644 --- a/rtv/__main__.py +++ b/rtv/__main__.py @@ -26,6 +26,7 @@ except ImportError: # 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 @@ -36,7 +37,7 @@ 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__ @@ -156,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/objects.py b/rtv/objects.py index 48f0074..7113a82 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,26 @@ 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': + + # This is a copy of what's at the end of webbrowser.py, except that + # it adds MacOSXOSAScript entries instead of GenericBrowser entries. + if "BROWSER" in os.environ: + _userchoices = os.environ["BROWSER"].split(os.pathsep) + for cmdline in reversed(_userchoices): + if cmdline in ('safari', 'firefox', 'chrome', 'default'): + browser = webbrowser.MacOSXOSAScript(cmdline) + webbrowser.register(cmdline, None, browser, -1) + + @contextmanager def curses_session(): """ diff --git a/rtv/terminal.py b/rtv/terminal.py index eadec67..871dcae 100644 --- a/rtv/terminal.py +++ b/rtv/terminal.py @@ -13,6 +13,7 @@ import webbrowser import subprocess import curses.ascii from curses import textpad +from multiprocessing import Process from contextlib import contextmanager from tempfile import NamedTemporaryFile @@ -464,11 +465,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,36 +481,33 @@ 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 diff --git a/tests/test_objects.py b/tests/test_objects.py index e3d8213..31b0c30 100644 --- a/tests/test_objects.py +++ b/tests/test_objects.py @@ -5,13 +5,15 @@ 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 @@ -19,6 +21,28 @@ except ImportError: import mock +@mock.patch.dict(os.environ, {'BROWSER': 'safari'}) +@mock.patch('sys.platform', 'darwin') +@mock.patch('shutil.which', return_value=None) # py3 method +@mock.patch('os.path.isfile', return_value=None) # py2 method +def test_patch_webbrowser(which, isfile): + + # 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): terminal.config['ascii'] = use_ascii