After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 681246 - Add GNOME Shell search provider
Add GNOME Shell search provider
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-08-05 17:11 UTC by Marc-Andre Lureau
Modified: 2016-03-31 13:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
display-config: save last-seen-name (3.36 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
util: move foreach_from_dir () (2.66 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
Split util.vala (33.34 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
util: add a few helper copied from gnome-contacts (2.10 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
util: add uuid_generate () (1.84 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
display: move DisplayProperties in seperate file (7.46 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
Start GNOME Shell search provider (6.73 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
display-config: store a unique uuid (3.77 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
main: add --open-uuid (3.16 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
machine: use UUID instead of screenshot prefix (2.24 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
screenshot: simplify, make get_screenshot_filename () reusable (2.68 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
search-provider: learn to return results (7.62 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
none Details | Review
search-provider: implement ActivateResult (1.35 KB, patch)
2012-08-05 17:13 UTC, Marc-Andre Lureau
committed Details | Review
build-sys: use a libcommon.a library to fix tons of build warnings (6.87 KB, patch)
2012-08-05 17:14 UTC, Marc-Andre Lureau
committed Details | Review
search-provider: learn to return results (7.08 KB, patch)
2012-08-13 11:09 UTC, Marc-Andre Lureau
committed Details | Review
build: re-add gio-2.0.vapi (223.64 KB, patch)
2012-08-14 10:16 UTC, Christophe Fergeau
rejected Details | Review

Description Marc-Andre Lureau 2012-08-05 17:11:21 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.
Comment 1 Marc-Andre Lureau 2012-08-05 17:13:18 UTC
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.
Comment 2 Marc-Andre Lureau 2012-08-05 17:13:21 UTC
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.
Comment 3 Marc-Andre Lureau 2012-08-05 17:13:25 UTC
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
Comment 4 Marc-Andre Lureau 2012-08-05 17:13:28 UTC
Created attachment 220381 [details] [review]
util: add a few helper copied from gnome-contacts
Comment 5 Marc-Andre Lureau 2012-08-05 17:13:32 UTC
Created attachment 220382 [details] [review]
util: add uuid_generate ()
Comment 6 Marc-Andre Lureau 2012-08-05 17:13:35 UTC
Created attachment 220383 [details] [review]
display: move DisplayProperties in seperate file

This allow the file to be used seperately by gnome-boxes-search-provider.
Comment 7 Marc-Andre Lureau 2012-08-05 17:13:38 UTC
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.
Comment 8 Marc-Andre Lureau 2012-08-05 17:13:42 UTC
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.
Comment 9 Marc-Andre Lureau 2012-08-05 17:13:45 UTC
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.
Comment 10 Marc-Andre Lureau 2012-08-05 17:13:49 UTC
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.
Comment 11 Marc-Andre Lureau 2012-08-05 17:13:53 UTC
Created attachment 220388 [details] [review]
screenshot: simplify, make get_screenshot_filename () reusable
Comment 12 Marc-Andre Lureau 2012-08-05 17:13:56 UTC
Created attachment 220389 [details] [review]
search-provider: learn to return results
Comment 13 Marc-Andre Lureau 2012-08-05 17:13:59 UTC
Created attachment 220390 [details] [review]
search-provider: implement ActivateResult
Comment 14 Marc-Andre Lureau 2012-08-05 17:14:03 UTC
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)
Comment 15 Christophe Fergeau 2012-08-05 17:45:22 UTC
(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)
Comment 16 Marc-Andre Lureau 2012-08-05 17:58:49 UTC
(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!
Comment 17 Zeeshan Ali 2012-08-09 00:25:41 UTC
Review of attachment 220378 [details] [review]:

ACK
Comment 18 Zeeshan Ali 2012-08-09 00:32:03 UTC
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.
Comment 19 Zeeshan Ali 2012-08-09 00:46:22 UTC
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.
Comment 20 Zeeshan Ali 2012-08-09 00:58:06 UTC
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.
Comment 21 Zeeshan Ali 2012-08-09 00:59:27 UTC
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.
Comment 22 Zeeshan Ali 2012-08-09 01:00:22 UTC
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.
Comment 23 Zeeshan Ali 2012-08-09 01:08:12 UTC
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
Comment 24 Zeeshan Ali 2012-08-12 22:23:32 UTC
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.
Comment 25 Zeeshan Ali 2012-08-12 22:25:07 UTC
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
Comment 26 Zeeshan Ali 2012-08-12 22:27:19 UTC
Review of attachment 220383 [details] [review]:

Yes please :)
Comment 27 Zeeshan Ali 2012-08-12 22:38:00 UTC
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.
Comment 28 Marc-Andre Lureau 2012-08-13 08:45:35 UTC
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
Comment 29 Zeeshan Ali 2012-08-13 08:57:16 UTC
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 :)
Comment 30 Zeeshan Ali 2012-08-13 09:35:39 UTC
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.
Comment 31 Marc-Andre Lureau 2012-08-13 09:56:21 UTC
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.
Comment 32 Zeeshan Ali 2012-08-13 09:59:23 UTC
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.
Comment 33 Marc-Andre Lureau 2012-08-13 10:28:08 UTC
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
Comment 34 Zeeshan Ali 2012-08-13 10:32:19 UTC
Review of attachment 220387 [details] [review]:

ACK
Comment 35 Zeeshan Ali 2012-08-13 10:35:21 UTC
Review of attachment 220388 [details] [review]:

looks obviously right
Comment 36 Marc-Andre Lureau 2012-08-13 11:09:53 UTC
Created attachment 221010 [details] [review]
search-provider: learn to return results
Comment 37 Zeeshan Ali 2012-08-13 12:11:22 UTC
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?
Comment 38 Zeeshan Ali 2012-08-13 12:15:05 UTC
Review of attachment 220390 [details] [review]:

Looks good
Comment 39 Zeeshan Ali 2012-08-13 12:17:53 UTC
Review of attachment 220391 [details] [review]:

ACK
Comment 40 Marc-Andre Lureau 2012-08-13 12:21:27 UTC
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
Comment 41 Marc-Andre Lureau 2012-08-13 12:25:58 UTC
(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.
Comment 42 Christophe Fergeau 2012-08-14 08:39:47 UTC
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)
Comment 43 Marc-Andre Lureau 2012-08-14 09:40:27 UTC
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>
Comment 44 Christophe Fergeau 2012-08-14 09:44:19 UTC
This is not really the fix I expected... Can't we import the gio-2.0 bindings instead?
Comment 45 Marc-Andre Lureau 2012-08-14 09:49:55 UTC
(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.
Comment 46 Christophe Fergeau 2012-08-14 09:53:46 UTC
(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.
Comment 47 Marc-Andre Lureau 2012-08-14 09:58:31 UTC
(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.
Comment 48 Christophe Fergeau 2012-08-14 10:07:02 UTC
(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.
Comment 49 Christophe Fergeau 2012-08-14 10:16:24 UTC
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
Comment 50 Marc-Andre Lureau 2012-08-14 10:33:46 UTC
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
Comment 51 Christophe Fergeau 2012-08-14 10:37:05 UTC
How convenient.... Let's make everyone life harder because we prefer to track git for no good reason!