Merge lp:~victored/granite/contractor-wrapper into lp:~elementary-pantheon/granite/granite

[Victor Martinez]

I've modified the public API of Contractor to be contract-id independent, so that we encourage code that manages contracts "blindly".

---
Akshay will be away from coding for a couple of days (or weeks?), so I'm proposing this alternative API for contractor. This is based on Akshay's work.

[Sergey "Shnatsel" Davidoff (shnatsel)]

After a long discussion about the API this seems to be the best option we could possibly get. Yay!

There are two things to do before this goes in:
1) Update Contractor daemon D-bus API to throw errors
2) Create a test client for this API

review: Approve

[Victor Martinez (victored)]

(2) lp:~victored/pantheon-files/contractor-plugin

[Sergey "Shnatsel" Davidoff (shnatsel)]

Too bad I can't test it with testing scripts because the Granite build fails due to symbol changes.

[Sergey "Shnatsel" Davidoff (shnatsel)]

Here are the symbol changes: http://pastebin.com/dyGnNqUi

Looks like this branch breaks ABI.

[Victor Martinez (victored)]

Indeed, this breaks the ABI, since it removes the constructors. I want to propose a solution that doesn't break the ABI:

1) Leave the previous Contractor class and it's DBus interface as they are, make their methods return empty hashmaps, and add the proper deprecation annotations. In other words, leave the previous contractor ABI untouched.

2) In addition to (1), Modify the class name for the new Contractor API. Instead of Granite.Services.Contractor we could use something like Granite.Services.ContractorProxy (please let me know if you can figure out a better name!) for the new API. This way we only add symbols without removing or modifying any previous symbol.

review: Needs Fixing

[Rico Tzschichholz (ricotz)]

"Removing" the constructor is fine since it is suppose to be a singleton which is a needed API break. And preserving ABI in this area shouldn't be a target anyway. So better break it for real if the consumers are fixable in no-time.

[Rico Tzschichholz (ricotz)]

Please list/mention the current consumers of the contractor api

[Victor Martinez (victored)]

The only ones I know are Scratch and Pantheon Files. Of course there might be others, but since the new API depends on a new daemon, those applications will have to migrate to the new API eventually.

I am handling the port of Pantheon Files to the new API on the branch mentioned above.

The ABI breakage should be gone now.

[Sergey "Shnatsel" Davidoff (shnatsel)]

Here's a list of current Contractor users (From https://docs.google.com/a/elementaryos.org/document/d/1Ijsc57vYEHBZxVdM0fRgCuBX2NbdRDv1kuOj0OG75v4/edit#):

    ContractorMenu widget in Granite

    ContractorView widget in Granite

    Context menu of Pantheon-Files

    Maya, Scratch, Midori and elementary-flavored Simple Scan access Contractor via Granite convenience functions and rely on the fact that Contractor returns a string to be executed

    Eidete uses ContractorView widget from Granite, needs to be updated due to API break

[Victor Martinez (victored)]

ABI break fixed. Now we only have symbol additions and no removals.

Build log: http://pastebin.ubuntu.com/5594664/

[Victor Martinez (victored)]

The current merge stopper is bug #1171690 IMO.

review: Needs Fixing

[Sergey "Shnatsel" Davidoff (shnatsel)]

Bug #1171690 debunked.

[Victor Martinez (victored)]

review: Approve

[Sergey "Shnatsel" Davidoff (shnatsel)]

I've tested it with lp:~victored/pantheon-files/contractor-plugin and it seems to work fine.

review: Approve

[lp:~victored/granite/contractor-wrapper]

574. By Victor Martinez on 2013-04-23

Contracts can specify either an icon name or an absolute path to an icon file. Make sure we support both of them properly.

575. By Victor Martinez on 2013-04-23

Avoid copying structs to avoid segmentation fault

[Sergey "Shnatsel" Davidoff (shnatsel)]

Files still exhibits a bug that's not present in raw D-bus API: https://dl.dropboxusercontent.com/u/5279564/contractor-item-dupe-bug.webm

review: Needs Fixing

[Sergey "Shnatsel" Davidoff (shnatsel)]

After this is approved I'll land Contractor 0.3 to PPA. Everything is ready, we just have to enable the recipe: https://code.launchpad.net/~elementary-os/+recipe/contractor-daily

[lp:~victored/granite/contractor-wrapper]

576. By Victor Martinez on 2013-04-24

Let's keep the references to the contracts around all the time.

Previously we were depending on references held by client code, but it's a source of bugs, so better keep our own references in addition to external references.

