From 0703a578bacac322b6bcc73cc05af84c4a9283e8 Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 30 Aug 2017 01:00:28 -0400 Subject: [PATCH 1/9] Trying to update $BROWSER --- rtv/__main__.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/rtv/__main__.py b/rtv/__main__.py index 994db2c..05f806d 100644 --- a/rtv/__main__.py +++ b/rtv/__main__.py @@ -23,6 +23,13 @@ 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. +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 @@ -110,8 +117,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 +146,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, ' From d7bb75d065318aa41e6862526d5cd4b851646eaa Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Mon, 4 Sep 2017 23:07:23 -0400 Subject: [PATCH 2/9] Adding helper script --- scripts/inspect_webbrowser.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100755 scripts/inspect_webbrowser.py diff --git a/scripts/inspect_webbrowser.py b/scripts/inspect_webbrowser.py new file mode 100755 index 0000000..e9b7493 --- /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' % 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') From 5499aadffbb1f89a897f552f4d5c3b5f46c5033d Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 00:22:05 -0400 Subject: [PATCH 3/9] 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 From f514edf06f7f42407aba0076922408fdd4c0d222 Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 00:24:09 -0400 Subject: [PATCH 4/9] Typo in script --- scripts/inspect_webbrowser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/inspect_webbrowser.py b/scripts/inspect_webbrowser.py index e9b7493..35ef49a 100755 --- a/scripts/inspect_webbrowser.py +++ b/scripts/inspect_webbrowser.py @@ -15,7 +15,7 @@ RTV_BROWSER, BROWSER = os.environ.get('RTV_BROWSER'), os.environ.get('BROWSER') if RTV_BROWSER: os.environ['BROWSER'] = RTV_BROWSER -print('RTV_BROWSER=%s' % BROWSER) +print('RTV_BROWSER=%s' % RTV_BROWSER) print('BROWSER=%s' % BROWSER) import webbrowser From 5a981769923362a9175cd5333359bdf23e777071 Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 00:59:20 -0400 Subject: [PATCH 5/9] Fix open_browser tests --- rtv/terminal.py | 9 ++++----- tests/test_terminal.py | 36 ++++++++++++++++++++++++++---------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/rtv/terminal.py b/rtv/terminal.py index 871dcae..87829b9 100644 --- a/rtv/terminal.py +++ b/rtv/terminal.py @@ -18,8 +18,6 @@ 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 @@ -509,9 +507,10 @@ class Terminal(object): raise exceptions.BrowserError( '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: 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 From 77a3557eddd667c1d9221a261a6c0c15365a506d Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 01:16:42 -0400 Subject: [PATCH 6/9] Trying to fix the tests --- rtv/objects.py | 6 +++++- tests/test_objects.py | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/rtv/objects.py b/rtv/objects.py index 7113a82..64116df 100644 --- a/rtv/objects.py +++ b/rtv/objects.py @@ -42,7 +42,11 @@ def patch_webbrowser(): for cmdline in reversed(_userchoices): if cmdline in ('safari', 'firefox', 'chrome', 'default'): browser = webbrowser.MacOSXOSAScript(cmdline) - webbrowser.register(cmdline, None, browser, -1) + try: + webbrowser.register(cmdline, None, browser, update_tryorder=-1) + except AttributeError: + # 3.7 nightly build changed the method signature + webbrowser.register(cmdline, None, browser, preferred=True) @contextmanager diff --git a/tests/test_objects.py b/tests/test_objects.py index 31b0c30..3d797a8 100644 --- a/tests/test_objects.py +++ b/tests/test_objects.py @@ -20,11 +20,16 @@ try: 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.patch('shutil.which', return_value=None) # py3 method -@mock.patch('os.path.isfile', return_value=None) # py2 method +@mock_isfile def test_patch_webbrowser(which, isfile): # Make sure that webbrowser re-generates the browser list using the From 9631521805d998df4d588825cd7a27a8621f18f8 Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 01:21:23 -0400 Subject: [PATCH 7/9] Trying harder --- tests/test_objects.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_objects.py b/tests/test_objects.py index 3d797a8..411fd7c 100644 --- a/tests/test_objects.py +++ b/tests/test_objects.py @@ -30,7 +30,7 @@ else: @mock.patch.dict(os.environ, {'BROWSER': 'safari'}) @mock.patch('sys.platform', 'darwin') @mock_isfile -def test_patch_webbrowser(which, isfile): +def test_patch_webbrowser(*_): # Make sure that webbrowser re-generates the browser list using the # mocked environment From b73e902d4aaccb1894b87e68b838963c8af14a6b Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 01:27:40 -0400 Subject: [PATCH 8/9] Adding pylint exception --- rtv/objects.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/rtv/objects.py b/rtv/objects.py index 64116df..cc5aa8f 100644 --- a/rtv/objects.py +++ b/rtv/objects.py @@ -33,20 +33,21 @@ def patch_webbrowser(): https://bugs.python.org/issue31348 """ - if sys.platform == 'darwin': + 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. - 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) - try: - webbrowser.register(cmdline, None, browser, update_tryorder=-1) - except AttributeError: - # 3.7 nightly build changed the method signature - webbrowser.register(cmdline, None, browser, preferred=True) + # 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 AttributeError: + # 3.7 nightly build changed the method signature + # pylint: disable=unexpected-keyword-arg + webbrowser.register(cmdline, None, browser, preferred=True) @contextmanager From b1b1a648549d94e8120f2f56bcfbd0d9b16d8dc3 Mon Sep 17 00:00:00 2001 From: Michael Lazar Date: Wed, 6 Sep 2017 01:36:24 -0400 Subject: [PATCH 9/9] Pylint and fixing error type --- rtv/mime_parsers.py | 10 ++++++---- rtv/objects.py | 4 ++-- rtv/terminal.py | 6 +++--- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/rtv/mime_parsers.py b/rtv/mime_parsers.py index b039249..502ee8a 100644 --- a/rtv/mime_parsers.py +++ b/rtv/mime_parsers.py @@ -151,20 +151,22 @@ class RedditVideoMIMEParser(BaseMIMEParser): class ImgurApiMIMEParser(BaseMIMEParser): - """ + """ Imgur now provides a json API exposing its entire infrastructure. Each Imgur page has an associated hash and can either contain an album, a gallery, or single image. - + The default client token for RTV is shared among users and allows a maximum global number of requests per day of 12,500. If we find that this limit is not sufficient for all of rtv's traffic, this method will be revisited. - + Reference: 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 cc5aa8f..d58b37d 100644 --- a/rtv/objects.py +++ b/rtv/objects.py @@ -29,7 +29,7 @@ def patch_webbrowser(): """ Patch webbrowser on macOS to support setting BROWSER=firefox, BROWSER=chrome, etc.. - + https://bugs.python.org/issue31348 """ @@ -44,7 +44,7 @@ def patch_webbrowser(): browser = webbrowser.MacOSXOSAScript(cmdline) try: webbrowser.register(cmdline, None, browser, update_tryorder=-1) - except AttributeError: + except TypeError: # 3.7 nightly build changed the method signature # pylint: disable=unexpected-keyword-arg webbrowser.register(cmdline, None, browser, preferred=True) diff --git a/rtv/terminal.py b/rtv/terminal.py index 87829b9..0ebfa08 100644 --- a/rtv/terminal.py +++ b/rtv/terminal.py @@ -775,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 @@ -785,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