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

[Marco Trevisan (Treviño)]

UbuntuOneSyncMenuLinux: use GAppInfo with proper context to launch URIs

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

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.

[Roberto Alsina (ralsina)]

review: Approve

[lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch]

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

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

[Diego Sarmentero (diegosarmentero)]

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

review: Needs Fixing

[dobey (dobey)]

+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

[lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch]

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)]

> +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)]

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)]

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

[Diego Sarmentero (diegosarmentero)]

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

[lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch]

1398. By Marco Trevisan (Treviño) on 2013-04-15

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

[Marco Trevisan (Treviño) (3v1n0)]

> This branch contains some lint issues:

They should be fixed now, thanks for pointing out.

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[dobey (dobey)]

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

[lp:~3v1n0/ubuntuone-client/sync-menu-use-gappinfo-launch]

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

UbuntuOneSyncMenuLinux: fixing spaces issues

[dobey (dobey)]

review: Approve

=== modified file 'tests/platform/sync_menu/test_linux.py'
--- tests/platform/sync_menu/test_linux.py	2013-04-16 13:14:42 +0000
+++ tests/platform/sync_menu/test_linux.py	2013-04-16 15:23:26 +0000
@@ -70,6 +70,73 @@
         self.callback = callback
 
 
+class FakeAppLaunchContext(object):
+    def set_timestamp(self, timestamp):
+        self.timestamp = timestamp
+
+
+class FakeGdkDisplay(object):
+    """Fake Gdk.Display"""
+    def get_app_launch_context(self):
+        return FakeAppLaunchContext()
+
+
+class FakeNullGdk(object):
+    """Fake Gdk.Display with no default"""
+    @staticmethod
+    def get_default():
+        return None
+
+
+class FakeAppInfo(object):
+    """Fake Gio.AppInfo"""
+    instance = None
+    name = ""
+    desktop_id = ""
+    command_line = ""
+    opened_uri = ""
+    launched = True
+    context = None
+    files = []
+    flags = 0
+
+    def __new__(cls, *args, **kwargs):
+        cls.instance = super(FakeAppInfo, cls).__new__(cls, *args, **kwargs)
+        return cls.instance
+
+    def __init__(self, command_line="", name="", flags=0):
+        self.command_line = command_line
+        self.name = name
+        self.flags = flags
+
+    @classmethod
+    def launch_default_for_uri(cls, uri, context):
+        cls.opened_uri = uri
+        cls.context = context
+
+    @classmethod
+    def create_from_commandline(cls, command_line, name, flags):
+        cls.instance.__init__(command_line, name, flags)
+        return cls.instance
+
+    def launch(self, files, context):
+        self.launched = True
+        self.files = files
+        self.context = context
+
+
+class FakeDesktopAppInfo(FakeAppInfo):
+    """Fake Gio.DestkopAppInfo"""
+    def __init__(self, desktop_id=""):
+        super(FakeDesktopAppInfo, self).__init__()
+        self.desktop_id = desktop_id
+
+    @classmethod
+    def new(cls, desktop_id):
+        cls.instance.__init__(desktop_id)
+        return cls.instance
+
+
 class FakeSyncdaemonService(object):
     """Fake SyncdaemonService."""
 
@@ -142,6 +209,7 @@
     def setUp(self):
         yield super(SyncMenuTestCase, self).setUp()
         self.patch(linux.SyncMenu, "App", FakeSyncMenuApp)
+        self.patch(linux.Gdk.Display, "get_default", FakeGdkDisplay)
         FakeSyncMenuApp.clean()
         self.syncdaemon_service = FakeSyncdaemonService()
         self.status_frontend = FakeStatusFrontend()
@@ -187,54 +255,105 @@
         self.assertIsInstance(self.sync_menu.transfers.separator,
             linux.Dbusmenu.Menuitem)
 
+    def test_get_launch_context_with_display(self):
+        """Check that the proper context is returned."""
+        timestamp = time.time()
+        context = self.sync_menu._get_launch_context(timestamp)
+        self.assertEqual(timestamp, context.timestamp)
+
+    def test_get_launch_context_with_no_display(self):
+        """Check that the proper context is returned."""
+        self.patch(linux.Gdk, "Display", FakeNullGdk)
+        context = self.sync_menu._get_launch_context(time.time())
+        self.assertEqual(context, None)
+
+    def test_open_control_panel_by_command_line(self):
+        """Check that the proper action is executed."""
+        appinfo = FakeAppInfo()
+        self.patch(linux.Gio, "AppInfo", appinfo)
+        timestamp = time.time()
+        self.sync_menu._open_control_panel_by_command_line(timestamp)
+
+        self.assertEqual(appinfo.command_line, linux.CLIENT_COMMAND_LINE)
+        self.assertEqual(appinfo.context.timestamp, timestamp)
+
+    def test_open_control_panel_by_command_line_with_arg(self):
+        """Check that the proper action is executed."""
+        appinfo = FakeAppInfo()
+        self.patch(linux.Gio, "AppInfo", appinfo)
+        timestamp = time.time()
+        arg = "--test-arg"
+        self.sync_menu._open_control_panel_by_command_line(timestamp, arg)
+
+        self.assertEqual(appinfo.command_line, "%s %s" % (linux.CLIENT_COMMAND_LINE, arg))
+        self.assertEqual(appinfo.context.timestamp, timestamp)
+
+    def test_open_uri(self):
+        """Check that the proper action is executed."""
+        appinfo = FakeAppInfo()
+        self.patch(linux.Gio, "AppInfo", appinfo)
+        timestamp = time.time()
+
+        self.sync_menu._open_uri(linux.UBUNTUONE_LINK, timestamp)
+        self.assertEqual(appinfo.opened_uri, linux.UBUNTUONE_LINK)
+        self.assertEqual(appinfo.context.timestamp, timestamp)
+
     def test_open_u1(self):
         """Check that the proper action is executed."""
