Created attachment 274098 [details] [review]
desktop-app-info test: use g_assert_strcmp()

Replace some assert(strcmp()) with g_assert_strcmp() so that we get
better output in case of failures.

Created attachment 274099 [details] [review]
mimeapps test: fix defaults vs. recommended

The desktop file for myapp3 didn't declare support for image/png, but
the testcase expects it to be recommended on the basis of it being the
default app according to defaults.list.

This will not work in the future -- we will only list apps that actually
support the filetype in question, unless they've been explicitly added
as associations.

Created attachment 274100 [details] [review]
tests: expand 'apps' tool

Add modes to output the applications found for get_default_,
get_recommended_, get_fallback_ and get_all_for_type().

Created attachment 274101 [details] [review]
appmonitor test: avoid /usr/share

Set XDG_DATA_DIRS to make sure we don't use /usr/share from the
appmonitor test.  We will soon throw a warning if we find defaults.list,
so make sure we don't open ourselves up to that if there is one on the
system.

Created attachment 274102 [details] [review]
mimetype tests: don't depend on specific behaviour

We currently assume that setting an application as the default will take
it to the front of the list of supported applications for a given type,
but this is not necessarily true.

Check instead that the application is actually set as default.

Created attachment 274103 [details] [review]
GDesktopAppInfo: rewrite content type code

Redo the code for type-based selection of applications (all,
recommended, default, fallback) based on the new DesktopFileDir
structures that we introduced last cycle.

At the same time, we expand the functionality to add support for the new
features of the specification:

 - moving ~/.local/share/applications/mimeapps.list to ~/.config/

 - per-desktop default applications (via XDG_CURRENT_DESKTOP)

 - sysadmin customisation of defaults (via /etc/xdg/mimeapps.list)

 - deprecation of the old defaults.list, favouring the use of
   /usr/share/applications/mimeapps.list (or gnome-mimeapps.list) to
   accomplish the same

We modify the mimeapps testcase to check for mimeapps.list having been
created in XDG_CONFIG_HOME instead of XDG_DATA_HOME.

The modification is a net reduction of code (due to less duplication in
bookkeeping).  It is also an increase in performance and reduction in
memory consumption (due to simplified data structures).  Finally, it
removes the stat-based timestamp checking in favour of the
GFileMonitor-based approach that was already being used in the
implementation of DesktopFileDir (in order to know if we had to rescan
the desktop files themselves).

Created attachment 274104 [details] [review]
tests: use mimeapps.list over defaults.list

defaults.list is deprecated, so use mimeapps.list as a filename instead.

Review of attachment 274098 [details] [review]:

Okay

Review of attachment 274099 [details] [review]:

Okay

Review of attachment 274100 [details] [review]:

::: gio/tests/apps.c
@@ +75,3 @@
+        print (g_app_info_get_id (info));
+
+      g_object_unref (info);

The unref should be in the if branch.

Review of attachment 274099 [details] [review]:

Okay

Review of attachment 274101 [details] [review]:

Okay

Review of attachment 274102 [details] [review]:

::: gio/tests/desktop-app-info.c
@@ +120,3 @@
+  /* check that info2 is still default */
+  info = g_app_info_get_default_for_type ("application/x-test", FALSE);
+  g_assert (info != NULL);

Shouldn't you check whether info == info2?

@@ +129,3 @@
+  /* and make sure info2 is still default */
+  info = g_app_info_get_default_for_type ("application/x-test", FALSE);
+  g_assert (info != NULL);

info = info2?

Review of attachment 274102 [details] [review]:

::: gio/tests/desktop-app-info.c
@@ +123,1 @@
   g_assert_cmpstr (g_app_info_get_id (info), ==, g_app_info_get_id (info2));

...which is done here.

@@ +132,1 @@
   g_assert_cmpstr (g_app_info_get_id (info), ==, g_app_info_get_id (info2));

...and here.

