Merge lp:~voldyman/granite/contractor-wid-dep-new-Contractor into lp:~elementary-pantheon/granite/granite

[Sergey "Shnatsel" Davidoff]

* Added Granite.Services.Contractor methods that work with the new Contractor API
* Deprecated the widgets that work with the old and abandoned API instead of including updated widgets, because Dan doesn't want any Contractor widgets in Granite

This merge does not cause an API/ABI break.

[Sergey "Shnatsel" Davidoff (shnatsel)]

This seems to break the ABI (according to Rico's check in deb-packaging) even thought it shouldn't.

review: Needs Fixing

[lp:~voldyman/granite/contractor-wid-dep-new-Contractor]

557. By Akshay Shekher on 2013-04-10

Deprecated ContractorView and ContractorMenu. Added support for the new contractor API in services/Contractor.vala

558. By Akshay Shekher on 2013-04-10

Change the constructor of Contractor.vala to private

[Victor Martinez (victored)]

Wow, fantastic work here!

Since overall this is very cohesive and well done, I'll just focus on a couple of details to make it even better IMHO:

(1) Why isn't ensure() private?
(2) Make sure the interface ContractorDBus is internal to Granite.
(3) The names of execute_* () suggest URIs, but in the parameters they are named paths. What do these really expect?

(1) and (2) are easily fixable.

Regarding (3), since this is a wrapper to the DBus API, I could suggest receiving GLib.File objects instead, and calling get_path() or get_uri() on them depending on what Contractor is expecting. Make sure you rename the methods accordingly (..._with_files).

Regarding the API, it would be nice to have something like:
    Granite.Services.Contract contract;
    File file;
    // get some instances from somewhere...
    contract.execute_with_file (file);

Instead of:
    string contract_id;
    string file_uri;
    // get those strings from somewhere...
    Granite.Services.Contractor.execute_with_uri (contract_id, uri);

We'd have something like:

public interface Granite.Services.Contract : Object {
    public abstract void execute_with_file (File file);
    public abstract void execute_with_files (File[] files);
}

public class Granite.Services.Contractor : Object {
    private struct GenericContract { ... }

    // Private implementation of Contract
    private class ContractorContract : Object, Contract {
        private GenericContract data;
        private Con

        public ContractorContract (GenericContract data) {
            this.data = data;
        }

        public void execute_with_file (File file) {
            // Yes, we have access to the parent class's private static methods
            execute_with_uri (data.id, file.get_uri ());
        }

        [...]
    }

    [...]

    // Of course, this method could be removed and its contents may be moved to
    // Contract.execute_with_file() directly.
    private static void execute_with_uri (string id, string uri);

    [...]
}

Finally, please fix the coding style issues. A quick read through the code will show what I mean. Of course there are only a couple of them :)

[lp:~voldyman/granite/contractor-wid-dep-new-Contractor]

559. By Akshay Shekher on 2013-04-13

Changed GenericContract to ContractorContract and renamed arguments

560. By Akshay Shekher on 2013-04-13

fixed coding style

561. By Akshay Shekher on 2013-04-18

More OO contractor interface

[Victor Martinez (victored)]

Thank you for your work Akshay!

Actually, my suggestion was not related to hiding contractor behind an interface, since it's only a facade. What I was suggesting was abstracting the Contracts themselves, so that no implementation details are made available to the client code.

Please take a look to rev. 562 of lp:~victored/granite/contractor-wrapper

[Victor Martinez (victored)]

For an overview of the API, you can go here: http://pastebin.ubuntu.com/5719759/ :)

[Sergey "Shnatsel" Davidoff (shnatsel)]

