Merge branch 'browser_osx'

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"

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

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)

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