[Victor Martinez (victored)]

> Files still exhibits a bug that's not present in raw D-bus API: https://dl.dropboxusercontent.com/u/5279564/contractor-item-dupe-bug.webm

This bug is only related to Files. The DBus API and this wrapper API are doing their job properly.

[Sergey "Shnatsel" Davidoff (shnatsel)]

Works fine for me now. Thanks!

review: Approve

=== modified file 'demo/main.vala'
--- demo/main.vala	2013-03-20 15:42:25 +0000
+++ demo/main.vala	2013-04-24 17:07:27 +0000
@@ -13,7 +13,7 @@
     but WITHOUT ANY WARRANTY; without even the implied warranty of
     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
     Lesser General Public License for more details.
- 
+
     You should have received a copy of the GNU Lesser General
     Public License along with this library; if not, write to the
     Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
@@ -276,32 +276,6 @@
         right_sep.set_expand (true);
         main_toolbar.insert (right_sep, -1);
 
-        // Contractor
-        var contractor_tab = new Gtk.Box (Gtk.Orientation.VERTICAL, 0);
-        var text_view = new Gtk.TextView ();
-
-        var hash_tables = Granite.Services.Contractor.get_contract ("/.zip", "application/zip");
-        foreach (var hash_table in hash_tables) {
-            text_view.buffer.text += hash_table.lookup ("Name")
-                                  + ": " + hash_table.lookup ("Description")
-                                  + " icon: " + hash_table.lookup ("Exec") + "\n";
-        }
-
-        contractor_tab.add (text_view);
-        contractor_tab.add (new Granite.Widgets.ContractorView ("file:///home/user/file.txt", "text/plain"));
-        var contractor_menu = new Granite.Widgets.ContractorMenu ("/home/user/file.txt", "text");
-        var contractor_button_image = new Gtk.Image.from_icon_name ("document-export",
-                                                                     Gtk.IconSize.LARGE_TOOLBAR);
-        var contractor_tool_item = new Granite.Widgets.ToolButtonWithMenu (contractor_button_image,
-                                                                            "Share", contractor_menu);
-        main_toolbar.insert (contractor_tool_item, -1);
-
-        contractor_tool_item.halign = contractor_tool_item.valign = Gtk.Align.CENTER;
-
-        var contractor_item = new SourceListItem ("Contractor");
-        contractor_item.page_num = page_switcher.append_page (contractor_tab, null);
-        services_category.add (contractor_item);
-
         // Search Entry
         var search_entry = new Granite.Widgets.SearchBar ("Search");
         var search_item = new Gtk.ToolItem ();
@@ -343,42 +317,42 @@
 
     private void show_light_window () {
         var light_window = new Granite.Widgets.LightWindow ();
-        
+
         var light_window_notebook = new Granite.Widgets.StaticNotebook ();
         var entry = new Gtk.Entry ();
         var open_drop = new Gtk.ComboBoxText ();
         var open_lbl = new LLabel ("Alwas Open Mpeg Video Files with Audience");
-        
+
         var grid = new Gtk.Grid ();
         grid.attach (new Gtk.Image.from_icon_name ("video-x-generic", Gtk.IconSize.DIALOG), 0, 0, 1, 2);
         grid.attach (entry, 1, 0, 1, 1);
         grid.attach (new LLabel ("1.13 GB, Mpeg Video File"), 1, 1, 1, 1);
-        
+
         grid.attach (light_window_notebook, 0, 2, 2, 1);
-        
+
         var general = new Gtk.Grid ();
         general.attach (new LLabel.markup ("<b>Info:</b>"), 0, 0, 2, 1);
-        
+
         general.attach (new LLabel.right ("Created:"), 0, 1, 1, 1);
         general.attach (new LLabel.right ("Modified:"), 0, 2, 1, 1);
         general.attach (new LLabel.right ("Opened:"), 0, 3, 1, 1);
         general.attach (new LLabel.right ("Mimetype:"), 0, 4, 1, 1);
         general.attach (new LLabel.right ("Location:"), 0, 5, 1, 1);
-        
+
         general.attach (new LLabel ("Today at 9:50 PM"), 1, 1, 1, 1);
         general.attach (new LLabel ("Today at 9:50 PM"), 1, 2, 1, 1);
         general.attach (new LLabel ("Today at 10:00 PM"), 1, 3, 1, 1);
         general.attach (new LLabel ("video/mpeg"), 1, 4, 1, 1);
         general.attach (new LLabel ("/home/daniel/Downloads"), 1, 5, 1, 1);
-        
+
         general.attach (new LLabel.markup ("<b>Open with:</b>"), 0, 6, 2, 1);
         general.attach (open_drop, 0, 7, 2, 1);
         general.attach (open_lbl, 0, 8, 2, 1);
-        
+
         light_window_notebook.append_page (general, new Gtk.Label ("General"));
         light_window_notebook.append_page (new Gtk.Label ("More"), new Gtk.Label ("More"));
         light_window_notebook.append_page (new Gtk.Label ("Sharing"), new Gtk.Label ("Sharing"));
-        
+
         open_lbl.margin_left = 24;
         open_drop.margin_left = 12;
         open_drop.append ("audience", "Audience");
@@ -389,7 +363,7 @@
         entry.text = "Cool Hand Luke";
         general.column_spacing = 6;
         general.row_spacing = 6;
-        
+
         light_window.add (grid);
         light_window.show_all ();
     }
 