-        data = []
+        appinfo = FakeDesktopAppInfo()
+        timestamp = time.time()
+        self.patch(linux.Gio, "DesktopAppInfo", appinfo)
 
-        self.patch(linux.glib, "spawn_command_line_async", data.append)
-        self.sync_menu.open_control_panel()
-        self.assertEqual(data, ['ubuntuone-control-panel-qt'])
+        self.sync_menu.open_control_panel(timestamp=timestamp)
+        self.assertEqual(appinfo.desktop_id, linux.CLIENT_DESKTOP_ID)
+        self.assertTrue(appinfo.launched)
+        self.assertEqual(appinfo.files, [])
+        self.assertEqual(appinfo.context.timestamp, timestamp)
 
     def test_open_share_tab(self):
         """Check that the proper action is executed."""
+        timestamp = time.time()
         data = []
 
-        self.patch(linux.glib, "spawn_command_line_async", data.append)
-        self.sync_menu.open_share_file_tab()
-        self.assertEqual(data, [
-            'ubuntuone-control-panel-qt --switch-to share_links'])
+        self.patch(self.sync_menu, "_open_control_panel_by_command_line", lambda t, a: data.append((t, a)))
+        self.sync_menu.open_share_file_tab(timestamp=timestamp)
+        self.assertEqual(data, [(timestamp, "--switch-to share_links")])
 
     def test_go_to_web(self):
         """Check that the proper action is executed."""
+        timestamp = time.time()
         data = []
 
-        self.patch(linux.webbrowser, "open", data.append)
-        self.sync_menu.open_go_to_web()
-        self.assertEqual(data, [linux.DASHBOARD])
+        self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
+        self.sync_menu.open_go_to_web(timestamp=timestamp)
+        self.assertEqual(data, [(timestamp, linux.DASHBOARD)])
 
     def test_open_ubuntu_one_folder(self):
         """Check that the proper action is executed."""
+        timestamp = time.time()
         data = []
 
-        self.patch(linux.webbrowser, "open", data.append)
-        self.sync_menu.open_ubuntu_one_folder()
-        self.assertEqual(data, [self.syncdaemon_service.fake_root_path])
+        self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
+        self.sync_menu.open_ubuntu_one_folder(timestamp=timestamp)
+        self.assertEqual(data, [(timestamp, "file://" + self.syncdaemon_service.fake_root_path)])
 
     def test_get_help(self):
         """Check that the proper action is executed."""
+        timestamp = time.time()
         data = []
 
-        self.patch(linux.webbrowser, "open", data.append)
-        self.sync_menu.open_web_help()
-        self.assertEqual(data, [linux.HELP_LINK])
+        self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
+        self.sync_menu.open_web_help(timestamp=timestamp)
+        self.assertEqual(data, [(timestamp, linux.HELP_LINK)])
 
     def test_more_storage(self):
         """Check that the proper action is executed."""
+        timestamp = time.time()
         data = []
 
-        self.patch(linux.webbrowser, "open", data.append)
-        self.sync_menu.open_get_more_storage()
-        self.assertEqual(data, [linux.GET_STORAGE_LINK])
+        self.patch(self.sync_menu, "_open_uri", lambda u, t: data.append((t, u)))
+        self.sync_menu.open_get_more_storage(timestamp=timestamp)
+        self.assertEqual(data, [(timestamp, linux.GET_STORAGE_LINK)])
 
     def test_empty_transfers(self):
         """Check that the Transfers menu is empty."""
 
=== modified file 'ubuntuone/platform/sync_menu/linux.py'
--- ubuntuone/platform/sync_menu/linux.py	2013-02-20 22:41:12 +0000
+++ ubuntuone/platform/sync_menu/linux.py	2013-04-16 15:23:26 +0000
@@ -31,7 +31,6 @@
 import gettext
 import logging
 import time
-import webbrowser
 from twisted.python.util import OrderedDict
 from operator import itemgetter
 
@@ -39,6 +38,8 @@
     from gi.repository import GLib as glib
     from gi.repository import (
         Dbusmenu,
+        Gdk,
+        Gio,
         SyncMenu,
     )
     use_syncmenu = True
