Created attachment 276156 [details] [review]
AppInfo: use XDG_CURRENT_DESKTOP for OnlyShowIn

Review of attachment 276156 [details] [review]:

There's a small regression: calling g_desktop_app_info_get_show_in() before g_desktop_app_info_set_desktop_env() will make the latter call not work. I don't think that will be an issue in practice, or will it?

Review of attachment 276156 [details] [review]:

Seems pretty much okay to me, a couple of comments, but I don't see any obvious show-stopping bugs.

::: gio/gdesktopappinfo.c
@@ +246,3 @@
+  static gchar **result;
+
+  if (g_once_init_enter (&result))

As mentioned by Lars, maybe this shouldn't be GOnce since set_desktop_env might be called after get_show_in (though that's sort of questionable behavior on the apps part since set_desktop_env can only be called once)

@@ +254,3 @@
+
+      if (value)
+        tmp = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);

shouldn't this be the literal ":" ?  XDG_CURRENT_DESKTOP doesn't use ";" on windows right? (Granted, I guess, it's not used at all on windows at the moment)

@@ +256,3 @@
+        tmp = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
+      else
+        tmp = g_new0 (gchar *, 0 + 1);

I think the "0 + 1" is supposed to mean "0 elements + NULL terminator" but it's a little confusing to read.  What might be clearer is to unconditionally call g_strsplit, but pass it ' value? value : "" ' or above it have:

if (!value)
   value = "";

and then let g_strsplit handle it for you. Just an idea, take it or leave it :-)

Review of attachment 276156 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +246,3 @@
+  static gchar **result;
+
+  if (g_once_init_enter (&result))

This was intentional.  I think this is correct, for a couple of reasons.

First is that the call is deprecated.

Second is that the only reasonable argument that you might make here is that you have a process that contains two entities and one of them manages to call _get_show_in() before the other has a chance to call set_desktop_env().  In that case, though, the one that calls _get_show_in() could notice inconsistent behaviour later (apps appearing or disappearing, effectively), despite not having received a signal from the appinfo monitor.

@@ +254,3 @@
+
+      if (value)
+        tmp = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);

Ya.  That would probably read easier.

@@ +256,3 @@
+        tmp = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
+      else
+        tmp = g_new0 (gchar *, 0 + 1);

I really like "0 + 1" because although it is "a little confusing to read" it catches the eye and makes you think about it, after which you invariably come to the correct conclusion about what is going on here (as you just did).

In any case, I think your suggestion to unconditionally call split() is a good one.

Created attachment 276199 [details] [review]
AppInfo: use XDG_CURRENT_DESKTOP for OnlyShowIn

Expand the functionality of g_desktop_app_info_set_desktop_env() to
include the possibility of passing strings containing ':' characters (as
some apps, such as gnome-session, are directly passing the value of
XDG_CURRENT_DESKTOP).  At the same time, deprecate it, since now we get
the list from the environment variable for ourselves.

Modify the checks in g_desktop_app_info_get_show_in() to deal with
multiple items listed in XDG_CURRENT_DESKTOP.  For example, if we find
that we have

  XDG_CURRENT_DESKTOP=GNOME-Classic:GNOME

and a desktop file contains:

  OnlyShowIn=GNOME

then we will show this file because of the fallback to GNOME.  If the
file _also_ contains the line:

  NotShowIn=GNOME-Classic

Then we will not show it, because GNOME-Classic comes before GNOME in
XDG_CURRENT_DESKTOP.

Created attachment 276200 [details] [review]
tests: add testcase for {Only,Not}ShowIn

Attachment 276199 [details] pushed as 5a5e16e - AppInfo: use XDG_CURRENT_DESKTOP for OnlyShowIn
Attachment 276200 [details] pushed as 079d20f - tests: add testcase for {Only,Not}ShowIn