=== modified file 'lib/CMakeLists.txt'
--- lib/CMakeLists.txt	2012-12-21 00:46:56 +0000
+++ lib/CMakeLists.txt	2013-04-24 17:07:27 +0000
@@ -16,6 +16,7 @@
     Services/Paths.vala
     Services/System.vala
     Services/Contractor.vala
+    Services/ContractorProxy.vala
     Services/IconFactory.vala
     Services/SimpleCommand.vala
     Widgets/Utils.vala
 
=== added file 'lib/Services/Contractor.vala'
--- lib/Services/Contractor.vala	1970-01-01 00:00:00 +0000
+++ lib/Services/Contractor.vala	2013-04-24 17:07:27 +0000
@@ -0,0 +1,67 @@
+/***
+    Copyright (C) 2011-2013 Lucas Baudin <[e-mail]>
+
+    This program or library is free software; you can redistribute it
+    and/or modify it under the terms of the GNU Lesser General Public
+    License as published by the Free Software Foundation; either
+    version 3 of the License, or (at your option) any later version.
+
+    This library is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+    Lesser General Public License for more details.
+ 
+    You should have received a copy of the GNU Lesser General
+    Public License along with this library; if not, write to the
+    Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+    Boston, MA 02110-1301 USA.
+***/
+
+namespace Granite.Services {
+    [DBus (name = "org.elementary.Contractor")]
+    interface ContractorDBus : Object {
+        public abstract GLib.HashTable<string,string>[] GetServicesByLocation (string strlocation, string? file_mime = "") throws IOError;
+        public abstract GLib.HashTable<string,string>[] GetServicesByLocationsList (GLib.HashTable<string,string>[] locations) throws IOError;
+    }
+
+    /**
+     * A way to handle contractor, a way to communicate between apps.
+     * 
+     * /!\ Highly unstable API
+     */
+    [Deprecated (replacement = "Granite.Services.ContractorProxy", since = "0.2")]
+    public class Contractor : Object {
+        internal ContractorDBus contract;
+        internal static Contractor? contractor = null;
+
+        /**
+         * This creates a new Contractor 
+         */
+        public Contractor () {
+        }
+
+        internal static void ensure () {
+        }
+
+        /**
+         * This searches for available contracts of a particular file
+         *
+         * @param uri uri of file
+         * @param mime mime type of file
+         * @return Hashtable of available contracts
+         */
+        public static GLib.HashTable<string,string>[] get_contract (string uri, string mime) {
+            return { new GLib.HashTable<string,string> (null, null) };
+        }
+
+        /**
+         * generate contracts for arguments and filter them by common parent mimetype.
+         *
+         * @param locations Hashtable of locations
+         * @return Hashtable of available contracts
+         */
+        public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations) {
+            return { new GLib.HashTable<string,string> (null, null) };
+        }
+    }
+}
 
