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

Proposed by Victor Martinez on 2013-04-20
Status: Merged
Merged at revision: 559
Proposed branch: lp:~victored/granite/contractor-wrapper
Merge into: lp:~elementary-pantheon/granite/granite
Diff against target: 508 lines (+269/-120)
6 files modified
demo/main.vala (+11/-37)
lib/CMakeLists.txt (+1/-0)
lib/Services/Contractor.vala (+67/-0)
lib/Services/ContractorProxy.vala (+188/-83)
lib/Widgets/ContractorMenu.vala (+1/-0)
lib/Widgets/ContractorView.vala (+1/-0)
To merge this branch: bzr merge lp:~victored/granite/contractor-wrapper
Reviewer Review Type Date Requested Status
Sergey "Shnatsel" Davidoff (community) 2013-04-20 Approve on 2013-04-24
Victor Martinez 2013-04-21 Approve on 2013-04-23
Review via email: mp+159948@code.launchpad.net

Description of the change

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.

To post a comment you must log in.

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

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

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

Looks like this branch breaks ABI.

Victor Martinez (victored) wrote :

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

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

Please list/mention the current consumers of the contractor api

Victor Martinez (victored) wrote :

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.

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

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

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

Victor Martinez (victored) wrote :

The current merge stopper is bug #1171690 IMO.

review: Needs Fixing

Bug #1171690 debunked.

Victor Martinez (victored) :
review: Approve

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

review: Approve
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

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

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

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

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

Works fine for me now. Thanks!