@@ -69,6 +70,8 @@
 DASHBOARD = UBUNTUONE_LINK + u'dashboard/'
 HELP_LINK = UBUNTUONE_LINK + u'support/'
 GET_STORAGE_LINK = UBUNTUONE_LINK + u'services/add-storage/'
+CLIENT_COMMAND_LINE = 'ubuntuone-control-panel-qt'
+CLIENT_DESKTOP_ID = 'ubuntuone-installer.desktop'
 
 
 class UbuntuOneSyncMenuLinux(object):
@@ -133,7 +136,7 @@
 
         self.server = Dbusmenu.Server()
         self.server.set_root(self.root_menu)
-        self.app = SyncMenu.App.new("ubuntuone-installer.desktop")
+        self.app = SyncMenu.App.new(CLIENT_DESKTOP_ID)
         self.app.set_menu(self.server)
         self.app.connect("notify::paused", self.change_sync_status)
 
@@ -155,36 +158,70 @@
             self._syncdaemon_service.connect()
             self._connected = True
 
-    def open_control_panel(self, *args):
+    def _get_launch_context(self, timestamp):
+        """Returns the launch context for the current display"""
+        dpy = Gdk.Display.get_default()
+
+        if dpy:
+            context = dpy.get_app_launch_context()
+            context.set_timestamp(timestamp)
+            return context
+
+        return None
+
+    def _open_uri(self, uri, timestamp=0):
+        """Open an uri Using the default handler and the action timestamp"""
+        try:
+            Gio.AppInfo.launch_default_for_uri(uri, self._get_launch_context(timestamp))
+        except glib.GError as e:
+            logger.warning('Failed to open the uri %s: %s.' % (uri, e))
+
+    def _open_control_panel_by_command_line(self, timestamp, args = ''):
+        """Open the control panel by command line"""
+        flags = Gio.AppInfoCreateFlags.SUPPORTS_STARTUP_NOTIFICATION
+        command_line = CLIENT_COMMAND_LINE
+        if len(args):
+            command_line += ' ' + args
+
+        try:
+            app = Gio.AppInfo.create_from_commandline(command_line, 'Ubuntu One', flags)
+
+            if app:
+                app.launch([], self._get_launch_context(timestamp))
+        except glib.GError as e:
+            logger.warning('Failed to open the control panel: %s.' % e)
+
+    def open_control_panel(self, menuitem=None, timestamp=0):
         """Open the Ubuntu One Control Panel."""
-        try:
-            glib.spawn_command_line_async('ubuntuone-control-panel-qt')
-        except glib.GError as e:
-            logger.warning('Failed to open the control panel: %s.' % e)
-
-    def open_ubuntu_one_folder(self, *args):
+        app = Gio.DesktopAppInfo.new(CLIENT_DESKTOP_ID)
+
+        if app:
+            try:
+                app.launch([], self._get_launch_context(timestamp))
+            except glib.GError as e:
+                logger.warning('Failed to open the control panel: %s.' % e)
+        else:
+            self._open_control_panel_by_command_line(timestamp)
+
+    def open_ubuntu_one_folder(self, menuitem=None, timestamp=0):
         """Open the Ubuntu One folder."""
-        webbrowser.open(self._syncdaemon_service.get_rootdir())
+        self._open_uri("file://" + self._syncdaemon_service.get_rootdir(), timestamp)
 
-    def open_share_file_tab(self, *args):
+    def open_share_file_tab(self, menuitem=None, timestamp=0):
         """Open the Control Panel in the Share Tab."""
-        try:
-            glib.spawn_command_line_async('ubuntuone-control-panel-qt '
-                '--switch-to share_links')
-        except glib.GError as e:
-            logger.warning('Failed to open the control panel: %s.' % e)
-
-    def open_go_to_web(self, *args):
-        """Open the Ubunto One Help Page"""
-        webbrowser.open(DASHBOARD)
-
-    def open_web_help(self, *args):
-        """Open the Ubunto One Help Page"""
-        webbrowser.open(HELP_LINK)
-
-    def open_get_more_storage(self, *args):
-        """Open the Ubunto One Help Page"""
-        webbrowser.open(GET_STORAGE_LINK)
+        self._open_control_panel_by_command_line(timestamp, "--switch-to share_links")
+
+    def open_go_to_web(self, menuitem=None, timestamp=0):
+        """Open the Ubuntu One Help Page"""
+        self._open_uri(DASHBOARD, timestamp)
+
+    def open_web_help(self, menuitem=None, timestamp=0):
+        """Open the Ubuntu One Help Page"""
+        self._open_uri(HELP_LINK, timestamp)
+
+    def open_get_more_storage(self, menuitem=None, timestamp=0):
+        """Open the Ubuntu One Help Page"""
+        self._open_uri(GET_STORAGE_LINK, timestamp)
 
     def _timeout(self, result):
         """The aggregating timer has expired, so update the UI."""
@@ -284,3 +321,4 @@
     UbuntuOneSyncMenu = UbuntuOneSyncMenuLinux
 else:
     UbuntuOneSyncMenu = DummySyncMenu
+