Merge lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch into lp:ubuntuone-client

Proposed by Marco Trevisan (Treviño) on 2013-04-11
Status: Merged
Approved by: dobey on 2013-04-16
Approved revision: 1399
Merged at revision: 1393
Proposed branch: lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch
Merge into: lp:ubuntuone-client
Diff against target: 352 lines (+205/-48)
2 files modified
tests/platform/sync_menu/test_linux.py (+139/-20)
ubuntuone/platform/sync_menu/linux.py (+66/-28)
To merge this branch: bzr merge lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch
Reviewer Review Type Date Requested Status
dobey (community) Approve on 2013-04-16
Diego Sarmentero (community) Approve on 2013-04-16
Roberto Alsina (community) 2013-04-11 Approve on 2013-04-12
Review via email: mp+158503@code.launchpad.net

Commit message

UbuntuOneSyncMenuLinux: use GAppInfo with proper context to launch URIs

This allows to correctly open the application with proper startup notify.

Description of the change

The linux sync-menu should use GAppInfo with LaunchContext to launch applications and URIs so that the window manager and the applications will be properly focused and launched with startup notification.

To post a comment you must log in.
Roberto Alsina (ralsina) :
review: Approve
1393. By Marco Trevisan (Treviño) on 2013-04-12

UbuntuOneSyncMenuLinux: protect the commandline-app-creation from error too

Diego Sarmentero (diegosarmentero) wrote :

Tests are failing, and there isn't new tests for the new code.

review: Needs Fixing
dobey (dobey) wrote :

+CLIENT_DESKTOP_ID = 'ubuntuone-control-panel-qt-gnome.desktop'
+INSTALLER_DESKTOP_ID = 'ubuntuone-installer.desktop'

Why are both of these being used? There is no more "installer" exactly, but the filename is unchanged, to avoid breaking the launcher. Also, the "-qt-gnome.desktop" file is only for GNOME, so I think we want to use the "ubuntuone-installer.desktop" everywhere. But I wouldn't call it "INSTALLER_DESKTOP_ID" as the variable, as it's not an installer. Maybe CONTROL_PANEL_DESKTOP_ID or CLIENT_DESKTOP_ID, and use that constant for all the instances.

dobey (dobey) :
review: Needs Fixing
1394. By Marco Trevisan (Treviño) on 2013-04-12

UbuntuOneSyncMenuLinux: don't crash if the display is not set, don't use the Context

1395. By Marco Trevisan (Treviño) on 2013-04-12

SyncMenuTestCase: add new tests to cover changes and update the old ones

1396. By Marco Trevisan (Treviño) on 2013-04-12

SyncMenuTestCase: use random values for timestasmp

1397. By Marco Trevisan (Treviño) on 2013-04-12

UbuntuOneSyncMenuLinux: always use CLIENT_DESKTOP_ID as client

Marco Trevisan (Treviño) (3v1n0) wrote :

> +CLIENT_DESKTOP_ID = 'ubuntuone-control-panel-qt-gnome.desktop'
> +INSTALLER_DESKTOP_ID = 'ubuntuone-installer.desktop'
>
> Why are both of these being used? There is no more "installer" exactly, but
> the filename is unchanged, to avoid breaking the launcher. Also, the "-qt-
> gnome.desktop" file is only for GNOME, so I think we want to use the
> "ubuntuone-installer.desktop" everywhere. But I wouldn't call it
> "INSTALLER_DESKTOP_ID" as the variable, as it's not an installer. Maybe
> CONTROL_PANEL_DESKTOP_ID or CLIENT_DESKTOP_ID, and use that constant for all
> the instances.

Fine, I've moved everything to CLIENT_DESKTOP_ID set to "ubuntuone-installer.desktop" now.

dobey (dobey) wrote :

There are some lint issues in the test file.

== Python Lint Notices ==

./tests/platform/sync_menu/test_linux.py:
    276: redefinition of unused 'time' from line 32
    286: redefinition of unused 'time' from line 32
    297: redefinition of unused 'time' from line 32
    306: redefinition of unused 'time' from line 32
    317: redefinition of unused 'time' from line 32
    326: redefinition of unused 'time' from line 32
    335: redefinition of unused 'time' from line 32
    344: redefinition of unused 'time' from line 32
    353: redefinition of unused 'time' from line 32