Looks good to me (but disregard my opinion because I'm OOP-impaired). In hindsight, using Glib.File instead of messing with paths and URIs is a good idea.

[Sergey "Shnatsel" Davidoff (shnatsel)]

review: Approve

[Sergey "Shnatsel" Davidoff (shnatsel)]

Argh, Victor's updated branch is not yet merged into this one... but I don't have anything left to review here anyway, so "Approved" from me it is.

=== modified file 'demo/main.vala'
--- demo/main.vala	2013-03-20 15:42:25 +0000
+++ demo/main.vala	2013-04-18 12:53:23 +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/Services/Contractor.vala'
--- lib/Services/Contractor.vala	2013-04-04 20:05:38 +0000
+++ lib/Services/Contractor.vala	2013-04-18 12:53:23 +0000
@@ -10,7 +10,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,
@@ -19,86 +19,251 @@
 
 namespace Granite.Services {
 
-    [DBus (name = "org.elementary.contractor")]
-    interface ContractorDBus : Object
-    {
+    public struct ContractorContract {
+        string id;
+        string display_name;
+        string description;
+        string icon_path;
+
+        public int execute_uri (string uri) {
+            return Contractor.execute_with_uri (this.id, uri);
+        }
+        public int execute_uris (string[] uris) {
+            return Contractor.execute_with_uri_list (this.id, uris);
+        }
+    }
+
+    [DBus (name = "org.elementary.Contractor")]
+    internal interface ContractorDBus : Object {
+        [Deprecated]
         public abstract GLib.HashTable<string,string>[] GetServicesByLocation (string strlocation, string? file_mime="")    throws IOError;
+        [Deprecated]
         public abstract GLib.HashTable<string,string>[] GetServicesByLocationsList (GLib.HashTable<string,string>[] locations)  throws IOError;
+        public abstract ContractorContract[] list_all_contracts () throws Error;
+        public abstract ContractorContract[] get_contracts_by_mime (string mime_type) throws Error;
+        public abstract ContractorContract[] 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;
+    }
+
+    /*
+     * interface that the contractor class must implement
+     */
+    public interface ContractorIface {
+        public abstract ContractorContract[] get_contracts () throws ContractError;
+        public abstract ContractorContract get_for_id (string id) throws ContractError;
     }
 
     /**
      * A way to handle contractor, a way to communicate between apps.
-     * 
-     * /!\ Highly unstable API
+     *
      */
-    public class Contractor : Object
-    {
+    public class Contractor : Object, ContractorIface {
+
+        private string? mime_type;
+        private string[]? mime_types;
+        private ContractorContract[]? contracts;
+
 
         internal ContractorDBus contract;
 
         internal static Contractor? contractor = null;
 
         /**
-         * This creates a new Contractor 
+         * This creates a new Contractor
          */
-        public Contractor()
-        {
-            try
-            {
+        private Contractor () {
+            try {
                 contract = Bus.get_proxy_sync (BusType.SESSION,
-                                               "org.elementary.contractor",
+                                               "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
-         * 
+            } catch (IOError e) {
+                stderr.printf ("%s\n", e.message);
+            }
+        }
+
+        internal static void ensure () {
+            if (contractor == null) {
+                contractor = new Contractor ();
+            }
+        }
+
+        /*
+         * Class constructor for single mime type
+         */
+        public Contractor.for_mime (string mime) {
+            this.mime_type = mime;
+            this.mime_types = null;
+        }
+
+        /*
+         * Class constructor for multiple mime types
+         */
+        public Contractor.for_mime_list (string[] mimes) {
+            this.mime_types = mimes;
+            this.mime_type = null;
+        }
+
+        /*
+         * searches for contracts that support the specified mime type(s)
+         * throws error if mime type is not set.
+         */
+        public ContractorContract[] get_contracts () throws ContractError {
+            if (this.mime_type != null) {
+                this.contracts = get_contracts_by_mime (this.mime_type);
+                return contracts;
+            } else if (this.mime_types != null) {
+                contracts = get_contracts_by_mimelist (this.mime_types);
+                return this.contracts;
+            }
+            throw new ContractError.OBJECT_ERROR ("data members are null");
+        }
+
+        /*
+         * Searches for the contract with with the id as specified
+         * if not found, throws NOT_FOUND error
+         */
+        public ContractorContract get_for_id (string id) throws ContractError {
+            if (this.contracts == null) {
+                throw new ContractError.NOT_FOUND ("contracts were not found");
+            }
+            foreach (var cont in this.contracts) {
+                if (cont.id == id)
+                    return cont;
+            }
+            throw new ContractError.NOT_FOUND ("Contract of this ID was not found");
+        }
+
+        /**
+         * Lists all the contracts
+         *
+         * @return an array of struct ContractorContract
+         */
+        public static ContractorContract[] list_all_contracts () {
+            ensure ();
+            ContractorContract[] contracts = null;
+
+            try {
+                contracts = contractor.contract.list_all_contracts ();
+            } catch (Error e) {
+                stderr.printf ("%s\n", e.message);
+            }
+
+            return contracts;
+        }
+
+        /**
+         * This searches for available contracts of a particular file
+         *
+         * @param mime mime type of file
+         * @return an array of struct ContractorContract
+         */
+        public static ContractorContract[] get_contracts_by_mime (string mime_type) {
+            ensure ();
+            ContractorContract[] contracts = null;
+
+            try {
+                contracts = contractor.contract.get_contracts_by_mime (mime_type);
+            } catch (Error e) {
+                stderr.printf ("%s\n", e.message);
+            }
+
+            return contracts;
+        }
+
+        /**
+         * generate contracts for a list of mime types. the contracts which support
+         * all of them are returned
+         *
+         * @param locations Array of MimeTypes
+         * @return array of struct (ContractorContract)
+         */
+        public static ContractorContract[] get_contracts_by_mimelist (string[] mime_types) {
+            ensure ();
+            ContractorContract[] contracts = null;
+
+            try {
+                contracts = contractor.contract.get_contracts_by_mimelist (mime_types);
+            } catch (Error e) {
+                stderr.printf ("%s\n", e.message);
+            }
+
+            return contracts;
+        }
+
+        /**
+         * This executes the exec parameter provided by the contract of given id
+         * with the uri as arguments
+         *
+         * @param id id of the contract
+         * @param uris uri to execute
+         * @return int status of execution
+         */
+        public static int execute_with_uri (string id, string uri) {
+            ensure ();
+            int ret_val = 1;
+
+            try {
+                ret_val = contractor.contract.execute_with_uri (id, uri);
+            } catch (Error e) {
+                stderr.printf ("%s\n", e.message);
+            }
+            return ret_val;
+        }
+
+        /**
+         * This executes the exec parameter provided by the contract of given id
+         * with the uris as arguments
+         *
+         * @param id id of the contract
+         * @param uris array of uris to execute
+         * @return int status of execution
+         */
+        public static int execute_with_uri_list (string id, string[] uris) {
+            ensure ();
+            int ret_val = 1;
+
+            try {
+                ret_val = contractor.contract.execute_with_uri_list (id, uris);
+            } catch (Error e) {
+                stderr.printf ("%s\n", e.message);
+            }
+            return ret_val;
+        }
+
+        /**
+         * The functions below have been deprecated in order to support the
+         * the new contractor API.
+         */
+
+        /**
+         * 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;
+        [Deprecated]
+        public static GLib.HashTable<string,string>[] get_contract(string uri, string mime) {
+
+            return { new GLib.HashTable<string,string> (null, null) };
         }
 
         /**
          * 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;
+        [Deprecated]
+        public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations) {
+
+            return { new GLib.HashTable<string,string> (null, null) };
         }
     }
+
+    public errordomain ContractError {
+        OBJECT_ERROR,
+        NOT_FOUND
+    }
 }
 
=== modified file 'lib/Widgets/ContractorMenu.vala'
--- lib/Widgets/ContractorMenu.vala	2013-03-20 15:42:25 +0000
+++ lib/Widgets/ContractorMenu.vala	2013-04-18 12:53:23 +0000
@@ -20,6 +20,7 @@
 /**
  * This class provides a simple menu for managing Contractor.
  */
+[Deprecated]
 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-18 12:53:23 +0000
@@ -22,6 +22,7 @@
 /**
  * This class provides a simple way to look at contracts from Contractor
  */
+[Deprecated]
 public class Granite.Widgets.ContractorView : TreeView {
     
     /**