=== renamed file 'lib/Services/Contractor.vala' => 'lib/Services/ContractorProxy.vala'
--- lib/Services/Contractor.vala	2013-04-04 20:05:38 +0000
+++ lib/Services/ContractorProxy.vala	2013-04-24 17:07:27 +0000
@@ -1,5 +1,7 @@
 /***
-    Copyright (C) 2011-2013 Lucas Baudin <[e-mail]>
+    Copyright (C) 2011-2013 Lucas Baudin <[e-mail]>,
+                            Akshay Shekher <[e-mail]>,
+                            Victor Martinez <[e-mail]>
 
     This program or library is free software; you can redistribute it
     and/or modify it under the terms of the GNU Lesser General Public
@@ -10,7 +12,7 @@
     but WITHOUT ANY WARRANTY; without even the implied warranty of
     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
     Lesser General Public License for more details.
- 
+
     You should have received a copy of the GNU Lesser General
     Public License along with this library; if not, write to the
     Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
@@ -18,87 +20,190 @@
 ***/
 
 namespace Granite.Services {
-
-    [DBus (name = "org.elementary.contractor")]
-    interface ContractorDBus : Object
-    {
-        public abstract GLib.HashTable<string,string>[] GetServicesByLocation (string strlocation, string? file_mime="")    throws IOError;
-        public abstract GLib.HashTable<string,string>[] GetServicesByLocationsList (GLib.HashTable<string,string>[] locations)  throws IOError;
-    }
-
-    /**
-     * A way to handle contractor, a way to communicate between apps.
-     * 
-     * /!\ Highly unstable API
-     */
-    public class Contractor : Object
-    {
-
-        internal ContractorDBus contract;
-
-        internal static Contractor? contractor = null;
-
-        /**
-         * This creates a new Contractor 
-         */
-        public Contractor()
-        {
-            try
-            {
-                contract = Bus.get_proxy_sync (BusType.SESSION,
-                                               "org.elementary.contractor",
-                                               "/org/elementary/contractor");
-            }
-            catch (IOError e)
-            {
-                stderr.printf ("%s\n", e.message);
-            }
-        }
-
-        internal static void ensure ()
-        {
-            if(contractor == null) contractor = new Contractor ();
-        }
-
-        /**
-         * This searches for available contracts of a particular file
-         * 
-         * @param uri uri of file
-         * @param mime mime type of file
-         * @return Hashtable of available contracts
-         */
-        public static GLib.HashTable<string,string>[] get_contract(string uri, string mime)
-        {
-            ensure ();
-            GLib.HashTable<string,string>[] contracts = null;
-
-            try {
-                contracts = contractor.contract.GetServicesByLocation(uri, mime);
-            }catch (IOError e) {
-                stderr.printf ("%s\n", e.message);
-            }
-
-            return contracts;
-        }
-
-        /**
-         * generate contracts for rguments and filter them by  common parent mimetype.
-         * 
-         * @param locations Hashtable of locations
-         * @return Hashtable of available contracts
-         */
-        public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations)
-        {
-            ensure ();
-            GLib.HashTable<string,string>[] contracts = null;
-
-            try {
-                contracts = contractor.contract.GetServicesByLocationsList (locations);
-            }catch (IOError e) {
-                stderr.printf ("%s\n", e.message);
-            }
-
-            return contracts;
+    public interface Contract : Object {
+        public abstract string get_display_name ();
+        public abstract string get_description ();
+        public abstract Icon get_icon ();
+        public abstract int execute_with_file (File file) throws Error;
+        public abstract int execute_with_files (File[] files) throws Error;
+    }
+
+    public errordomain ContractorError {
+        SERVICE_NOT_AVAILABLE
+    }
+
+    internal struct ContractData {
+        string id;
+        string display_name;
+        string description;
+        string icon;
+    }
+
+    [DBus (name = "org.elementary.Contractor")]
+    internal interface ContractorDBusAPI : Object {
+        public abstract ContractData[] list_all_contracts () throws Error;
+        public abstract ContractData[] get_contracts_by_mime (string mime_type) throws Error;
+        public abstract ContractData[] get_contracts_by_mimelist (string[] mime_types) throws Error;
+        public abstract int execute_with_uri (string id, string uri) throws Error;
+        public abstract int execute_with_uri_list (string id, string[] uri) throws Error;
+    }
+
+    public class ContractorProxy {
+        private class GenericContract : Object, Contract {
+            private string id;
+            private string display_name;
+            private string description;
+            private string icon_key;
+
+            private Icon icon;
+
+            public GenericContract (ContractData data) {
+                icon_key = "";
+                update_data (data);
+            }
+
+            public void update_data (ContractData data) {
+                id = data.id ?? "";
+                display_name = data.display_name ?? "";
+                description = data.description ?? "";
+
+                if (icon_key != data.icon) {
+                    icon_key = data.icon ?? "";
+                    icon = null;
+                }
+            }
+
+            public string get_display_name () {
+                return display_name;
+            }
+
+            public string get_description () {
+                return description;
+            }
+
+            public Icon get_icon () {
+                if (icon == null) {
+                    if (Path.is_absolute (icon_key))
+                        icon = new FileIcon (File.new_for_path (icon_key));
+                    else
+                        icon = new ThemedIcon.with_default_fallbacks (icon_key);
+                }
+
+                return icon;
+            }
+
+            public int execute_with_file (File file) throws Error {
+                return ContractorProxy.execute_with_uri (id, file.get_uri ());
+            }
+
+            public int execute_with_files (File[] files) throws Error {
+                string[] uris = new string[files.length];
+
+                foreach (var file in files)
+                    uris += file.get_uri ();
+
+                return ContractorProxy.execute_with_uri_list (id, uris);
+            }
+        }
+
+
+        private static ContractorDBusAPI contractor_dbus;
+        private static Gee.HashMap<string, GenericContract> contracts;
+
+        private ContractorProxy () { }
+
+        private static void ensure () throws Error {
+            if (contractor_dbus == null) {
+                try {
+                    contractor_dbus = Bus.get_proxy_sync (BusType.SESSION,
+                                                          "org.elementary.Contractor",
+                                                          "/org/elementary/contractor");
+                } catch (IOError e) {
+                    throw new ContractorError.SERVICE_NOT_AVAILABLE (e.message);
+                }
+            }
+
+            if (contracts == null)
+                contracts = new Gee.HashMap<string, GenericContract> ();
+        }
+
+        private static int execute_with_uri (string id, string uri) throws Error {
+            ensure ();
+            return contractor_dbus.execute_with_uri (id, uri);
+        }
+
+        private static int execute_with_uri_list (string id, string[] uris) throws Error {
+            ensure ();
+            return contractor_dbus.execute_with_uri_list (id, uris);
+        }
+
+        /**
+         * Provides all the contracts.
+         *
+         * @return List containing all the contracts available in the system.
+         */
+        public static Gee.List<Contract> get_all_contracts () throws Error {
+            ensure ();
+
+            var data = contractor_dbus.list_all_contracts ();
+
+            return get_contracts_from_data (data);
+        }
+
+        /**
+         * This searches for available contracts of a particular file type.
+         *
+         * @param mime_type Mimetype of file.
+         * @return List of contracts that support the given mimetype.
+         */
+        public static Gee.List<Contract> get_contracts_by_mime (string mime_type) throws Error {
+            ensure ();
+
+            var data = contractor_dbus.get_contracts_by_mime (mime_type);
+
+            return get_contracts_from_data (data);
+        }
+
+        /**
+         * Generate contracts for a list of mimetypes.
+         *
+         * Only the contracts that support all the mimetypes are returned.
+         *
+         * @param mime_types Array of mimetypes.
+         * @return List of contracts that support the given mimetypes.
+         */
+        public static Gee.List<Contract> get_contracts_by_mimelist (string[] mime_types) throws Error {
+            ensure ();
+
+            var data = contractor_dbus.get_contracts_by_mimelist (mime_types);
+
+            return get_contracts_from_data (data);
+        }
+
+        private static Gee.List<Contract> get_contracts_from_data (ContractData[] data) {
+            var contract_list = new Gee.LinkedList<Contract> ();
+
+            if (data != null) {
+                foreach (var contract_data in data) {
+                    string contract_id = contract_data.id;
+
+                    // See if we have a contract already. Otherwise create a new one.
+                    // We do this in order to be able to compare contracts by reference
+                    // from client code.
+                    var contract = contracts.get (contract_id);
+
+                    if (contract == null) {
+                        contract = new GenericContract (contract_data);
+                        contracts.set (contract_id, contract);
+                    } else {
+                        contract.update_data (contract_data);
+                    }
+
+                    contract_list.add (contract);
+                }
+            }
+
+            return contract_list;
         }
     }
 }
 
=== modified file 'lib/Widgets/ContractorMenu.vala'
--- lib/Widgets/ContractorMenu.vala	2013-03-20 15:42:25 +0000
+++ lib/Widgets/ContractorMenu.vala	2013-04-24 17:07:27 +0000
@@ -20,6 +20,7 @@
 /**
  * This class provides a simple menu for managing Contractor.
  */
+[Deprecated (since = "0.2")]
 public class Granite.Widgets.ContractorMenu : Gtk.Menu {
     /**
      * The Hashtable of available contracts
 
=== modified file 'lib/Widgets/ContractorView.vala'
--- lib/Widgets/ContractorView.vala	2013-03-20 15:42:25 +0000
+++ lib/Widgets/ContractorView.vala	2013-04-24 17:07:27 +0000
@@ -22,6 +22,7 @@
 /**
  * This class provides a simple way to look at contracts from Contractor
  */
+[Deprecated (since = "0.2")]
 public class Granite.Widgets.ContractorView : TreeView {
     
     /**