GNOME Bugzilla – Bug 681246
Add GNOME Shell search provider
Last modified: 2016-03-31 13:59:33 UTC
This series allows to search boxes from the GNOME Shell search results. The matching boxes will show up with their last screenshot if available, and can be activated to display the box.
Created attachment 220378 [details] [review] display-config: save last-seen-name We will use this value when doing offline searches, to avoid having to connect to all the brokers and introspecting their box to find out their name. This is mostly a cache, and in case of remote-display, it is even redundant, but that doesn't matter much.
Created attachment 220379 [details] [review] util: move foreach_from_dir () This is fairly generic code, make app.vala a bit smaller and can be later reused.
Created attachment 220380 [details] [review] Split util.vala Let's move gnome-boxes application helpers to util-app.vala. util.vala will be shared with gnome-boxes-search-provider.vala
Created attachment 220381 [details] [review] util: add a few helper copied from gnome-contacts
Created attachment 220382 [details] [review] util: add uuid_generate ()
Created attachment 220383 [details] [review] display: move DisplayProperties in seperate file This allow the file to be used seperately by gnome-boxes-search-provider.
Created attachment 220384 [details] [review] Start GNOME Shell search provider Add gnome-boxes-search-provider skeleton. With this basic form, it can already be seen by GNOME Shell, but will return no result atm.
Created attachment 220385 [details] [review] display-config: store a unique uuid Move the DisplayConfig () constructor out of its class, since it has some logic and can now generate UUID if it's not provided. This also allows to simplify a little bit the code.
Created attachment 220386 [details] [review] main: add --open-uuid Allow to open a Box with its UUID. This is needed for reliably opening a box from GNOME Shell search results.
Created attachment 220387 [details] [review] machine: use UUID instead of screenshot prefix Now that there is a UUID for each box, we can stop having various get_screenshot_prefix () code, and use it as filename prefix.
Created attachment 220388 [details] [review] screenshot: simplify, make get_screenshot_filename () reusable
Created attachment 220389 [details] [review] search-provider: learn to return results
Created attachment 220390 [details] [review] search-provider: implement ActivateResult
Created attachment 220391 [details] [review] build-sys: use a libcommon.a library to fix tons of build warnings The same Vala file can't be used several times, as automake will generate the same target to compile the Vala file. Instead, compile those shared file once in a common library. Also update the Vala files to remote the warnings (make classes and interface public)
(In reply to comment #14) > The same Vala file can't be used several times, as automake will > generate the same target to compile the Vala file. [Can't make inline replies with splinter, using a regular bugzilla comment instead...] Wouldn't this be an automake bug, or a bug in our Makefile.am? As far as I remember, C files can be reused several times in different targets without issues (maybe using AM_PROG_CC_C_O or subdir-objects is needed in these cases)
(In reply to comment #15) > Wouldn't this be an automake bug, or a bug in our Makefile.am? As far as I > remember, C files can be reused several times in different targets without > issues (maybe using AM_PROG_CC_C_O or subdir-objects is needed in these cases) It's an "automake vala support" bug, see also: http://www.gnu.org/software/automake/manual/automake.html#Vala-Support Anyway, it makes tons of sense to build those files only once!
Review of attachment 220378 [details] [review]: ACK
Review of attachment 220379 [details] [review]: 'make' -> 'makes' Good otherwise. ::: src/app.vala @@ -307,4 +307,4 @@ - yield get_sources_from_dir (dir); - } - - private async void get_sources_from_dir (File dir) { + yield foreach_from_dir (dir, (filename) => { + var source = new CollectionSource.with_file (filename); + add_collection_source.begin (source); + return false; coding-style nitpick: newline before return. ::: src/util.vala @@ -627,1 +627,5 @@ } + + public delegate bool ForeachFromDirFunc (string filename) throws GLib.Error; + + public async void foreach_from_dir (File dir, ForeachFromDirFunc func) { 'foreach_filename_under_dir' would be a better name IMO.
Review of attachment 220380 [details] [review]: What are application helpers? I first thought you mean UI-related helpers but then i see some non-UI related helpers being moved too.
Review of attachment 220381 [details] [review]: 'helper' -> 'helpers' ACK ::: src/util.vala @@ -222,2 +222,3 @@ } + public bool is_set (string? str) { are we the only ones caring for good names? :( @@ +314,3 @@ + var c = strip_char (s.get_char ()); + if (c != 0) { + var size = c.fully_decompose (false, buf); Don't pretend to know what 'decomposition' means here but I trust that this function has already been tested well in Contacts so will simply trust here.
Review of attachment 220381 [details] [review]: 'add a few helper copied from gnome-contacts' -> 'Copy a few helpers from gnome-contacts' ACK ::: src/util.vala @@ -222,2 +222,3 @@ } + public bool is_set (string? str) { are we the only ones caring for good names? :( @@ +314,3 @@ + var c = strip_char (s.get_char ()); + if (c != 0) { + var size = c.fully_decompose (false, buf); Don't pretend to know what 'decomposition' means here but I trust that this function has already been tested well in Contacts so will simply trust here.
Review of attachment 220382 [details] [review]: Assuming you'll use it later patches, ACK! ::: src/util-app.vala @@ -342,0 +342,5 @@ + + namespace UUID { + [CCode (cname = "uuid_generate", cheader_filename = "uuid/uuid.h")] ... 2 more ... i think you don't need the two annotations as rygel does with only 1: http://git.gnome.org/browse/rygel/tree/src/librygel-core/uuid.vapi
Review of attachment 220380 [details] [review]: Keeping the end goal in mind now, i see why you are doing this split. Although the naming of the modules don't make the reason for this split obvious.
Review of attachment 220383 [details] [review]: Yes please :)
Review of attachment 220384 [details] [review]: Without any docs on this and no easy way to test this, I'll trust that it works. Code looks good.
Review of attachment 220379 [details] [review]: ::: src/app.vala @@ -307,4 +307,4 @@ - yield get_sources_from_dir (dir); - } - - private async void get_sources_from_dir (File dir) { + yield foreach_from_dir (dir, (filename) => { + var source = new CollectionSource.with_file (filename); + add_collection_source.begin (source); + return false; this return value is not significant since we always loop over all the files. I prefer not to make this return block outstanding from the rest of the loop block. ::: src/util.vala @@ -627,1 +627,5 @@ } + + public delegate bool ForeachFromDirFunc (string filename) throws GLib.Error; + + public async void foreach_from_dir (File dir, ForeachFromDirFunc func) { ack
Review of attachment 220379 [details] [review]: ::: src/app.vala @@ -307,4 +307,4 @@ - yield get_sources_from_dir (dir); - } - - private async void get_sources_from_dir (File dir) { + yield foreach_from_dir (dir, (filename) => { + var source = new CollectionSource.with_file (filename); + add_collection_source.begin (source); + return false; ok :)
Review of attachment 220385 [details] [review]: Good otherwise.. ::: src/machine.vala @@ +209,3 @@ } + protected void make_config (string? uuid = null) I'd prefer 'display' in this name: make_display_config or create_display_config.. @@ -209,2 +209,4 @@ } + protected void make_config (string? uuid = null) + requires (this.config == null) coding-style nitpick: Although we didn't agree about this but I've been indenting 'requires' the same as 'throws' and arguments so please do the same for consistency.
Review of attachment 220385 [details] [review]: ::: src/machine.vala @@ +209,3 @@ } + protected void make_config (string? uuid = null) ok @@ -209,2 +209,4 @@ } + protected void make_config (string? uuid = null) + requires (this.config == null) given that requires/ensures are not necessarily of caller concern, I would prefer we don't enforce a policy on having those at the same level as the function prototype. They are actually evaluated expressions, not just declaration, which place them at the level of the function block rather than at the declaration level.
Review of attachment 220386 [details] [review]: Good otherwise. ::: src/app.vala @@ +177,3 @@ + public bool open_uuid (string uuid) { + ui_state = UIState.COLLECTION; + view.visible = false; // to avoid some glitches what glitches? More explanation needed i guess? @@ +182,3 @@ + foreach (var item in collection.items.data) { + var machine = item as Boxes.Machine; + if (machine == null) I'd prefer (to keep the cast check less vala-specific and more readable to vala newbies): if (!(item is Machine)) continue; var machine = item as Boxes.Machine; @@ +185,3 @@ + continue; + + message (item.name); debug perhaps? ::: src/main.vala @@ +42,3 @@ + if (uris.length > 1 || + (open_uuid != null && uris != null)) { + GLib.stderr.printf (_("Too many arguments specified.\n")); To make it very clear to translators, I suggest: arguments -> commandline arguments. @@ +115,3 @@ app.ready.connect ((first_time) => { + if (open_uuid != null) { + app.open_uuid (open_uuid); got indented twice.
Review of attachment 220386 [details] [review]: ::: src/app.vala @@ +177,3 @@ + public bool open_uuid (string uuid) { + ui_state = UIState.COLLECTION; + view.visible = false; // to avoid some glitches what about "we don't want to show the collection items as it will appear as a glitch when opening a box immediately at startup" @@ +182,3 @@ + foreach (var item in collection.items.data) { + var machine = item as Boxes.Machine; + if (machine == null) there are pros and cons, but I can see it's more friendly to non-vala people.. @@ +185,3 @@ + continue; + + message (item.name); yup, removed ::: src/main.vala @@ +42,3 @@ + if (uris.length > 1 || + (open_uuid != null && uris != null)) { + GLib.stderr.printf (_("Too many arguments specified.\n")); ok @@ +115,3 @@ app.ready.connect ((first_time) => { + if (open_uuid != null) { + app.open_uuid (open_uuid); fixed
Review of attachment 220387 [details] [review]: ACK
Review of attachment 220388 [details] [review]: looks obviously right
Created attachment 221010 [details] [review] search-provider: learn to return results
Review of attachment 221010 [details] [review]: Looks right otherwise. ::: src/gnome-boxes-search-provider.vala @@ +21,3 @@ + } + + private async void load () { just wonder if this code could be done cleaner using recursion?
Review of attachment 220390 [details] [review]: Looks good
Review of attachment 220391 [details] [review]: ACK
Attachment 220378 [details] pushed as 9253b50 - display-config: save last-seen-name Attachment 220379 [details] pushed as 693d65d - util: move foreach_from_dir () Attachment 220380 [details] pushed as c4917dd - Split util.vala Attachment 220382 [details] pushed as bcef9aa - util: add uuid_generate () Attachment 220383 [details] pushed as 8e0c53a - display: move DisplayProperties in seperate file Attachment 220384 [details] pushed as cc5dff0 - Start GNOME Shell search provider Attachment 220385 [details] pushed as 839eed8 - display-config: store a unique uuid Attachment 220386 [details] pushed as ff2fa2d - main: add --open-uuid Attachment 220387 [details] pushed as fbb94c7 - machine: use UUID instead of screenshot prefix Attachment 220390 [details] pushed as 05ecb76 - search-provider: implement ActivateResult Attachment 220391 [details] pushed as cb2d5af - build-sys: use a libcommon.a library to fix tons of build warnings Attachment 221010 [details] pushed as 3c1201e - search-provider: learn to return results
(In reply to comment #37) > + private async void load () { > > just wonder if this code could be done cleaner using recursion? By nature the problem on waiting for load is not recursive. It is better written in an efficient way, vala doesn't have tail-call optimization.
Does not build with vala 0.16: gnome-boxes-search-provider.vala:184.13-184.16: error: The name `quit' does not exist in the context of `Boxes.SearchProviderApp.dbus_register' quit (); ^^^^ gnome-boxes-search-provider.vala:179.5-179.38: error: Boxes.SearchProviderApp.dbus_register: no suitable method found to override public override bool dbus_register (GLib.DBusConnection connection, string object_path) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Compilation failed: 2 error(s), 0 warning(s)
commit a3198458e7b430bf5368cf20b70641e0c8e3ba55 Author: Marc-André Lureau <marcandre.lureau@gmail.com> Date: Tue Aug 14 12:27:41 2012 +0300 build-sys: bump vala dependency to 0.17.2 Fix recent gio-2.0 bindings requirements check. Acked-by: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>
This is not really the fix I expected... Can't we import the gio-2.0 bindings instead?
(In reply to comment #44) > This is not really the fix I expected... Can't we import the gio-2.0 bindings > instead? As long as it has been part of a vala release, I don't see why we can't bump our dependency and use it. If it wouldn't be released already we would have to copy the bindings, like we did sometime ago. Now we keep some bindings around to avoid tons of deprecation warnings, which is bad imho, we should fix our code instead. It just takes a bit of time.
(In reply to comment #45) > (In reply to comment #44) > > This is not really the fix I expected... Can't we import the gio-2.0 bindings > > instead? > > As long as it has been part of a vala release, I don't see why we can't bump > our dependency and use it. I'd rather be able to use a stable vala version than a development one, if it's just a matter of copying gio-2.0 in our tree, I'd rather we do that.
(In reply to comment #46) > I'd rather be able to use a stable vala version than a development one, if it's > just a matter of copying gio-2.0 in our tree, I'd rather we do that. During unstable period we should follow GNOME vala version, which is the latest release.
(In reply to comment #47) > During unstable period we should follow GNOME vala version, which is the latest > release. I'm not saying we must not use vala 0.17, but if we can keep things working with 0.16 as well this would be much nicer. I am *not* interested in using a development version of the toolchain, and vala is supposed to be mature and stable, so I don't see why we'd need to use devel versions.
Created attachment 221124 [details] [review] build: re-add gio-2.0.vapi This is needed to build the GNOME Shell integration code with vala 0.16
Review of attachment 221124 [details] [review]: You can copy custom bindings under $XDG_DATA_DIRS For instance, you can export XDG_DATA_DIRS=$HOME/.local: /usr/local/share/:/usr/share/ and copy a newer gio-2.0.vapi in $HOME/.local/vala/vapi/ if you want to use older version of vala. Note that you will also have to change configure.ac to not require vala 0.17.2
How convenient.... Let's make everyone life harder because we prefer to track git for no good reason!