review: Approve

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-24 17:07:27 +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/CMakeLists.txt'
109--- lib/CMakeLists.txt 2012-12-21 00:46:56 +0000
110+++ lib/CMakeLists.txt 2013-04-24 17:07:27 +0000
111@@ -16,6 +16,7 @@
112 Services/Paths.vala
113 Services/System.vala
114 Services/Contractor.vala
115+ Services/ContractorProxy.vala
116 Services/IconFactory.vala
117 Services/SimpleCommand.vala
118 Widgets/Utils.vala
119
120=== added file 'lib/Services/Contractor.vala'
121--- lib/Services/Contractor.vala 1970-01-01 00:00:00 +0000
122+++ lib/Services/Contractor.vala 2013-04-24 17:07:27 +0000
123@@ -0,0 +1,67 @@
124+/***
125+ Copyright (C) 2011-2013 Lucas Baudin <xapantu@gmail.com>
126+
127+ This program or library is free software; you can redistribute it
128+ and/or modify it under the terms of the GNU Lesser General Public
129+ License as published by the Free Software Foundation; either
130+ version 3 of the License, or (at your option) any later version.
131+
132+ This library is distributed in the hope that it will be useful,
133+ but WITHOUT ANY WARRANTY; without even the implied warranty of
134+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
135+ Lesser General Public License for more details.
136+
137+ You should have received a copy of the GNU Lesser General
138+ Public License along with this library; if not, write to the
139+ Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
140+ Boston, MA 02110-1301 USA.
141+***/
142+
143+namespace Granite.Services {
144+ [DBus (name = "org.elementary.Contractor")]
145+ interface ContractorDBus : Object {
146+ public abstract GLib.HashTable<string,string>[] GetServicesByLocation (string strlocation, string? file_mime = "") throws IOError;
147+ public abstract GLib.HashTable<string,string>[] GetServicesByLocationsList (GLib.HashTable<string,string>[] locations) throws IOError;
148+ }
149+
150+ /**
151+ * A way to handle contractor, a way to communicate between apps.
152+ *
153+ * /!\ Highly unstable API
154+ */
155+ [Deprecated (replacement = "Granite.Services.ContractorProxy", since = "0.2")]
156+ public class Contractor : Object {
157+ internal ContractorDBus contract;
158+ internal static Contractor? contractor = null;
159+
160+ /**
161+ * This creates a new Contractor
162+ */
163+ public Contractor () {
164+ }
165+
166+ internal static void ensure () {
167+ }
168+
169+ /**
170+ * This searches for available contracts of a particular file
171+ *
172+ * @param uri uri of file
173+ * @param mime mime type of file
174+ * @return Hashtable of available contracts
175+ */
176+ public static GLib.HashTable<string,string>[] get_contract (string uri, string mime) {
177+ return { new GLib.HashTable<string,string> (null, null) };
178+ }
179+
180+ /**
181+ * generate contracts for arguments and filter them by common parent mimetype.
182+ *
183+ * @param locations Hashtable of locations
184+ * @return Hashtable of available contracts
185+ */
186+ public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations) {
187+ return { new GLib.HashTable<string,string> (null, null) };
188+ }
189+ }
190+}
191
192=== renamed file 'lib/Services/Contractor.vala' => 'lib/Services/ContractorProxy.vala'
193--- lib/Services/Contractor.vala 2013-04-04 20:05:38 +0000
194+++ lib/Services/ContractorProxy.vala 2013-04-24 17:07:27 +0000
195@@ -1,5 +1,7 @@
196 /***
197- Copyright (C) 2011-2013 Lucas Baudin <xapantu@gmail.com>
198+ Copyright (C) 2011-2013 Lucas Baudin <xapantu@gmail.com>,
199+ Akshay Shekher <voldyman666@gmail.com>,
200+ Victor Martinez <victoreduardm@gmail.com>
201
202 This program or library is free software; you can redistribute it
203 and/or modify it under the terms of the GNU Lesser General Public
204@@ -10,7 +12,7 @@
205 but WITHOUT ANY WARRANTY; without even the implied warranty of
206 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
207 Lesser General Public License for more details.
208-
209+
210 You should have received a copy of the GNU Lesser General
211 Public License along with this library; if not, write to the
212 Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
213@@ -18,87 +20,190 @@
214 ***/
215
216 namespace Granite.Services {
217-
218- [DBus (name = "org.elementary.contractor")]
219- interface ContractorDBus : Object
220- {
221- public abstract GLib.HashTable<string,string>[] GetServicesByLocation (string strlocation, string? file_mime="") throws IOError;
222- public abstract GLib.HashTable<string,string>[] GetServicesByLocationsList (GLib.HashTable<string,string>[] locations) throws IOError;
223- }
224-
225- /**
226- * A way to handle contractor, a way to communicate between apps.
227- *
228- * /!\ Highly unstable API
229- */
230- public class Contractor : Object
231- {
232-
233- internal ContractorDBus contract;
234-
235- internal static Contractor? contractor = null;
236-
237- /**
238- * This creates a new Contractor
239- */
240- public Contractor()
241- {
242- try
243- {
244- contract = Bus.get_proxy_sync (BusType.SESSION,
245- "org.elementary.contractor",
246- "/org/elementary/contractor");
247- }
248- catch (IOError e)
249- {
250- stderr.printf ("%s\n", e.message);
251- }
252- }
253-
254- internal static void ensure ()
255- {
256- if(contractor == null) contractor = new Contractor ();
257- }
258-
259- /**
260- * This searches for available contracts of a particular file
261- *
262- * @param uri uri of file
263- * @param mime mime type of file
264- * @return Hashtable of available contracts
265- */
266- public static GLib.HashTable<string,string>[] get_contract(string uri, string mime)
267- {
268- ensure ();
269- GLib.HashTable<string,string>[] contracts = null;
270-
271- try {
272- contracts = contractor.contract.GetServicesByLocation(uri, mime);
273- }catch (IOError e) {
274- stderr.printf ("%s\n", e.message);
275- }
276-
277- return contracts;
278- }
279-
280- /**
281- * generate contracts for rguments and filter them by common parent mimetype.
282- *
283- * @param locations Hashtable of locations
284- * @return Hashtable of available contracts
285- */
286- public static GLib.HashTable<string,string>[] get_selection_contracts (GLib.HashTable<string, string>[] locations)
287- {
288- ensure ();
289- GLib.HashTable<string,string>[] contracts = null;
290-
291- try {
292- contracts = contractor.contract.GetServicesByLocationsList (locations);
293- }catch (IOError e) {
294- stderr.printf ("%s\n", e.message);
295- }
296-
297- return contracts;
298+ public interface Contract : Object {
299+ public abstract string get_display_name ();
300+ public abstract string get_description ();
301+ public abstract Icon get_icon ();
302+ public abstract int execute_with_file (File file) throws Error;
303+ public abstract int execute_with_files (File[] files) throws Error;
304+ }
305+
306+ public errordomain ContractorError {
307+ SERVICE_NOT_AVAILABLE
308+ }
309+
310+ internal struct ContractData {
311+ string id;
312+ string display_name;
313+ string description;
314+ string icon;
315+ }
316+
317+ [DBus (name = "org.elementary.Contractor")]
318+ internal interface ContractorDBusAPI : Object {
319+ public abstract ContractData[] list_all_contracts () throws Error;
320+ public abstract ContractData[] get_contracts_by_mime (string mime_type) throws Error;
321+ public abstract ContractData[] get_contracts_by_mimelist (string[] mime_types) throws Error;
322+ public abstract int execute_with_uri (string id, string uri) throws Error;
323+ public abstract int execute_with_uri_list (string id, string[] uri) throws Error;
324+ }
325+
326+ public class ContractorProxy {
327+ private class GenericContract : Object, Contract {
328+ private string id;
329+ private string display_name;
330+ private string description;
331+ private string icon_key;
332+
333+ private Icon icon;
334+
335+ public GenericContract (ContractData data) {
336+ icon_key = "";
337+ update_data (data);
338+ }
339+
340+ public void update_data (ContractData data) {
341+ id = data.id ?? "";
342+ display_name = data.display_name ?? "";
343+ description = data.description ?? "";
344+
345+ if (icon_key != data.icon) {
346+ icon_key = data.icon ?? "";
347+ icon = null;
348+ }
349+ }
350+
351+ public string get_display_name () {
352+ return display_name;
353+ }
354+
355+ public string get_description () {
356+ return description;
357+ }
358+
359+ public Icon get_icon () {
360+ if (icon == null) {
361+ if (Path.is_absolute (icon_key))
362+ icon = new FileIcon (File.new_for_path (icon_key));
363+ else
364+ icon = new ThemedIcon.with_default_fallbacks (icon_key);
365+ }
366+
367+ return icon;
368+ }
369+
370+ public int execute_with_file (File file) throws Error {
371+ return ContractorProxy.execute_with_uri (id, file.get_uri ());
372+ }
373+
374+ public int execute_with_files (File[] files) throws Error {
375+ string[] uris = new string[files.length];
376+
377+ foreach (var file in files)
378+ uris += file.get_uri ();
379+
380+ return ContractorProxy.execute_with_uri_list (id, uris);
381+ }
382+ }
383+
384+
385+ private static ContractorDBusAPI contractor_dbus;
386+ private static Gee.HashMap<string, GenericContract> contracts;
387+
388+ private ContractorProxy () { }
389+
390+ private static void ensure () throws Error {
391+ if (contractor_dbus == null) {
392+ try {
393+ contractor_dbus = Bus.get_proxy_sync (BusType.SESSION,
394+ "org.elementary.Contractor",
395+ "/org/elementary/contractor");
396+ } catch (IOError e) {
397+ throw new ContractorError.SERVICE_NOT_AVAILABLE (e.message);
398+ }
399+ }
400+
401+ if (contracts == null)
402+ contracts = new Gee.HashMap<string, GenericContract> ();
403+ }
404+
405+ private static int execute_with_uri (string id, string uri) throws Error {
406+ ensure ();
407+ return contractor_dbus.execute_with_uri (id, uri);
408+ }
409+
410+ private static int execute_with_uri_list (string id, string[] uris) throws Error {
411+ ensure ();
412+ return contractor_dbus.execute_with_uri_list (id, uris);
413+ }
414+
415+ /**
416+ * Provides all the contracts.
417+ *
418+ * @return List containing all the contracts available in the system.
419+ */
420+ public static Gee.List<Contract> get_all_contracts () throws Error {
421+ ensure ();
422+
423+ var data = contractor_dbus.list_all_contracts ();
424+
425+ return get_contracts_from_data (data);
426+ }
427+
428+ /**
429+ * This searches for available contracts of a particular file type.
430+ *
431+ * @param mime_type Mimetype of file.
432+ * @return List of contracts that support the given mimetype.
433+ */
434+ public static Gee.List<Contract> get_contracts_by_mime (string mime_type) throws Error {
435+ ensure ();
436+
437+ var data = contractor_dbus.get_contracts_by_mime (mime_type);
438+
439+ return get_contracts_from_data (data);
440+ }
441+
442+ /**
443+ * Generate contracts for a list of mimetypes.
444+ *
445+ * Only the contracts that support all the mimetypes are returned.
446+ *
447+ * @param mime_types Array of mimetypes.
448+ * @return List of contracts that support the given mimetypes.
449+ */
450+ public static Gee.List<Contract> get_contracts_by_mimelist (string[] mime_types) throws Error {
451+ ensure ();
452+
453+ var data = contractor_dbus.get_contracts_by_mimelist (mime_types);
454+
455+ return get_contracts_from_data (data);
456+ }
457+
458+ private static Gee.List<Contract> get_contracts_from_data (ContractData[] data) {
459+ var contract_list = new Gee.LinkedList<Contract> ();
460+
461+ if (data != null) {
462+ foreach (var contract_data in data) {
463+ string contract_id = contract_data.id;
464+
465+ // See if we have a contract already. Otherwise create a new one.
466+ // We do this in order to be able to compare contracts by reference
467+ // from client code.
468+ var contract = contracts.get (contract_id);
469+
470+ if (contract == null) {
471+ contract = new GenericContract (contract_data);
472+ contracts.set (contract_id, contract);
473+ } else {
474+ contract.update_data (contract_data);
475+ }
476+
477+ contract_list.add (contract);
478+ }
479+ }
480+
481+ return contract_list;
482 }
483 }
484 }
485
486=== modified file 'lib/Widgets/ContractorMenu.vala'
487--- lib/Widgets/ContractorMenu.vala 2013-03-20 15:42:25 +0000
488+++ lib/Widgets/ContractorMenu.vala 2013-04-24 17:07:27 +0000
489@@ -20,6 +20,7 @@
490 /**
491 * This class provides a simple menu for managing Contractor.
492 */
493+[Deprecated (since = "0.2")]
494 public class Granite.Widgets.ContractorMenu : Gtk.Menu {
495 /**
496 * The Hashtable of available contracts
497
498=== modified file 'lib/Widgets/ContractorView.vala'
499--- lib/Widgets/ContractorView.vala 2013-03-20 15:42:25 +0000
500+++ lib/Widgets/ContractorView.vala 2013-04-24 17:07:27 +0000
501@@ -22,6 +22,7 @@
502 /**
503 * This class provides a simple way to look at contracts from Contractor
504 */
505+[Deprecated (since = "0.2")]
506 public class Granite.Widgets.ContractorView : TreeView {
507
508 /**

Subscribers

People subscribed via source and target branches