Review of attachment 274103 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +1077,3 @@
+
+          if (!desktop_file_dir_app_name_is_masked (dir, app_name) &&
+              !g_hash_table_contains (blacklist, app_name) &&

This check is unnecessary

@@ +1078,3 @@
+          if (!desktop_file_dir_app_name_is_masked (dir, app_name) &&
+              !g_hash_table_contains (blacklist, app_name) &&
+              !g_hash_table_contains (hits, app_name))

Shouldn't we remove it from 'hits' if it is in removals? (Or am I showing my lack of knowledge of the spec?)

@@ +1142,2 @@
 /*< internal >
+ * desktop_file_dir_create:

_for_config

@@ +1284,3 @@
+                              GHashTable     *hits,
+                              guint          *n_hits,
+                              GHashTable     *blacklist)

I find the hits/n_hits semantics a bit weird. Maybe a growing GPtrArray and a GHashTable (as a set) for lookups would be clearer?

Review of attachment 274104 [details] [review]:

Ya...

Review of attachment 274102 [details] [review]:

My bad, sorry.

> Review of attachment 274103 [details] [review]:
> 
> ::: gio/gdesktopappinfo.c
> @@ +1077,3 @@
> +
> +          if (!desktop_file_dir_app_name_is_masked (dir, app_name) &&
> +              !g_hash_table_contains (blacklist, app_name) &&
> 
> This check is unnecessary

Why?

> @@ +1078,3 @@
> +          if (!desktop_file_dir_app_name_is_masked (dir, app_name) &&
> +              !g_hash_table_contains (blacklist, app_name) &&
> +              !g_hash_table_contains (hits, app_name))
> 
> Shouldn't we remove it from 'hits' if it is in removals? (Or am I showing my
> lack of knowledge of the spec?)

The idea here is that if it gets into the blacklist before we find a hit then it's because we found an explicit removal in a higher-level directory (and should therefore ignore the add at this level).(In reply to comment #15)

> I find the hits/n_hits semantics a bit weird. Maybe a growing GPtrArray and a
> GHashTable (as a set) for lookups would be clearer?

OK.  I'll just use a GPtrArray; I did it this way for defaults anyway.

Created attachment 274116 [details] [review]
tests: expand 'apps' tool

Add modes to output the applications found for get_default_,
get_recommended_, get_fallback_ and get_all_for_type().

Created attachment 274117 [details] [review]
GDesktopAppInfo: rewrite content type code

Redo the code for type-based selection of applications (all,
recommended, default, fallback) based on the new DesktopFileDir
structures that we introduced last cycle.

At the same time, we expand the functionality to add support for the new
features of the specification:

 - moving ~/.local/share/applications/mimeapps.list to ~/.config/

 - per-desktop default applications (via XDG_CURRENT_DESKTOP)

 - sysadmin customisation of defaults (via /etc/xdg/mimeapps.list)

 - deprecation of the old defaults.list, favouring the use of
   /usr/share/applications/mimeapps.list (or gnome-mimeapps.list) to
   accomplish the same

We modify the mimeapps testcase to check for mimeapps.list having been
created in XDG_CONFIG_HOME instead of XDG_DATA_HOME.

The modification is a net reduction of code (due to less duplication in
bookkeeping).  It is also an increase in performance and reduction in
memory consumption (due to simplified data structures).  Finally, it
removes the stat-based timestamp checking in favour of the
GFileMonitor-based approach that was already being used in the
implementation of DesktopFileDir (in order to know if we had to rescan
the desktop files themselves).

Review of attachment 274116 [details] [review]:

Thanks

Review of attachment 274117 [details] [review]:

Thanks.

Attachment 274098 [details] pushed as 84e9829 - desktop-app-info test: use g_assert_strcmp()
Attachment 274099 [details] pushed as 41761a1 - mimeapps test: fix defaults vs. recommended
Attachment 274101 [details] pushed as 5404708 - appmonitor test: avoid /usr/share
Attachment 274102 [details] pushed as caf0f1d - mimetype tests: don't depend on specific behaviour
Attachment 274104 [details] pushed as 9a5e85c - tests: use mimeapps.list over defaults.list
Attachment 274116 [details] pushed as a366b6f - tests: expand 'apps' tool
Attachment 274117 [details] pushed as 6fd5a8c - GDesktopAppInfo: rewrite content type code