make: *** [lint] Error 1

dobey (dobey) wrote :

Also, is there any reason to use use randint() here instead of time.time()?

Diego Sarmentero (diegosarmentero) wrote :

This branch contains some lint issues:

== Python Lint Notices ==

./tests/platform/sync_menu/test_linux.py:
    276: redefinition of unused 'time' from line 32
    286: redefinition of unused 'time' from line 32
    297: redefinition of unused 'time' from line 32
    306: redefinition of unused 'time' from line 32
    317: redefinition of unused 'time' from line 32
    326: redefinition of unused 'time' from line 32
    335: redefinition of unused 'time' from line 32
    344: redefinition of unused 'time' from line 32
    353: redefinition of unused 'time' from line 32

review: Needs Fixing
1398. By Marco Trevisan (Treviño) on 2013-04-15

SyncMenuTestCase: use time.time() instead of random.randint().

Marco Trevisan (Treviño) (3v1n0) wrote :

> This branch contains some lint issues:

They should be fixed now, thanks for pointing out.

Diego Sarmentero (diegosarmentero) wrote :

+1

review: Approve
dobey (dobey) wrote :

8 +

E303 too many blank lines (3)

+ def __init__(self, command_line = "", name = "", flags = 0):
+ def __init__(self, desktop_id = ""):
+ def _open_uri(self, uri, timestamp = 0):
+ def _open_control_panel_by_command_line(self, timestamp, args = ''):
+ def open_control_panel(self, menuitem = None, timestamp = 0):
+ def open_ubuntu_one_folder(self, menuitem = None, timestamp = 0):
+ def open_share_file_tab(self, menuitem = None, timestamp = 0):
+ def open_go_to_web(self, menuitem = None, timestamp = 0):
+ def open_web_help(self, menuitem = None, timestamp = 0):
+ def open_get_more_storage(self, menuitem = None, timestamp = 0):

E251 no spaces around keyword / parameter equals

68 + self.context = context
69 +
70 +class FakeDesktopAppInfo(FakeAppInfo):

E302 expected 2 blank lines, found 1

1399. By Marco Trevisan (Treviño) on 2013-04-16

UbuntuOneSyncMenuLinux: fixing spaces issues

