Merge pull request #428 from michael-lazar/rtv_browser

Rtv browser

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')),
+            ('$RTV_BROWSER', RTV_BROWSER),
-            ('$BROWSER', os.getenv('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__)
 

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/((?P<domain>a|album|gallery)/)?(?P<hash>[a-zA-Z0-9]+)$')
+    pattern = re.compile(
+        r'https?://(w+\.)?(m\.)?imgur\.com/'
+        r'((?P<domain>a|album|gallery)/)?(?P<hash>[a-zA-Z0-9]+)$')
 
     @classmethod
     def get_mimetype(cls, url):

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():
     """

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
+        For browsers requiring an X display, we open a new subprocess and
-        webbrowser.open_new_tab(url) and redirect stdout/stderr to devnull.
+        redirect stdout/stderr to devnull. This is a workaround to stop
-        This is a workaround to stop firefox from spewing warning messages to
+        BackgroundBrowsers (e.g. xdg-open, any BROWSER command ending in "&"),
-        the console. See http://bugs.python.org/issue22277 for a better
+        from spewing warning messages to the console. See
-        description of the problem.
+        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
+            with self.loader('Opening page in a new window'):
-            # the python code to prevent injection attacks.
+
-            command = (
+                def open_url_silent(url):
-                "import webbrowser\n"
+                    # This used to be done using subprocess.Popen().
-                "from six.moves.urllib.parse import unquote\n"
+                    # It was switched to multiprocessing.Process so that we
-                "webbrowser.open_new_tab(unquote('%s'))" % quote(url))
+                    # can re-use the webbrowser instance that has been patched
-            args = [sys.executable, '-c', command]
+                    # by RTV. It's also safer because it doesn't inject
-            with self.loader('Opening page in a new window'), \
+                    # python code through the command line.
-                    open(os.devnull, 'ab+', 0) as null:
+                    null = open(os.devnull, 'ab+', 0)
-                p = subprocess.Popen(args, stdout=null, stderr=null)
+                    sys.stdout, sys.stderr = null, null
-                # Give the browser 5 seconds to open a new tab. Because the
+                    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()
+                    p.join(7)
-                    while time.time() - start < 10:
+                    if p.is_alive():
-                        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:
                         raise exceptions.BrowserError(
-                            'Timeout opening browser')
+                            'Timeout waiting for browser to open')
                 finally:
-                    # Can't check the loader exception because the oauth module
+                    # This will be hit on the browser timeout, but also if the
-                    # supersedes this loader and we need to always kill the
+                    # user presses the ESC key. We always want to kill the
-                    # process if escape is pressed
+                    # 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

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')

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):

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):
+def test_open_browser_display(terminal):
-
-    url = 'http://www.test.com'
 
     terminal._display = True
-    with mock.patch('subprocess.Popen', autospec=True) as Popen:
+    with mock.patch('webbrowser.open_new_tab', autospec=True) as open_new_tab:
-        Popen.return_value.poll.return_value = 0
+        terminal.open_browser('http://www.test.com')
-        terminal.open_browser(url)
+
-    assert Popen.called
+    # 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)
+        terminal.open_browser('http://www.test.com')
-    open_new_tab.assert_called_with(url)
+    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