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

Proposed by Mike McCracken
Status: Merged
Approved by: Mike McCracken
Approved revision: 383
Merged at revision: 385
Proposed branch: lp:~mikemc/ubuntuone-control-panel/launch-separate-darwin-menu
Merge into: lp:ubuntuone-control-panel
Diff against target: 420 lines (+226/-26)
6 files modified
ubuntuone/controlpanel/backend.py (+5/-0)
ubuntuone/controlpanel/gui/qt/main/__init__.py (+22/-6)
ubuntuone/controlpanel/gui/qt/main/darwin.py (+52/-8)
ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py (+104/-5)
ubuntuone/controlpanel/gui/qt/main/tests/test_main.py (+17/-7)
ubuntuone/controlpanel/tests/test_backend.py (+26/-0)
To merge this branch: bzr merge lp:~mikemc/ubuntuone-control-panel/launch-separate-darwin-menu
Reviewer Review Type Date Requested Status
Diego Sarmentero (community) Approve
dobey (community) Approve
Review via email: mp+135268@code.launchpad.net

Commit message

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

Description of the change

- 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.

To post a comment you must log in.
dobey (dobey) :
review: Approve
383. By Mike McCracken on 2012-11-30

add docstring for dummy class

Diego Sarmentero (diegosarmentero) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/backend.py'
2--- ubuntuone/controlpanel/backend.py 2012-11-29 20:15:08 +0000
3+++ ubuntuone/controlpanel/backend.py 2012-11-30 18:08:22 +0000
4@@ -226,6 +226,11 @@
5 self.sd_client.set_status_changed_handler(cb)
6 self.is_sd_client_handler_set = True
7
8+ def remove_status_changed_handler(self, handler):
9+ """Remove 'handler' from callbacks list."""
10+ if handler in self._status_changed_handlers:
11+ self._status_changed_handlers.remove(handler)
12+
13 @inlineCallbacks
14 def _process_device_web_info(self, devices,
15 enabled=None, limit_bw=None,
16
17=== modified file 'ubuntuone/controlpanel/gui/qt/main/__init__.py'
18--- ubuntuone/controlpanel/gui/qt/main/__init__.py 2012-10-23 19:26:30 +0000
19+++ ubuntuone/controlpanel/gui/qt/main/__init__.py 2012-11-30 18:08:22 +0000
20@@ -40,10 +40,14 @@
21
22 if sys.platform == 'darwin':
23 from ubuntuone.controlpanel.gui.qt.main.darwin import (
24- install_platform_event_handlers)
25+ install_platform_event_handlers, MenubarIconLauncher)
26 assert(install_platform_event_handlers)
27+ assert(MenubarIconLauncher)
28 else:
29- install_platform_event_handlers = lambda app: None
30+ class MenubarIconLauncher:
31+ """Dummy. Separate menu launcher not used on win/linux."""
32+ install_platform_event_handlers = lambda app, **kwargs: None
33+
34 # pylint: enable=C0103
35
36 from ubuntuone.controlpanel.utils import install_config_and_daemons
37@@ -118,9 +122,7 @@
38 args = parser.parse_args(args=arg_list[bin_position:])
39 switch_to = args.switch_to
40 minimized = args.minimized
41- # On Darwin, because of how apps are started, we need
42- # this by default.
43- with_icon = args.with_icon or sys.platform == 'darwin'
44+ with_icon = args.with_icon
45 installer = args.installer
46 source.main(app)
47
48@@ -135,9 +137,17 @@
49
50 install_config_and_daemons()
51
52+ if sys.platform == 'darwin':
53+ with_icon = False
54+ # cmd-q in CP shouldn't kill SD or the menu:
55+ quit_kills_sd = False
56+ else:
57+ quit_kills_sd = not with_icon
58+
59 # need to keep a reference to the menu or our handler will be
60 # removed
61- menubar = install_platform_event_handlers(app)
62+ menubar = install_platform_event_handlers(app,
63+ quit_kills_sd=quit_kills_sd)
64 menubar
65
66 # Unused variable 'window', 'icon', pylint: disable=W0612
67@@ -151,4 +161,10 @@
68 if icon:
69 app.new_instance.connect(icon.restore_window)
70
71+ if sys.platform == 'darwin':
72+ # start separate menu app instead of Qt systray menu, waiting
73+ # until it is safe to do so without IPC races:
74+ menu_launcher = MenubarIconLauncher()
75+ assert(menu_launcher)
76+
77 source.main_start(app)
78
79=== modified file 'ubuntuone/controlpanel/gui/qt/main/darwin.py'
80--- ubuntuone/controlpanel/gui/qt/main/darwin.py 2012-10-04 20:34:22 +0000
81+++ ubuntuone/controlpanel/gui/qt/main/darwin.py 2012-11-30 18:08:22 +0000
82@@ -16,28 +16,40 @@
83
84 """Darwin-specific GUI event handling code."""
85
86+import sys
87+import subprocess
88+
89 from PyQt4 import QtGui
90 from twisted.internet.defer import inlineCallbacks
91
92+from dirspec.utils import get_program_path
93+
94 from ubuntuone.platform.tools import SyncDaemonTool
95
96+from ubuntuone.controlpanel import cache
97+from ubuntuone.controlpanel.backend import (STATUS_KEY, FILE_SYNC_ERROR)
98 from ubuntuone.controlpanel.gui.qt.main import twisted_main
99+from ubuntuone.controlpanel.logger import setup_logging
100+
101+
102+logger = setup_logging('qt.main.darwin')
103
104
105 @inlineCallbacks
106-def handle_cmd_q(app, event):
107+def handle_cmd_q(app, event, should_quit_sd=False):
108 """Handle the quit event on darwin."""
109 # pylint: disable=W0702
110- try:
111- st = SyncDaemonTool()
112- yield st.quit()
113- except:
114- pass
115+ if should_quit_sd:
116+ try:
117+ st = SyncDaemonTool()
118+ yield st.quit()
119+ except:
120+ pass
121 twisted_main.main_quit(app)
122 app.aboutToQuit.emit()
123
124
125-def install_platform_event_handlers(app):
126+def install_platform_event_handlers(app, quit_kills_sd=False):
127 """Add code to catch cmd-Q."""
128 # on darwin, a menu item with 'quit' in its title is moved to
129 # the application menu and given the cmd-Q shortcut by Qt. If
130@@ -52,9 +64,41 @@
131
132 quit_action = QtGui.QAction("quit", menubar,
133 triggered=lambda x:
134- handle_cmd_q(app, x))
135+ handle_cmd_q(app,
136+ x,
137+ quit_kills_sd))
138 quit_action.setObjectName("darwin-cmd-q")
139 menu = menubar.addMenu("This string is not used.")
140 menu.addAction(quit_action)
141
142 return menubar
143+
144+
145+class MenubarIconLauncher(cache.Cache):
146+ """Listens to status, launches the menubar app once it is safe to do so."""
147+
148+ def __init__(self):
149+ """Register as listener."""
150+ super(MenubarIconLauncher, self).__init__()
151+ self.backend.add_status_changed_handler(self.handle_status_update)
152+
153+ def handle_status_update(self, result):
154+ """Process updates, launch menu on non-error"""
155+
156+ if result[STATUS_KEY] == FILE_SYNC_ERROR:
157+ logger.warning("not starting menu, file sync in error state")
158+ return
159+
160+ self.start_menu_process()
161+ self.backend.remove_status_changed_handler(self.handle_status_update)
162+
163+ def start_menu_process(self):
164+ """Find and launch separate menu process."""
165+ if getattr(sys, 'frozen', None) is None:
166+ logger.warning("Can not launch pyobjc menu from source, ignoring.")
167+ return
168+ menu_app_name = 'Ubuntu One Menu'
169+ path = get_program_path(menu_app_name,
170+ app_names={menu_app_name:
171+ 'Ubuntu One Menu.app'})
172+ subprocess.Popen(path)
173
174=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py'
175--- ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py 2012-10-04 20:34:22 +0000
176+++ ubuntuone/controlpanel/gui/qt/main/tests/test_darwin.py 2012-11-30 18:08:22 +0000
177@@ -18,8 +18,12 @@
178
179 from PyQt4 import QtGui
180
181+import subprocess
182+import sys
183+
184 from twisted.internet.defer import (inlineCallbacks, succeed)
185
186+from ubuntuone.controlpanel import backend
187 from ubuntuone.controlpanel.gui.qt import main
188 from ubuntuone.controlpanel.tests import TestCase
189 from ubuntuone.controlpanel.gui.tests import FakeSignal
190@@ -60,6 +64,28 @@
191 raise Exception("exception")
192
193
194+class FakeBackend(object):
195+ """Mock backend"""
196+
197+ def __init__(self):
198+ """init"""
199+
200+ super(FakeBackend, self).__init__()
201+ self.handlers = []
202+
203+ def add_status_changed_handler(self, h):
204+ """add handler"""
205+ self.handlers.append(h)
206+
207+ def remove_status_changed_handler(self, h):
208+ """remove handler """
209+ self.handlers.remove(h)
210+
211+ def clear(self):
212+ """reset handlers"""
213+ self.handlers = []
214+
215+
216 class QuitTestCase(TestCase):
217 """Test quit event handling."""
218
219@@ -73,13 +99,22 @@
220
221 @inlineCallbacks
222 def test_cmd_q_func_quits_sd(self):
223- """Test that we call syncdaemontool.quit """
224+ """Test that we call syncdaemontool.quit when asked"""
225 self.patch(main.darwin, 'SyncDaemonTool', FakeSDTool)
226
227- yield main.darwin.handle_cmd_q(self.fake_app, None)
228+ yield main.darwin.handle_cmd_q(self.fake_app, None,
229+ should_quit_sd=True)
230 self.assertEqual(FakeSDTool.called, True)
231
232 @inlineCallbacks
233+ def test_cmd_q_func_doesnt_quit_sd(self):
234+ """Test that we don't call syncdaemontool.quit by default"""
235+ self.patch(main.darwin, 'SyncDaemonTool', FakeSDTool)
236+
237+ yield main.darwin.handle_cmd_q(self.fake_app, None)
238+ self.assertEqual(FakeSDTool.called, False)
239+
240+ @inlineCallbacks
241 def test_cmd_q_func_ignores_exception(self):
242 """Test that we keep going when SDT raises."""
243 self.patch(main.darwin, 'SyncDaemonTool', FakeSDToolRaising)
244@@ -93,12 +128,76 @@
245 class InstallEventHandlersTestCase(TestCase):
246 """Test creating Qt menu items."""
247
248- def test_install_handlers_creates_action(self):
249- """Test menu items created"""
250+ def test_install_handlers_creates_kill_action(self):
251+ """Test creating menu item that will kill sd"""
252+ self.patch(main.darwin, 'handle_cmd_q',
253+ self._set_called)
254+ app = QtGui.QApplication.instance()
255+ k = dict(quit_kills_sd=True)
256+ menubar = main.darwin.install_platform_event_handlers(app, **k)
257+ quit_action = menubar.findChild(QtGui.QAction, "darwin-cmd-q")
258+ quit_action.trigger()
259+ self.assertEqual(self._called, ((app, False, True), {}))
260+
261+ def test_install_handlers_creates_nonkill_action(self):
262+ """Test creating menu item that will not kill sd"""
263 self.patch(main.darwin, 'handle_cmd_q',
264 self._set_called)
265 app = QtGui.QApplication.instance()
266 menubar = main.darwin.install_platform_event_handlers(app)
267 quit_action = menubar.findChild(QtGui.QAction, "darwin-cmd-q")
268 quit_action.trigger()
269- self.assertEqual(self._called, ((app, False), {}))
270+ self.assertEqual(self._called, ((app, False, False), {}))
271+
272+
273+class LauncherTestCase(TestCase):
274+ """Test pyobjc menu launcher."""
275+
276+ @inlineCallbacks
277+ def setUp(self):
278+ """Set up launcher and patches"""
279+ yield super(LauncherTestCase, self).setUp()
280+
281+ self.patch(backend, 'ControlBackend', FakeBackend)
282+ self.patch(main.darwin, 'get_program_path',
283+ lambda a, **kw: 'testpath')
284+ self.addCleanup(lambda: self.launcher.backend.clear())
285+
286+ self.launcher = main.darwin.MenubarIconLauncher()
287+
288+ def test_add_on_init(self):
289+ """Test adding to handler."""
290+ self.assertEqual(self.launcher.backend.handlers,
291+ [self.launcher.handle_status_update])
292+
293+ def test_handle_status_update_error(self):
294+ """Test doing nothing on error in handle_status_update"""
295+ self.patch(self.launcher, 'start_menu_process', self._set_called)
296+ self.launcher.handle_status_update({backend.STATUS_KEY:
297+ backend.FILE_SYNC_ERROR})
298+ self.assertEqual(self._called, False)
299+
300+ def test_handle_status_update(self):
301+ """Test calling start_menu_process and removing handler"""
302+ self.patch(self.launcher, 'start_menu_process', self._set_called)
303+ self.launcher.handle_status_update({backend.STATUS_KEY: 'copacetic'})
304+ self.assertEqual(self._called, ((), {}))
305+ self.assertEqual(self.launcher.backend.handlers, [])
306+
307+ def test_start_menu_process_nonfrozen(self):
308+ """Start does nothing when not frozen."""
309+ sys.frozen = None
310+ self.addCleanup(delattr, sys, 'frozen')
311+
312+ self.patch(subprocess, 'Popen', self._set_called)
313+ self.launcher.start_menu_process()
314+ self.assertEqual(self._called, False)
315+
316+ def test_start_menu_process_frozen(self):
317+ """Start launches menu when frozen."""
318+ sys.frozen = 'macosx'
319+ self.addCleanup(delattr, sys, 'frozen')
320+
321+ self.patch(subprocess, 'Popen', self._set_called)
322+ self.launcher.start_menu_process()
323+ self.assertEqual(self._called, (('testpath',), {}))
324
325=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_main.py'
326--- ubuntuone/controlpanel/gui/qt/main/tests/test_main.py 2012-10-31 19:32:18 +0000
327+++ ubuntuone/controlpanel/gui/qt/main/tests/test_main.py 2012-11-30 18:08:22 +0000
328@@ -137,7 +137,7 @@
329 self.patch(main.source, "main_start", lambda app: None)
330 self.patch(QtCore, "QTranslator", lambda: self.translator)
331 self.patch(main, 'install_platform_event_handlers',
332- lambda app: None)
333+ lambda app, **kwargs: None)
334
335 self.qt4reactor_installed = False
336
337@@ -194,11 +194,11 @@
338 {'minimized': True, 'with_icon': False, 'installer': False})
339
340 def test_darwin_defult_is_with_icon(self):
341- """On Darwin, the default is to show the icon."""
342+ """On Darwin, the default is not to show the icon."""
343 self.patch(sys, 'platform', 'darwin')
344 main.main([sys.argv[0]])
345 self.assertEqual(self.start.args[1],
346- {'minimized': False, 'with_icon': True, 'installer': False})
347+ {'minimized': False, 'with_icon': False, 'installer': False})
348
349 def test_not_darwin_defult_is_with_icon(self):
350 """On Not-darwin, the default is to not show the icon."""
351@@ -215,7 +215,7 @@
352 {'minimized': True, 'with_icon': False, 'installer': False})
353
354 def test_with_icon_option(self):
355- """Ensure the --minimized option is parsed and passed correctly."""
356+ """Ensure the --with-icon option is parsed and passed correctly."""
357 self.patch(sys, 'platform', 'not-darwin')
358 main.main([sys.argv[0], "--with-icon"])
359 self.assertEqual(self.start.args[1],
360@@ -308,10 +308,20 @@
361 main.main([sys.argv[0]], install_reactor_darwin=False)
362 self.assertEqual(self._called, ((), {}))
363
364- def test_install_event_handlers(self):
365- """Test that install_platform_event_handlers is called."""
366+ def test_install_event_handlers_darwin(self):
367+ """Test that install_platform_event_handlers is called on darwin."""
368+ self.patch(sys, 'platform', 'darwin')
369 self.patch(main, 'install_platform_event_handlers',
370 self._set_called)
371
372 main.main([sys.argv[0]], install_reactor_darwin=False)
373- self.assertEqual(self._called, ((self.app,), {}))
374+ self.assertEqual(self._called, ((self.app,), {'quit_kills_sd': False}))
375+
376+ def test_install_event_handlers_non_darwin(self):
377+ """Test install_platform_event_handlers is called on not-darwin."""
378+ self.patch(sys, 'platform', 'not-darwin')
379+ self.patch(main, 'install_platform_event_handlers',
380+ self._set_called)
381+
382+ main.main([sys.argv[0], '--with-icon'], install_reactor_darwin=False)
383+ self.assertEqual(self._called, ((self.app,), {'quit_kills_sd': False}))
384
385=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
386--- ubuntuone/controlpanel/tests/test_backend.py 2012-11-29 20:15:08 +0000
387+++ ubuntuone/controlpanel/tests/test_backend.py 2012-11-30 18:08:22 +0000
388@@ -1485,6 +1485,32 @@
389 self.assertEqual(self.call_count_b, 1)
390 self.assertEqual(self.call_count_a, 1)
391
392+ def test_remove_status_handler(self):
393+ """Test removing a handler."""
394+ self.call_count_a = 0
395+ self.call_count_b = 0
396+
397+ def inc_a(status):
398+ """Fake status handler #1"""
399+ self.call_count_a += 1
400+
401+ def inc_b(status):
402+ """Fake status handler #2"""
403+ self.call_count_b += 1
404+
405+ self.addCleanup(delattr, self, "call_count_a")
406+ self.addCleanup(delattr, self, "call_count_b")
407+
408+ self.be.add_status_changed_handler(inc_a)
409+ self.be.add_status_changed_handler(inc_b)
410+
411+ self.be.sd_client.status_changed_handler({})
412+ self.be.remove_status_changed_handler(inc_b)
413+ self.be.sd_client.status_changed_handler({})
414+
415+ self.assertEqual(self.call_count_b, 1)
416+ self.assertEqual(self.call_count_a, 2)
417+
418
419 class BackendSyncStatusIfDisabledTestCase(BackendSyncStatusTestCase):
420 """Syncdaemon state for the backend when file sync is disabled."""

Subscribers

People subscribed via source and target branches