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

Proposed by Sergey "Shnatsel" Davidoff on 2013-04-10
Status: Merged
Merged at revision: 559
Proposed branch: lp:~voldyman/granite/contractor-wid-dep-new-Contractor
Merge into: lp:~elementary-pantheon/granite/granite
Diff against target: 448 lines (+232/-91)
4 files modified
demo/main.vala (+11/-37)
lib/Services/Contractor.vala (+219/-54)
lib/Widgets/ContractorMenu.vala (+1/-0)
lib/Widgets/ContractorView.vala (+1/-0)
To merge this branch: bzr merge lp:~voldyman/granite/contractor-wid-dep-new-Contractor
Reviewer Review Type Date Requested Status
Sergey "Shnatsel" Davidoff (community) Approve on 2013-04-18
elementary Apps team 2013-04-10 Pending
Review via email: mp+158161@code.launchpad.net

This proposal supersedes a proposal from 2013-04-10.

Description of the change

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

To post a comment you must log in.
Sergey "Shnatsel" Davidoff (shnatsel) wrote : Posted in a previous version of this proposal

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

review: Needs Fixing
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) wrote :

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

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) wrote :

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) wrote :

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

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.

review: Approve

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'demo/main.vala'
2--- demo/main.vala 2013-03-20 15:42:25 +0000
3+++ demo/main.vala 2013-04-18 12:53:23 +0000
4@@ -13,7 +13,7 @@
5 but WITHOUT ANY WARRANTY; without even the implied warranty of
6 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
7 Lesser General Public License for more details.
8-
9+
10 You should have received a copy of the GNU Lesser General
11 Public License along with this library; if not, write to the
12 Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
13@@ -276,32 +276,6 @@
14 right_sep.set_expand (true);
15 main_toolbar.insert (right_sep, -1);
16
17- // Contractor
18- var contractor_tab = new Gtk.Box (Gtk.Orientation.VERTICAL, 0);
19- var text_view = new Gtk.TextView ();
20-
21- var hash_tables = Granite.Services.Contractor.get_contract ("/.zip", "application/zip");
22- foreach (var hash_table in hash_tables) {
23- text_view.buffer.text += hash_table.lookup ("Name")
24- + ": " + hash_table.lookup ("Description")
25- + " icon: " + hash_table.lookup ("Exec") + "\n";
26- }
27-
28- contractor_tab.add (text_view);
29- contractor_tab.add (new Granite.Widgets.ContractorView ("file:///home/user/file.txt", "text/plain"));
30- var contractor_menu = new Granite.Widgets.ContractorMenu ("/home/user/file.txt", "text");
31- var contractor_button_image = new Gtk.Image.from_icon_name ("document-export",
32- Gtk.IconSize.LARGE_TOOLBAR);
33- var contractor_tool_item = new Granite.Widgets.ToolButtonWithMenu (contractor_button_image,
34- "Share", contractor_menu);
35- main_toolbar.insert (contractor_tool_item, -1);
36-
37- contractor_tool_item.halign = contractor_tool_item.valign = Gtk.Align.CENTER;
38-
39- var contractor_item = new SourceListItem ("Contractor");
40- contractor_item.page_num = page_switcher.append_page (contractor_tab, null);
41- services_category.add (contractor_item);
42-
43 // Search Entry
44 var search_entry = new Granite.Widgets.SearchBar ("Search");
45 var search_item = new Gtk.ToolItem ();
46@@ -343,42 +317,42 @@
47
48 private void show_light_window () {
49 var light_window = new Granite.Widgets.LightWindow ();
50-
51+
52 var light_window_notebook = new Granite.Widgets.StaticNotebook ();
53 var entry = new Gtk.Entry ();
54 var open_drop = new Gtk.ComboBoxText ();
55 var open_lbl = new LLabel ("Alwas Open Mpeg Video Files with Audience");
56-
57+
58 var grid = new Gtk.Grid ();
59 grid.attach (new Gtk.Image.from_icon_name ("video-x-generic", Gtk.IconSize.DIALOG), 0, 0, 1, 2);
60 grid.attach (entry, 1, 0, 1, 1);
61 grid.attach (new LLabel ("1.13 GB, Mpeg Video File"), 1, 1, 1, 1);
62-
63+
64 grid.attach (light_window_notebook, 0, 2, 2, 1);
65-
66+
67 var general = new Gtk.Grid ();
68 general.attach (new LLabel.markup ("<b>Info:</b>"), 0, 0, 2, 1);
69-
70+
71 general.attach (new LLabel.right ("Created:"), 0, 1, 1, 1);
72 general.attach (new LLabel.right ("Modified:"), 0, 2, 1, 1);
73 general.attach (new LLabel.right ("Opened:"), 0, 3, 1, 1);
74 general.attach (new LLabel.right ("Mimetype:"), 0, 4, 1, 1);
75 general.attach (new LLabel.right ("Location:"), 0, 5, 1, 1);
76-
77+
78 general.attach (new LLabel ("Today at 9:50 PM"), 1, 1, 1, 1);
79 general.attach (new LLabel ("Today at 9:50 PM"), 1, 2, 1, 1);
80 general.attach (new LLabel ("Today at 10:00 PM"), 1, 3, 1, 1);
81 general.attach (new LLabel ("video/mpeg"), 1, 4, 1, 1);
82 general.attach (new LLabel ("/home/daniel/Downloads"), 1, 5, 1, 1);
83-
84+
85 general.attach (new LLabel.markup ("<b>Open with:</b>"), 0, 6, 2, 1);
86 general.attach (open_drop, 0, 7, 2, 1);
87 general.attach (open_lbl, 0, 8, 2, 1);
88-
89+
90 light_window_notebook.append_page (general, new Gtk.Label ("General"));
91 light_window_notebook.append_page (new Gtk.Label ("More"), new Gtk.Label ("More"));
92 light_window_notebook.append_page (new Gtk.Label ("Sharing"), new Gtk.Label ("Sharing"));
93-
94+
95 open_lbl.margin_left = 24;
96 open_drop.margin_left = 12;
97 open_drop.append ("audience", "Audience");
98@@ -389,7 +363,7 @@
99 entry.text = "Cool Hand Luke";
100 general.column_spacing = 6;
101 general.row_spacing = 6;
102-
103+
104 light_window.add (grid);
105 light_window.show_all ();
106 }
107
108=== modified file 'lib/Services/Contractor.vala'
109--- lib/Services/Contractor.vala 2013-04-04 20:05:38 +0000
110+++ lib/Services/Contractor.vala 2013-04-18 12:53:23 +0000
111@@ -10,7 +10,7 @@
112 but WITHOUT ANY WARRANTY; without even the implied warranty of
113 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
114 Lesser General Public License for more details.
115-
116+
117 You should have received a copy of the GNU Lesser General
118 Public License along with this library; if not, write to the
119 Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
120@@ -19,86 +19,251 @@
121
122 namespace Granite.Services {
123
124- [DBus (name = "org.elementary.contractor")]
125- interface ContractorDBus : Object
126- {
127+ public struct ContractorContract {
128+ string id;
129+ string display_name;
130+ string description;
131+ string icon_path;
132+
133+ public int execute_uri (string uri) {
134+ return Contractor.execute_with_uri (this.id, uri);
135+ }
136+ public int execute_uris (string[] uris) {
137+ return Contractor.execute_with_uri_list (this.id, uris);
138+ }
139+ }
140+
141+ [DBus (name = "org.elementary.Contractor")]
142+ internal interface ContractorDBus : Object {
143+ [Deprecated]
144 public abstract GLib.HashTable<string,string>[] GetServicesByLocation (string strlocation, string? file_mime="") throws IOError;
145+ [Deprecated]
146 public abstract GLib.HashTable<string,string>[] GetServicesByLocationsList (GLib.HashTable<string,string>[] locations) throws IOError;
147+ public abstract ContractorContract[] list_all_contracts () throws Error;
148+ public abstract ContractorContract[] get_contracts_by_mime (string mime_type) throws Error;
149+ public abstract ContractorContract[] get_contracts_by_mimelist (string[] mime_types) throws Error;
150+ public abstract int execute_with_uri (string id, string uri) throws Error;
151+ public abstract int execute_with_uri_list (string id, string[] uri) throws Error;
152+ }
153+
154+ /*
155+ * interface that the contractor class must implement
156+ */
157+ public interface ContractorIface {
158+ public abstract ContractorContract[] get_contracts () throws ContractError;
159+ public abstract ContractorContract get_for_id (string id) throws ContractError;
160 }
161
162 /**
163 * A way to handle contractor, a way to communicate between apps.
164- *
165- * /!\ Highly unstable API
166+ *
167 */
168- public class Contractor : Object
169- {
170+ public class Contractor : Object, ContractorIface {
171+
172+ private string? mime_type;
173+ private string[]? mime_types;
174+ private ContractorContract[]? contracts;
175+
176
177 internal ContractorDBus contract;
178
179 internal static Contractor? contractor = null;
180
181 /**
182- * This creates a new Contractor
183+ * This creates a new Contractor
184 */
185- public Contractor()
186- {
187- try
188- {
189+ private Contractor () {
190+ try {
191 contract = Bus.get_proxy_sync (BusType.SESSION,
192- "org.elementary.contractor",
193+ "org.elementary.Contractor",
194 "/org/elementary/contractor");
195- }
196- catch (IOError e)
197- {
198- stderr.printf ("%s\n", e.message);
199- }
200- }
201-
202- internal static void ensure ()
203- {
204- if(contractor == null) contractor = new Contractor ();
205- }
206-
207- /**
208- * This searches for available contracts of a particular file
209- *
210+ } catch (IOError e) {
211+ stderr.printf ("%s\n", e.message);
212+ }
213+ }
214+
215+ internal static void ensure () {
216+ if (contractor == null) {
217+ contractor = new Contractor ();
218+ }
219+ }
220+
221+ /*
222+ * Class constructor for single mime type
223+ */
224+ public Contractor.for_mime (string mime) {
225+ this.mime_type = mime;
226+ this.mime_types = null;
227+ }
228+
229+ /*
230+ * Class constructor for multiple mime types
231+ */
232+ public Contractor.for_mime_list (string[] mimes) {
233+ this.mime_types = mimes;
234+ this.mime_type = null;
235+ }
236+
237+ /*
238+ * searches for contracts that support the specified mime type(s)
239+ * throws error if mime type is not set.
240+ */
241+ public ContractorContract[] get_contracts () throws ContractError {
242+ if (this.mime_type != null) {
243+ this.contracts = get_contracts_by_mime (this.mime_type);
244+ return contracts;
245+ } else if (this.mime_types != null) {
246+ contracts = get_contracts_by_mimelist (this.mime_types);
247+ return this.contracts;
248+ }
249+ throw new ContractError.OBJECT_ERROR ("data members are null");
250+ }
251+
252+ /*
253+ * Searches for the contract with with the id as specified
254+ * if not found, throws NOT_FOUND error
255+ */
256+ public ContractorContract get_for_id (string id) throws ContractError {
257+ if (this.contracts == null) {
258+ throw new ContractError.NOT_FOUND ("contracts were not found");
259+ }
260+ foreach (var cont in this.contracts) {
261+ if (cont.id == id)
262+ return cont;
263+ }
264+ throw new ContractError.NOT_FOUND ("Contract of this ID was not found");
265+ }
266+
267+ /**
268+ * Lists all the contracts
269+ *
270+ * @return an array of struct ContractorContract
271+ */
272+ public static ContractorContract[] list_all_contracts () {
273+ ensure ();
274+ ContractorContract[] contracts = null;
275+
276+ try {
277+ contracts = contractor.contract.list_all_contracts ();
278+ } catch (Error e) {
279+ stderr.printf ("%s\n", e.message);
280+ }
281+
282+ return contracts;
283+ }
284+
285+ /**
286+ * This searches for available contracts of a particular file
287+ *
288+ * @param mime mime type of file
289+ * @return an array of struct ContractorContract
290+ */
291+ public static ContractorContract[] get_contracts_by_mime (string mime_type) {
292+ ensure ();
293+ ContractorContract[] contracts = null;
294+
295+ try {
296+ contracts = contractor.contract.get_contracts_by_mime (mime_type);
297+ } catch (Error e) {
298+ stderr.printf ("%s\n", e.message);
299+ }
300+
301+ return contracts;
302+ }
303+
304+ /**
305+ * generate contracts for a list of mime types. the contracts which support
306+ * all of them are returned
307+ *
308+ * @param locations Array of MimeTypes
309+ * @return array of struct (ContractorContract)
310+ */
311+ public static ContractorContract[] get_contracts_by_mimelist (string[] mime_types) {
312+ ensure ();
313+ ContractorContract[] contracts = null;
314+
315+ try {
316+ contracts = contractor.contract.get_contracts_by_mimelist (mime_types);
317+ } catch (Error e) {
318+ stderr.printf ("%s\n", e.message);
319+ }
320+
321+ return contracts;
322+ }
323+
324+ /**
325+ * This executes the exec parameter provided by the contract of given id
326+ * with the uri as arguments
327+ *
328+ * @param id id of the contract
329+ * @param uris uri to execute
330+ * @return int status of execution
331+ */
332+ public static int execute_with_uri (string id, string uri) {
333+ ensure ();
334+ int ret_val = 1;
335+
336+ try {
337+ ret_val = contractor.contract.execute_with_uri (id, uri);
338+ } catch (Error e) {
339+ stderr.printf ("%s\n", e.message);
340+ }
341+ return ret_val;
342+ }
343+
344+ /**
345+ * This executes the exec parameter provided by the contract of given id
346+ * with the uris as arguments
347+ *
348+ * @param id id of the contract
349+ * @param uris array of uris to execute
350+ * @return int status of execution
351+ */
352+ public static int execute_with_uri_list (string id, string[] uris) {
353+ ensure ();
354+ int ret_val = 1;
355+
356+ try {
357+ ret_val = contractor.contract.execute_with_uri_list (id, uris);
358+ } catch (Error e) {
359+ stderr.printf ("%s\n", e.message);
360+ }
361+ return ret_val;
362+ }
363+
364+ /**
365+ * The functions below have been deprecated in order to support the
366+ * the new contractor API.
367+ */
368+
369+ /**
370+ * This searches for available contracts of a particular file
371+ *
372 * @param uri uri of file
373 * @param mime mime type of file
374 * @return Hashtable of available contracts
375 */
376- public static GLib.HashTable<string,string>[] get_contract(string uri, string mime)
377- {
378- ensure ();
379- GLib.HashTable<string,string>[] contracts = null;
380-
381- try {
382- contracts = contractor.contract.GetServicesByLocation(uri, mime);
383- }catch (IOError e) {
384- stderr.printf ("%s\n", e.message);
385- }
386-
387- return contracts;
388+ [Deprecated]
389+ public static GLib.HashTable<string,string>[] get_contract(string uri, string mime) {
390+
391+ return { new GLib.HashTable<string,string> (null, null) };
392 }
393
394 /**
395 * generate contracts for rguments and filter them by common parent mimetype.
396- *
397+ *
398 * @param locations Hashtable of locations
399 * @return Hashtable of available contracts
400 */
401- public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations)
402- {
403- ensure ();
404- GLib.HashTable<string,string>[] contracts = null;
405-
406- try {
407- contracts = contractor.contract.GetServicesByLocationsList (locations);
408- }catch (IOError e) {
409- stderr.printf ("%s\n", e.message);
410- }
411-
412- return contracts;
413+ [Deprecated]
414+ public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations) {
415+
416+ return { new GLib.HashTable<string,string> (null, null) };
417 }
418 }
419+
420+ public errordomain ContractError {
421+ OBJECT_ERROR,
422+ NOT_FOUND
423+ }
424 }
425
426=== modified file 'lib/Widgets/ContractorMenu.vala'
427--- lib/Widgets/ContractorMenu.vala 2013-03-20 15:42:25 +0000
428+++ lib/Widgets/ContractorMenu.vala 2013-04-18 12:53:23 +0000
429@@ -20,6 +20,7 @@
430 /**
431 * This class provides a simple menu for managing Contractor.
432 */
433+[Deprecated]
434 public class Granite.Widgets.ContractorMenu : Gtk.Menu {
435 /**
436 * The Hashtable of available contracts
437
438=== modified file 'lib/Widgets/ContractorView.vala'
439--- lib/Widgets/ContractorView.vala 2013-03-20 15:42:25 +0000
440+++ lib/Widgets/ContractorView.vala 2013-04-18 12:53:23 +0000
441@@ -22,6 +22,7 @@
442 /**
443 * This class provides a simple way to look at contracts from Contractor
444 */
445+[Deprecated]
446 public class Granite.Widgets.ContractorView : TreeView {
447
448 /**

Subscribers

People subscribed via source and target branches