dobey (dobey) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/platform/sync_menu/test_linux.py'
2--- tests/platform/sync_menu/test_linux.py 2013-04-16 13:14:42 +0000
3+++ tests/platform/sync_menu/test_linux.py 2013-04-16 15:23:26 +0000
4@@ -70,6 +70,73 @@
5 self.callback = callback
6
7
8+class FakeAppLaunchContext(object):
9+ def set_timestamp(self, timestamp):
10+ self.timestamp = timestamp
11+
12+
13+class FakeGdkDisplay(object):
14+ """Fake Gdk.Display"""
15+ def get_app_launch_context(self):
16+ return FakeAppLaunchContext()
17+
18+
19+class FakeNullGdk(object):
20+ """Fake Gdk.Display with no default"""
21+ @staticmethod
22+ def get_default():
23+ return None
24+
25+
26+class FakeAppInfo(object):
27+ """Fake Gio.AppInfo"""
28+ instance = None
29+ name = ""
30+ desktop_id = ""
31+ command_line = ""
32+ opened_uri = ""
33+ launched = True
34+ context = None
35+ files = []
36+ flags = 0
37+
38+ def __new__(cls, *args, **kwargs):
39+ cls.instance = super(FakeAppInfo, cls).__new__(cls, *args, **kwargs)
40+ return cls.instance
41+
42+ def __init__(self, command_line="", name="", flags=0):
43+ self.command_line = command_line
44+ self.name = name
45+ self.flags = flags
46+
47+ @classmethod
48+ def launch_default_for_uri(cls, uri, context):
49+ cls.opened_uri = uri
50+ cls.context = context
51+
52+ @classmethod
53+ def create_from_commandline(cls, command_line, name, flags):
54+ cls.instance.__init__(command_line, name, flags)
55+ return cls.instance
56+
57+ def launch(self, files, context):
58+ self.launched = True
59+ self.files = files
60+ self.context = context
61+
62+
63+class FakeDesktopAppInfo(FakeAppInfo):
64+ """Fake Gio.DestkopAppInfo"""
65+ def __init__(self, desktop_id=""):
66+ super(FakeDesktopAppInfo, self).__init__()
67+ self.desktop_id = desktop_id
68+
69+ @classmethod
70+ def new(cls, desktop_id):
71+ cls.instance.__init__(desktop_id)
72+ return cls.instance
73+
74+
75 class FakeSyncdaemonService(object):
76 """Fake SyncdaemonService."""
77
78@@ -142,6 +209,7 @@
79 def setUp(self):
80 yield super(SyncMenuTestCase, self).setUp()
81 self.patch(linux.SyncMenu, "App", FakeSyncMenuApp)
82+ self.patch(linux.Gdk.Display, "get_default", FakeGdkDisplay)
83 FakeSyncMenuApp.clean()
84 self.syncdaemon_service = FakeSyncdaemonService()
85 self.status_frontend = FakeStatusFrontend()
86@@ -187,54 +255,105 @@
87 self.assertIsInstance(self.sync_menu.transfers.separator,
88 linux.Dbusmenu.Menuitem)
89
90+ def test_get_launch_context_with_display(self):
91+ """Check that the proper context is returned."""
92+ timestamp = time.time()
93+ context = self.sync_menu._get_launch_context(timestamp)
94+ self.assertEqual(timestamp, context.timestamp)
95+
96+ def test_get_launch_context_with_no_display(self):
97+ """Check that the proper context is returned."""
98+ self.patch(linux.Gdk, "Display", FakeNullGdk)
99+ context = self.sync_menu._get_launch_context(time.time())
100+ self.assertEqual(context, None)
101+
102+ def test_open_control_panel_by_command_line(self):
103+ """Check that the proper action is executed."""
104+ appinfo = FakeAppInfo()
105+ self.patch(linux.Gio, "AppInfo", appinfo)
106+ timestamp = time.time()
107+ self.sync_menu._open_control_panel_by_command_line(timestamp)
108+
109+ self.assertEqual(appinfo.command_line, linux.CLIENT_COMMAND_LINE)
110+ self.assertEqual(appinfo.context.timestamp, timestamp)
111+
112+ def test_open_control_panel_by_command_line_with_arg(self):
113+ """Check that the proper action is executed."""
114+ appinfo = FakeAppInfo()
115+ self.patch(linux.Gio, "AppInfo", appinfo)
116+ timestamp = time.time()
117+ arg = "--test-arg"
118+ self.sync_menu._open_control_panel_by_command_line(timestamp, arg)
119+
120+ self.assertEqual(appinfo.command_line, "%s %s" % (linux.CLIENT_COMMAND_LINE, arg))
121+ self.assertEqual(appinfo.context.timestamp, timestamp)
122+
123+ def test_open_uri(self):
124+ """Check that the proper action is executed."""
125+ appinfo = FakeAppInfo()
126+ self.patch(linux.Gio, "AppInfo", appinfo)
127+ timestamp = time.time()
128+
129+ self.sync_menu._open_uri(linux.UBUNTUONE_LINK, timestamp)
130+ self.assertEqual(appinfo.opened_uri, linux.UBUNTUONE_LINK)
131+ self.assertEqual(appinfo.context.timestamp, timestamp)
132+
133 def test_open_u1(self):
134 """Check that the proper action is executed."""
135- data = []
136+ appinfo = FakeDesktopAppInfo()
137+ timestamp = time.time()
138+ self.patch(linux.Gio, "DesktopAppInfo", appinfo)
139
140- self.patch(linux.glib, "spawn_command_line_async", data.append)
141- self.sync_menu.open_control_panel()
142- self.assertEqual(data, ['ubuntuone-control-panel-qt'])
143+ self.sync_menu.open_control_panel(timestamp=timestamp)
144+ self.assertEqual(appinfo.desktop_id, linux.CLIENT_DESKTOP_ID)
145+ self.assertTrue(appinfo.launched)
146+ self.assertEqual(appinfo.files, [])
147+ self.assertEqual(appinfo.context.timestamp, timestamp)
148
149 def test_open_share_tab(self):
150 """Check that the proper action is executed."""
151+ timestamp = time.time()
152 data = []
153
154- self.patch(linux.glib, "spawn_command_line_async", data.append)
155- self.sync_menu.open_share_file_tab()
156- self.assertEqual(data, [
157- 'ubuntuone-control-panel-qt --switch-to share_links'])
158+ self.patch(self.sync_menu, "_open_control_panel_by_command_line", lambda t, a: data.append((t, a)))
159+ self.sync_menu.open_share_file_tab(timestamp=timestamp)
160+ self.assertEqual(data, [(timestamp, "--switch-to share_links")])
161
162 def test_go_to_web(self):
163 """Check that the proper action is executed."""
164+ timestamp = time.time()
165 data = []
166
167- self.patch(linux.webbrowser, "open", data.append)
168- self.sync_menu.open_go_to_web()
169- self.assertEqual(data, [linux.DASHBOARD])
170+ self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
171+ self.sync_menu.open_go_to_web(timestamp=timestamp)
172+ self.assertEqual(data, [(timestamp, linux.DASHBOARD)])
173
174 def test_open_ubuntu_one_folder(self):
175 """Check that the proper action is executed."""
176+ timestamp = time.time()
177 data = []
178
179- self.patch(linux.webbrowser, "open", data.append)
180- self.sync_menu.open_ubuntu_one_folder()
181- self.assertEqual(data, [self.syncdaemon_service.fake_root_path])
182+ self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
183+ self.sync_menu.open_ubuntu_one_folder(timestamp=timestamp)
184+ self.assertEqual(data, [(timestamp, "file://" + self.syncdaemon_service.fake_root_path)])
185
186 def test_get_help(self):
187 """Check that the proper action is executed."""
188+ timestamp = time.time()
189 data = []
190
191- self.patch(linux.webbrowser, "open", data.append)
192- self.sync_menu.open_web_help()
193- self.assertEqual(data, [linux.HELP_LINK])
194+ self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
195+ self.sync_menu.open_web_help(timestamp=timestamp)
196+ self.assertEqual(data, [(timestamp, linux.HELP_LINK)])
197
198 def test_more_storage(self):
199 """Check that the proper action is executed."""
200+ timestamp = time.time()
201 data = []
202
203- self.patch(linux.webbrowser, "open", data.append)
204- self.sync_menu.open_get_more_storage()
205- self.assertEqual(data, [linux.GET_STORAGE_LINK])
206+ self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
207+ self.sync_menu.open_get_more_storage(timestamp=timestamp)
208+ self.assertEqual(data, [(timestamp, linux.GET_STORAGE_LINK)])
209
210 def test_empty_transfers(self):
211 """Check that the Transfers menu is empty."""
212
213=== modified file 'ubuntuone/platform/sync_menu/linux.py'
214--- ubuntuone/platform/sync_menu/linux.py 2013-02-20 22:41:12 +0000
215+++ ubuntuone/platform/sync_menu/linux.py 2013-04-16 15:23:26 +0000
216@@ -31,7 +31,6 @@
217 import gettext
218 import logging
219 import time
220-import webbrowser
221 from twisted.python.util import OrderedDict
222 from operator import itemgetter
223
224@@ -39,6 +38,8 @@
225 from gi.repository import GLib as glib
226 from gi.repository import (
227 Dbusmenu,
228+ Gdk,
229+ Gio,
230 SyncMenu,
231 )
232 use_syncmenu = True
233@@ -69,6 +70,8 @@
234 DASHBOARD = UBUNTUONE_LINK + u'dashboard/'
235 HELP_LINK = UBUNTUONE_LINK + u'support/'
236 GET_STORAGE_LINK = UBUNTUONE_LINK + u'services/add-storage/'
237+CLIENT_COMMAND_LINE = 'ubuntuone-control-panel-qt'
238+CLIENT_DESKTOP_ID = 'ubuntuone-installer.desktop'
239
240
241 class UbuntuOneSyncMenuLinux(object):
242@@ -133,7 +136,7 @@
243
244 self.server = Dbusmenu.Server()
245 self.server.set_root(self.root_menu)
246- self.app = SyncMenu.App.new("ubuntuone-installer.desktop")
247+ self.app = SyncMenu.App.new(CLIENT_DESKTOP_ID)
248 self.app.set_menu(self.server)
249 self.app.connect("notify::paused", self.change_sync_status)
250
251@@ -155,36 +158,70 @@
252 self._syncdaemon_service.connect()
253 self._connected = True
254
255- def open_control_panel(self, *args):
256+ def _get_launch_context(self, timestamp):
257+ """Returns the launch context for the current display"""
258+ dpy = Gdk.Display.get_default()
259+
260+ if dpy:
261+ context = dpy.get_app_launch_context()
262+ context.set_timestamp(timestamp)
263+ return context
264+
265+ return None
266+
267+ def _open_uri(self, uri, timestamp=0):
268+ """Open an uri Using the default handler and the action timestamp"""
269+ try:
270+ Gio.AppInfo.launch_default_for_uri(uri, self._get_launch_context(timestamp))
271+ except glib.GError as e:
272+ logger.warning('Failed to open the uri %s: %s.' % (uri, e))
273+
274+ def _open_control_panel_by_command_line(self, timestamp, args = ''):
275+ """Open the control panel by command line"""
276+ flags = Gio.AppInfoCreateFlags.SUPPORTS_STARTUP_NOTIFICATION
277+ command_line = CLIENT_COMMAND_LINE
278+ if len(args):
279+ command_line += ' ' + args
280+
281+ try:
282+ app = Gio.AppInfo.create_from_commandline(command_line, 'Ubuntu One', flags)
283+
284+ if app:
285+ app.launch([], self._get_launch_context(timestamp))
286+ except glib.GError as e:
287+ logger.warning('Failed to open the control panel: %s.' % e)
288+
289+ def open_control_panel(self, menuitem=None, timestamp=0):
290 """Open the Ubuntu One Control Panel."""
291- try:
292- glib.spawn_command_line_async('ubuntuone-control-panel-qt')
293- except glib.GError as e:
294- logger.warning('Failed to open the control panel: %s.' % e)
295-
296- def open_ubuntu_one_folder(self, *args):
297+ app = Gio.DesktopAppInfo.new(CLIENT_DESKTOP_ID)
298+
299+ if app:
300+ try:
301+ app.launch([], self._get_launch_context(timestamp))
302+ except glib.GError as e:
303+ logger.warning('Failed to open the control panel: %s.' % e)
304+ else:
305+ self._open_control_panel_by_command_line(timestamp)
306+
307+ def open_ubuntu_one_folder(self, menuitem=None, timestamp=0):
308 """Open the Ubuntu One folder."""
309- webbrowser.open(self._syncdaemon_service.get_rootdir())
310+ self._open_uri("file://" + self._syncdaemon_service.get_rootdir(), timestamp)
311
312- def open_share_file_tab(self, *args):
313+ def open_share_file_tab(self, menuitem=None, timestamp=0):
314 """Open the Control Panel in the Share Tab."""
315- try:
316- glib.spawn_command_line_async('ubuntuone-control-panel-qt '
317- '--switch-to share_links')
318- except glib.GError as e:
319- logger.warning('Failed to open the control panel: %s.' % e)
320-
321- def open_go_to_web(self, *args):
322- """Open the Ubunto One Help Page"""
323- webbrowser.open(DASHBOARD)
324-
325- def open_web_help(self, *args):
326- """Open the Ubunto One Help Page"""
327- webbrowser.open(HELP_LINK)
328-
329- def open_get_more_storage(self, *args):
330- """Open the Ubunto One Help Page"""
331- webbrowser.open(GET_STORAGE_LINK)
332+ self._open_control_panel_by_command_line(timestamp, "--switch-to share_links")
333+
334+ def open_go_to_web(self, menuitem=None, timestamp=0):
335+ """Open the Ubuntu One Help Page"""
336+ self._open_uri(DASHBOARD, timestamp)
337+
338+ def open_web_help(self, menuitem=None, timestamp=0):
339+ """Open the Ubuntu One Help Page"""
340+ self._open_uri(HELP_LINK, timestamp)
341+
342+ def open_get_more_storage(self, menuitem=None, timestamp=0):
343+ """Open the Ubuntu One Help Page"""
344+ self._open_uri(GET_STORAGE_LINK, timestamp)
345
346 def _timeout(self, result):
347 """The aggregating timer has expired, so update the UI."""
348@@ -284,3 +321,4 @@
349 UbuntuOneSyncMenu = UbuntuOneSyncMenuLinux
350 else:
351 UbuntuOneSyncMenu = DummySyncMenu
352+

Subscribers

People subscribed via source and target branches