Merge lp:~mikemc/ubuntuone-control-panel/launch-separate-darwin-menu into lp:ubuntuone-control-panel

[Mike McCracken]

- Add support for menu in separate process on darwin. (LP: #1045939)

- Add support for menu in separate process on darwin. (LP: #1045939)

- cmd-q handler in control panel no longer kills syncdaemon
- control panel waits until it gets first syncdaemon status update then launches menu to avoid IPC race in syncdaemontool.

Tests pass on darwin and linux P+Q.

To test IRL on darwin, you need lp:ubuntuone-cocoa-menu, and then you *MUST* package the entire app.
The ubuntuone-cocoa-menu code is not set up to run from source.
PyObjC works from source in general, but the Cocoa UI loading code doesn't handle that case.

[dobey (dobey)]

review: Approve

[lp:~mikemc/ubuntuone-control-panel/launch-separate-darwin-menu]

383. By Mike McCracken on 2012-11-30

add docstring for dummy class

[Diego Sarmentero (diegosarmentero)]

+1
"Let's put a smile on that face!" [The Dark Knight]

review: Approve

=== modified file 'ubuntuone/controlpanel/backend.py'
--- ubuntuone/controlpanel/backend.py	2012-11-29 20:15:08 +0000
+++ ubuntuone/controlpanel/backend.py	2012-11-30 18:08:22 +0000
@@ -226,6 +226,11 @@
             self.sd_client.set_status_changed_handler(cb)
             self.is_sd_client_handler_set = True
 
+    def remove_status_changed_handler(self, handler):
+        """Remove 'handler' from callbacks list."""
+        if handler in self._status_changed_handlers:
+            self._status_changed_handlers.remove(handler)
+
     @inlineCallbacks
     def _process_device_web_info(self, devices,
                                  enabled=None, limit_bw=None,
 
=== modified file 'ubuntuone/controlpanel/gui/qt/main/__init__.py'
--- ubuntuone/controlpanel/gui/qt/main/__init__.py	2012-10-23 19:26:30 +0000
+++ ubuntuone/controlpanel/gui/qt/main/__init__.py	2012-11-30 18:08:22 +0000
@@ -40,10 +40,14 @@
 
 if sys.platform == 'darwin':
     from ubuntuone.controlpanel.gui.qt.main.darwin import (
-        install_platform_event_handlers)
+        install_platform_event_handlers, MenubarIconLauncher)
     assert(install_platform_event_handlers)
+    assert(MenubarIconLauncher)
 else:
-    install_platform_event_handlers = lambda app: None
+    class MenubarIconLauncher:
+        """Dummy. Separate menu launcher not used on win/linux."""
+    install_platform_event_handlers = lambda app, **kwargs: None
+
 # pylint: enable=C0103
 
 from ubuntuone.controlpanel.utils import install_config_and_daemons
@@ -118,9 +122,7 @@
     args = parser.parse_args(args=arg_list[bin_position:])
     switch_to = args.switch_to
     minimized = args.minimized
-    # On Darwin, because of how apps are started, we need
-    # this by default.
-    with_icon = args.with_icon or sys.platform == 'darwin'
+    with_icon = args.with_icon
     installer = args.installer
     source.main(app)
 
@@ -135,9 +137,17 @@
 
     install_config_and_daemons()
 
+    if sys.platform == 'darwin':
+        with_icon = False
+        # cmd-q in CP shouldn't kill SD or the menu:
+        quit_kills_sd = False
+    else:
+        quit_kills_sd = not with_icon
+
     # need to keep a reference to the menu or our handler will be
     # removed
-    menubar = install_platform_event_handlers(app)
+    menubar = install_platform_event_handlers(app,
+                                              quit_kills_sd=quit_kills_sd)
     menubar
 
     # Unused variable 'window', 'icon', pylint: disable=W0612
@@ -151,4 +161,10 @@
     if icon:
         app.new_instance.connect(icon.restore_window)
 
+    if sys.platform == 'darwin':
+        # start separate menu app instead of Qt systray menu, waiting
+        # until it is safe to do so without IPC races:
+        menu_launcher = MenubarIconLauncher()
+        assert(menu_launcher)
+
     source.main_start(app)
 
=== modified file 'ubuntuone/controlpanel/gui/qt/main/darwin.py'
--- ubuntuone/controlpanel/gui/qt/main/darwin.py	2012-10-04 20:34:22 +0000
+++ ubuntuone/controlpanel/gui/qt/main/darwin.py	2012-11-30 18:08:22 +0000
@@ -16,28 +16,40 @@
 
 """Darwin-specific GUI event handling code."""
 
+import sys
+import subprocess
+
 from PyQt4 import QtGui
 from twisted.internet.defer import inlineCallbacks
 
+from dirspec.utils import get_program_path
+
 from ubuntuone.platform.tools import SyncDaemonTool
 
+from ubuntuone.controlpanel import cache
+from ubuntuone.controlpanel.backend import (STATUS_KEY, FILE_SYNC_ERROR)
 from ubuntuone.controlpanel.gui.qt.main import twisted_main
+from ubuntuone.controlpanel.logger import setup_logging
+
+
+logger = setup_logging('qt.main.darwin')
 
 
 @inlineCallbacks
-def handle_cmd_q(app, event):
+def handle_cmd_q(app, event, should_quit_sd=False):
     """Handle the quit event on darwin."""
     # pylint: disable=W0702
-    try:
-        st = SyncDaemonTool()
-        yield st.quit()
-    except:
-        pass
+    if should_quit_sd:
+        try:
+            st = SyncDaemonTool()
+            yield st.quit()
+        except:
+            pass
     twisted_main.main_quit(app)
     app.aboutToQuit.emit()
 
 
-def install_platform_event_handlers(app):
+def install_platform_event_handlers(app, quit_kills_sd=False):
     """Add code to catch cmd-Q."""
     # on darwin, a menu item with 'quit' in its title is moved to
     # the application menu and given the cmd-Q shortcut by Qt. If
@@ -52,9 +64,41 @@
 
     quit_action = QtGui.QAction("quit", menubar,
                                 triggered=lambda x:
-                                handle_cmd_q(app, x))
+                                handle_cmd_q(app,
+                                             x,
+                                             quit_kills_sd))
     quit_action.setObjectName("darwin-cmd-q")
     menu = menubar.addMenu("This string is not used.")
     menu.addAction(quit_action)
 
     return menubar
+
+
+class MenubarIconLauncher(cache.Cache):
+    """Listens to status, launches the menubar app once it is safe to do so."""
+
+    def __init__(self):
+        """Register as listener."""
+        super(MenubarIconLauncher, self).__init__()
+        self.backend.add_status_changed_handler(self.handle_status_update)
+
+    def handle_status_update(self, result):
+        """Process updates, launch menu on non-error"""
+
+        if result[STATUS_KEY] == FILE_SYNC_ERROR:
+            logger.warning("not starting menu, file sync in error state")
+            return
+
+        self.start_menu_process()
+        self.backend.remove_status_changed_handler(self.handle_status_update)
+
+    def start_menu_process(self):
+        """Find and launch separate menu process."""
+        if getattr(sys, 'frozen', None) is None:
+            logger.warning("Can not launch pyobjc menu from source, ignoring.")
+            return
+        menu_app_name = 'Ubuntu One Menu'
+        path = get_program_path(menu_app_name,
+                                app_names={menu_app_name:
+                                           'Ubuntu One Menu.app'})
+        subprocess.Popen(path)
 
=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py'
--- ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py	2012-10-04 20:34:22 +0000
+++ ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py	2012-11-30 18:08:22 +0000
@@ -18,8 +18,12 @@
 
 from PyQt4 import QtGui
 
+import subprocess
+import sys
+
 from twisted.internet.defer import (inlineCallbacks, succeed)
 
+from ubuntuone.controlpanel import backend
 from ubuntuone.controlpanel.gui.qt import main
 from ubuntuone.controlpanel.tests import TestCase
 from ubuntuone.controlpanel.gui.tests import FakeSignal
@@ -60,6 +64,28 @@
         raise Exception("exception")
 
 
+class FakeBackend(object):
+    """Mock backend"""
+
+    def __init__(self):
+        """init"""
+
+        super(FakeBackend, self).__init__()
+        self.handlers = []
+
+    def add_status_changed_handler(self, h):
+        """add handler"""
+        self.handlers.append(h)
+
+    def remove_status_changed_handler(self, h):
+        """remove handler """
+        self.handlers.remove(h)
+
+    def clear(self):
+        """reset handlers"""
+        self.handlers = []
+
+
 class QuitTestCase(TestCase):
     """Test quit event handling."""
 
@@ -73,13 +99,22 @@
 
     @inlineCallbacks
     def test_cmd_q_func_quits_sd(self):
-        """Test that we call syncdaemontool.quit """
+        """Test that we call syncdaemontool.quit when asked"""
         self.patch(main.darwin, 'SyncDaemonTool', FakeSDTool)
 
-        yield main.darwin.handle_cmd_q(self.fake_app, None)
+        yield main.darwin.handle_cmd_q(self.fake_app, None,
+                                       should_quit_sd=True)
         self.assertEqual(FakeSDTool.called, True)
 
     @inlineCallbacks
+    def test_cmd_q_func_doesnt_quit_sd(self):
+        """Test that we don't call syncdaemontool.quit by default"""
+        self.patch(main.darwin, 'SyncDaemonTool', FakeSDTool)
+
+        yield main.darwin.handle_cmd_q(self.fake_app, None)
+        self.assertEqual(FakeSDTool.called, False)
+
+    @inlineCallbacks
     def test_cmd_q_func_ignores_exception(self):
         """Test that we keep going when SDT raises."""
         self.patch(main.darwin, 'SyncDaemonTool', FakeSDToolRaising)
@@ -93,12 +128,76 @@
 class InstallEventHandlersTestCase(TestCase):
     """Test creating Qt menu items."""
 
-    def test_install_handlers_creates_action(self):
-        """Test menu items created"""
+    def test_install_handlers_creates_kill_action(self):
+        """Test creating menu item that will kill sd"""
+        self.patch(main.darwin, 'handle_cmd_q',
+                   self._set_called)
+        app = QtGui.QApplication.instance()
+        k = dict(quit_kills_sd=True)
+        menubar = main.darwin.install_platform_event_handlers(app, **k)
+        quit_action = menubar.findChild(QtGui.QAction, "darwin-cmd-q")
+        quit_action.trigger()
+        self.assertEqual(self._called, ((app, False, True), {}))
+
+    def test_install_handlers_creates_nonkill_action(self):
+        """Test creating menu item that will not kill sd"""
         self.patch(main.darwin, 'handle_cmd_q',
                    self._set_called)
         app = QtGui.QApplication.instance()
         menubar = main.darwin.install_platform_event_handlers(app)
         quit_action = menubar.findChild(QtGui.QAction, "darwin-cmd-q")
         quit_action.trigger()
-        self.assertEqual(self._called, ((app, False), {}))
+        self.assertEqual(self._called, ((app, False, False), {}))
+
+
+class LauncherTestCase(TestCase):
+    """Test pyobjc menu launcher."""
+
+    @inlineCallbacks
+    def setUp(self):
+        """Set up launcher and patches"""
+        yield super(LauncherTestCase, self).setUp()
+
+        self.patch(backend, 'ControlBackend', FakeBackend)
+        self.patch(main.darwin, 'get_program_path',
+                   lambda a, **kw: 'testpath')
+        self.addCleanup(lambda: self.launcher.backend.clear())
+
+        self.launcher = main.darwin.MenubarIconLauncher()
+
+    def test_add_on_init(self):
+        """Test adding to handler."""
+        self.assertEqual(self.launcher.backend.handlers,
+                         [self.launcher.handle_status_update])
+
+    def test_handle_status_update_error(self):
+        """Test doing nothing on error in handle_status_update"""
+        self.patch(self.launcher, 'start_menu_process', self._set_called)
+        self.launcher.handle_status_update({backend.STATUS_KEY:
+                                            backend.FILE_SYNC_ERROR})
+        self.assertEqual(self._called, False)
+
+    def test_handle_status_update(self):
+        """Test calling start_menu_process and removing handler"""
+        self.patch(self.launcher, 'start_menu_process', self._set_called)
+        self.launcher.handle_status_update({backend.STATUS_KEY: 'copacetic'})
+        self.assertEqual(self._called, ((), {}))
+        self.assertEqual(self.launcher.backend.handlers, [])
+
+    def test_start_menu_process_nonfrozen(self):
+        """Start does nothing when not frozen."""
+        sys.frozen = None
+        self.addCleanup(delattr, sys, 'frozen')
+
+        self.patch(subprocess, 'Popen', self._set_called)
+        self.launcher.start_menu_process()
+        self.assertEqual(self._called, False)
+
+    def test_start_menu_process_frozen(self):
+        """Start launches menu when frozen."""
+        sys.frozen = 'macosx'
+        self.addCleanup(delattr, sys, 'frozen')
+
+        self.patch(subprocess, 'Popen', self._set_called)
+        self.launcher.start_menu_process()
+        self.assertEqual(self._called, (('testpath',), {}))
 
=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_main.py'
--- ubuntuone/controlpanel/gui/qt/main/tests/test_main.py	2012-10-31 19:32:18 +0000
+++ ubuntuone/controlpanel/gui/qt/main/tests/test_main.py	2012-11-30 18:08:22 +0000
@@ -137,7 +137,7 @@
         self.patch(main.source, "main_start", lambda app: None)
         self.patch(QtCore, "QTranslator", lambda: self.translator)
         self.patch(main, 'install_platform_event_handlers',
-                   lambda app: None)
+                   lambda app, **kwargs: None)
 
         self.qt4reactor_installed = False
 
