diff --git a/rtv/exceptions.py b/rtv/exceptions.py index 34a6d4b..ab6a8c6 100644 --- a/rtv/exceptions.py +++ b/rtv/exceptions.py @@ -28,3 +28,7 @@ class SubscriptionError(RTVError): class ProgramError(RTVError): "Problem executing an external program" + + +class BrowserError(RTVError): + "Could not open a web browser tab" \ No newline at end of file diff --git a/rtv/oauth.py b/rtv/oauth.py index a0034a9..03de25e 100644 --- a/rtv/oauth.py +++ b/rtv/oauth.py @@ -97,7 +97,6 @@ class OAuthHelper(object): self.term.open_browser(authorize_url) io.start() if self.term.loader.exception: - io.clear_instance() return else: # Open the terminal webbrowser in a background thread and wait @@ -116,6 +115,9 @@ class OAuthHelper(object): elif self.params['error']: self.term.show_notification('Authentication error') return + elif self.params['state'] is None: + # Something went wrong but it's not clear what happened + return elif self.params['state'] != state: self.term.show_notification('UUID mismatch') return diff --git a/rtv/terminal.py b/rtv/terminal.py index 5b92fb0..377a9e0 100644 --- a/rtv/terminal.py +++ b/rtv/terminal.py @@ -16,8 +16,8 @@ from tempfile import NamedTemporaryFile import six from kitchen.text.display import textual_width_chop +from . import exceptions from .objects import LoadScreen, Color -from .exceptions import EscapeInterrupt, ProgramError class Terminal(object): @@ -68,7 +68,12 @@ class Terminal(object): """ if self._display is None: - display = bool(os.environ.get("DISPLAY")) + if sys.platform == 'darwin': + # OSX doesn't always set DISPLAY so we can't use this to check + display = True + else: + display = bool(os.environ.get("DISPLAY")) + # Use the convention defined here to parse $BROWSER # https://docs.python.org/2/library/webbrowser.html console_browsers = ['www-browser', 'links', 'links2', 'elinks', @@ -286,8 +291,29 @@ class Terminal(object): if self.display: command = "import webbrowser; webbrowser.open_new_tab('%s')" % url args = [sys.executable, '-c', command] - with open(os.devnull, 'ab+', 0) as null: - subprocess.check_call(args, stdout=null, stderr=null) + null = open(os.devnull, 'ab+', 0) + p = subprocess.Popen(args, stdout=null, stderr=null) + with self.loader(message='Opening page in a new window'): + # Give the browser 5 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 < 5: + code = p.poll() + if code == 0: + break # Success + elif code is not None: + raise exceptions.BrowserError( + 'Browser exited with status=%s' % code) + time.sleep(0.01) + else: + raise exceptions.BrowserError('Timeout opening browser') + 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 + p.terminate() else: with self.suspend(): webbrowser.open_new_tab(url) @@ -310,7 +336,8 @@ class Terminal(object): with self.suspend(): subprocess.Popen([editor, fp.name]).wait() except OSError: - raise ProgramError('Could not open file with %s' % editor) + raise exceptions.ProgramError( + 'Could not open file with %s' % editor) # Open a second file object to read. This appears to be necessary # in order to read the changes made by some editors (gedit). w+ @@ -343,9 +370,9 @@ class Terminal(object): def validate(ch): "Filters characters for special key sequences" if ch == self.ESCAPE: - raise EscapeInterrupt() + raise exceptions.EscapeInterrupt() if (not allow_resize) and (ch == curses.KEY_RESIZE): - raise EscapeInterrupt() + raise exceptions.EscapeInterrupt() # Fix backspace for iterm if ch == curses.ascii.DEL: ch = curses.KEY_BACKSPACE @@ -358,7 +385,7 @@ class Terminal(object): out = textbox.edit(validate=validate) if isinstance(out, six.binary_type): out = out.decode('utf-8') - except EscapeInterrupt: + except exceptions.EscapeInterrupt: out = None curses.curs_set(0) @@ -442,4 +469,4 @@ class Terminal(object): break out = '\n'.join(stack) - return out \ No newline at end of file + return out diff --git a/tests/test_terminal.py b/tests/test_terminal.py index ce4e7db..1b76e9a 100644 --- a/tests/test_terminal.py +++ b/tests/test_terminal.py @@ -28,9 +28,10 @@ def test_terminal_properties(terminal, config): assert len(terminal.guilded) == 2 assert isinstance(terminal.guilded[0], six.text_type) + # DISPLAY isn't reliable on all systems so we no longer rely on it. terminal._display = None with mock.patch.dict('os.environ', {'DISPLAY': ''}): - assert terminal.display is False + assert terminal.display is True terminal._display = None with mock.patch.dict('os.environ', {'DISPLAY': ':0', 'BROWSER': 'w3m'}): @@ -279,9 +280,10 @@ def test_open_browser(terminal): url = 'http://www.test.com' terminal._display = True - with mock.patch('subprocess.check_call', autospec=True) as check_call: + with mock.patch('subprocess.Popen', autospec=True) as Popen: + Popen.return_value.poll.return_value = 0 terminal.open_browser(url) - assert check_call.called + assert Popen.called assert not curses.endwin.called assert not curses.doupdate.called