Created attachment 289296 [details] [review]
search: use g_get_system_data_dirs() to discover providers

Currently, the search panel looks for search providers in
DATADIR/gnome-shell/search-providers. This won't work when providers are
located in a different directory which is still part of XDG_DATA_DIRS,
which is a valid use case and supported by gnome-shell.

This commit refactors the loader code to scan all the directories
upfront in a separate thread.

[endlessm/eos-shell#4049]

Review of attachment 289296 [details] [review]:

Is the endless bug available publically? If not, please remove it, if it is, please put the full URL.

::: panels/search/cc-search-panel.c
@@ +572,3 @@
+   */
+  search_panel_propagate_sort_order (self);
+  g_list_free_full (providers, g_object_unref);

Unref in the loop above?

@@ +605,1 @@
+      if (error != NULL)

Need to handle the cancelled case.

@@ +620,3 @@
+        }
+
+      if (error != NULL)

Ditto.

@@ +641,3 @@
+  GTask *task;
+
+  task = g_task_new (self, NULL, search_providers_discover_ready, self);

Looks like the thread isn't cancellable, which is going to make it crash when leaving the panel before the thread has finished the enumeration.

Created attachment 289357 [details] [review]
search: use g_get_system_data_dirs() to discover providers

--

Thanks for the review - this should address your comments.

Review of attachment 289357 [details] [review]:

::: panels/search/cc-search-panel.c
@@ +558,2 @@
+  providers = g_task_propagate_pointer (G_TASK (result), &error);
+  g_clear_object (&self->priv->load_cancellable);

You can't access the user_data before checking that the cancellable isn't cancelled.

@@ +651,3 @@
+        }
+
+    cleanup:

A label inside a for loop is pretty gross. Could you split out the contents of the loop into a separate function so that this sort of construct isn't needed. Even using a macro would probably be nicer than this.

@@ +676,3 @@
+
+static void
+cc_search_panel_dispose (GObject *object)

Can't this be done in finalize() instead? Is there a cyclical reference somewhere?

Created attachment 289471 [details] [review]
search: use g_get_system_data_dirs() to discover providers

--

Address comments from review

(In reply to comment #4)
 
> You can't access the user_data before checking that the cancellable isn't
> cancelled.

Fixed, but it shouldn't really matter - see below.
 
> A label inside a for loop is pretty gross. Could you split out the contents of
> the loop into a separate function so that this sort of construct isn't needed.
> Even using a macro would probably be nicer than this.

You're right, code becomes much nicer - fixed.

> +
> +static void
> +cc_search_panel_dispose (GObject *object)
> 
> Can't this be done in finalize() instead? Is there a cyclical reference
> somewhere?

Yeah; g_task_new() will take a reference to the panel itself for the duration of the task, so canceling from finalize() is too late.

Review of attachment 289471 [details] [review]:

Looks good.

Thanks, pushed now

Attachment 289471 [details] pushed as 0a7b552 - search: use g_get_system_data_dirs() to discover providers