@@ -194,11 +194,11 @@
             {'minimized': True, 'with_icon': False, 'installer': False})
 
     def test_darwin_defult_is_with_icon(self):
-        """On Darwin, the default is to show the icon."""
+        """On Darwin, the default is not to show the icon."""
         self.patch(sys, 'platform', 'darwin')
         main.main([sys.argv[0]])
         self.assertEqual(self.start.args[1],
-            {'minimized': False, 'with_icon': True, 'installer': False})
+            {'minimized': False, 'with_icon': False, 'installer': False})
 
     def test_not_darwin_defult_is_with_icon(self):
         """On Not-darwin, the default is to not show the icon."""
@@ -215,7 +215,7 @@
             {'minimized': True, 'with_icon': False, 'installer': False})
 
     def test_with_icon_option(self):
-        """Ensure the --minimized option is parsed and passed correctly."""
+        """Ensure the --with-icon option is parsed and passed correctly."""
         self.patch(sys, 'platform', 'not-darwin')
         main.main([sys.argv[0], "--with-icon"])
         self.assertEqual(self.start.args[1],
@@ -308,10 +308,20 @@
         main.main([sys.argv[0]], install_reactor_darwin=False)
         self.assertEqual(self._called, ((), {}))
 
-    def test_install_event_handlers(self):
-        """Test that install_platform_event_handlers is called."""
+    def test_install_event_handlers_darwin(self):
+        """Test that install_platform_event_handlers is called on darwin."""
+        self.patch(sys, 'platform', 'darwin')
         self.patch(main, 'install_platform_event_handlers',
                    self._set_called)
 
         main.main([sys.argv[0]], install_reactor_darwin=False)
-        self.assertEqual(self._called, ((self.app,), {}))
+        self.assertEqual(self._called, ((self.app,), {'quit_kills_sd': False}))
+
+    def test_install_event_handlers_non_darwin(self):
+        """Test install_platform_event_handlers is called on not-darwin."""
+        self.patch(sys, 'platform', 'not-darwin')
+        self.patch(main, 'install_platform_event_handlers',
+                   self._set_called)
+
+        main.main([sys.argv[0], '--with-icon'], install_reactor_darwin=False)
+        self.assertEqual(self._called, ((self.app,), {'quit_kills_sd': False}))
 
=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
--- ubuntuone/controlpanel/tests/test_backend.py	2012-11-29 20:15:08 +0000
+++ ubuntuone/controlpanel/tests/test_backend.py	2012-11-30 18:08:22 +0000
@@ -1485,6 +1485,32 @@
         self.assertEqual(self.call_count_b, 1)
         self.assertEqual(self.call_count_a, 1)
 
+    def test_remove_status_handler(self):
+        """Test removing a handler."""
+        self.call_count_a = 0
+        self.call_count_b = 0
+
+        def inc_a(status):
+            """Fake status handler #1"""
+            self.call_count_a += 1
+
+        def inc_b(status):
+            """Fake status handler #2"""
+            self.call_count_b += 1
+
+        self.addCleanup(delattr, self, "call_count_a")
+        self.addCleanup(delattr, self, "call_count_b")
+
+        self.be.add_status_changed_handler(inc_a)
+        self.be.add_status_changed_handler(inc_b)
+
+        self.be.sd_client.status_changed_handler({})
+        self.be.remove_status_changed_handler(inc_b)
+        self.be.sd_client.status_changed_handler({})
+
+        self.assertEqual(self.call_count_b, 1)
+        self.assertEqual(self.call_count_a, 2)
+
 
 class BackendSyncStatusIfDisabledTestCase(BackendSyncStatusTestCase):
     """Syncdaemon state for the backend when file sync is disabled."""