For the sake of completeness: editing the file in some other location
and then moving it into the destination (to ensure that the shell never
sees a half-written, broken file) does no work either — that would be:

  % sed -e 's/^Exec=/Exec=zenity --error --text=ItWorks' \
        /usr/share/applications/inkscape.desktop > ~/inkscape.desktop
  % mv ~/inkscape.desktop ~/.local/share/applications

(Renaming a file inside the same file system is an atomic operation.)

Interestingly enough, changing the “Icon=” field makes the Shell notice
invalidate its cache of applications, and the icon shows changed in the
overview. I am looking at making a patch for this, so changes in other
fields also cause the Shell to invalidate all the cached data for a
modified “.desktop” file.

Created attachment 339260 [details] [review]
Crude patch to always refresh info from .desktop files

I have been investigating this, and the problem is that the cached
application infos in ShellAppSystem (src/shell-app-system.c) are not
properly invalidated when “.desktop” files which override the system
ones are moved into/out of “~/.local/share/applications”, or when the
“Exec=” line changes.

The attached patch is very crude and probably we don't want to merge
it as-is, but when applied it makes the shell pick all changes properly
because it completely nukes the cache when changes are detected.

Created attachment 339271 [details] [review]
Properly detect changes in .desktop files

Instead of nuking the whole hash table like the old patch did, this just
causes removal of the ShellApp objects for which one of the following has
changed:

 * Filename of the .desktop file (covers the case of overriding with a
   file in ~/.local/share/applications).
 * Visibility of the application (g_app_info_should_show()).
 * Executable.
 * Command line.
 * Application name.
 * Displayed name.
 * Application description.
 * Application icon.

Review of attachment 339271 [details] [review]:

Thanks for working on this.

This is just a couple preliminary comments; you'll need to wait for a shell maintainer to review it as well:

::: src/shell-app-system.c
@@ +112,3 @@
+/* Same as g_str_equal(), but NULLs are taken into account. */
+static inline gboolean
+nullable_str_equal (const gchar *a, const gchar *b)

You should use g_strcmp0

@@ +138,1 @@
+  old = shell_app_get_app_info (app);

This is leaked

(In reply to Michael Catanzaro from comment #5)
> Review of attachment 339271 [details] [review] [review]:
> 
> Thanks for working on this.
> 
> This is just a couple preliminary comments; you'll need to wait for a shell
> maintainer to review it as well:
> 
> ::: src/shell-app-system.c
> @@ +112,3 @@
> +/* Same as g_str_equal(), but NULLs are taken into account. */
> +static inline gboolean
> +nullable_str_equal (const gchar *a, const gchar *b)
> 
> You should use g_strcmp0

Ah, nice one, I didn't know tht GLib already had such an utility function.
I will update the patch to use it :-)
 
> @@ +138,1 @@
> +  old = shell_app_get_app_info (app);
> 
> This is leaked

Not quite: If you check “src/shell-app.c” (lines 1276-1287), you will
see that the return value for “shell_app_get_app_info()” is annotated as
“transfer none” — and indeed its code returns the pointer directly.

Created attachment 339291 [details] [review]
Properly detect changes in .desktop files (v2)

Updated patch to use “strcmp()” and “g_strcmp0()” as suggested by
Michael. Also, I moved the function to compare icons inline, as it
used only once.

Review of attachment 339291 [details] [review]:

::: src/shell-app-system.c
@@ +132,1 @@
+  is_unchanged =

There's a part of me that wishes g_app_info_equal() did all of these checks instead...

(In reply to Cosimo Cecchi from comment #8)
> Review of attachment 339291 [details] [review] [review]:
> 
> ::: src/shell-app-system.c
> @@ +132,1 @@
> +  is_unchanged =
> 
> There's a part of me that wishes g_app_info_equal() did all of these checks
> instead...

That was my first attempt, to end up noticing that it didn't work, then
checking its implementation it turned out that g_app_info_equal() only
compares the application identifier :-\

Review of attachment 339291 [details] [review]:

::: src/shell-app-system.c
@@ +133,3 @@
+    g_app_info_should_show (old_info) == g_app_info_should_show (new_info) &&
+    strcmp (g_desktop_app_info_get_filename (old),
+            g_desktop_app_info_get_filename (info)) == 0 &&

Clearly not wrong, but do we care?

@@ +135,3 @@
+            g_desktop_app_info_get_filename (info)) == 0 &&
+    strcmp (g_app_info_get_executable (old_info),
+            g_app_info_get_executable (new_info)) == 0 &&

Should use g_strcmp0 (the property is set from the commandline if available, so if you account for the latter being unset, you should do the same here)

@@ +141,3 @@
+            g_app_info_get_name (new_info)) == 0 &&
+    strcmp (g_app_info_get_display_name (old_info),
+            g_app_info_get_display_name (new_info)) == 0 &&

OK, though not actually used

@@ +143,3 @@
+            g_app_info_get_display_name (new_info)) == 0 &&
+    g_strcmp0 (g_app_info_get_description (old_info),
+               g_app_info_get_description (new_info)) == 0 &&

Dto.

@@ +144,3 @@
+    g_strcmp0 (g_app_info_get_description (old_info),
+               g_app_info_get_description (new_info)) == 0 &&
+    (old_icon == new_icon || (old_icon && g_icon_equal (old_icon, new_icon)));

It is odd to handle old_icon == NULL, but not new_icon == NULL. This should really just be g_icon_equal (old_icon, new_icon) though, as the method handles NULL just fine (https://git.gnome.org//browse/glib/tree/gio/gicon.c#n121)

(In reply to Florian Müllner from comment #10)
> Review of attachment 339291 [details] [review] [review]:
> 
> ::: src/shell-app-system.c
> @@ +133,3 @@
> +    g_app_info_should_show (old_info) == g_app_info_should_show (new_info)
> &&
> +    strcmp (g_desktop_app_info_get_filename (old),
> +            g_desktop_app_info_get_filename (info)) == 0 &&
> 
> Clearly not wrong, but do we care?

Yes, this covers the case in which a file with the same identifier (for
example “inkscape.desktop”) gets copied over ~/.local/share/applications
with the intention of overriding some field from the system-installed
version. Of course if some other field changed, the info will be updated
anyway because some other comparison should fail, but if the file in the
user directory is a copy of the system-wide one, it still feels to me
more correct to have the Shell pointing to the version in the user
directory.

I guess we could drop this check, if you would prefer that.

> @@ +135,3 @@
> +            g_desktop_app_info_get_filename (info)) == 0 &&
> +    strcmp (g_app_info_get_executable (old_info),
> +            g_app_info_get_executable (new_info)) == 0 &&
> 
> Should use g_strcmp0 (the property is set from the commandline if available,
> so if you account for the latter being unset, you should do the same here)

Ack, I'll update the patch.

> @@ +141,3 @@
> +            g_app_info_get_name (new_info)) == 0 &&
> +    strcmp (g_app_info_get_display_name (old_info),
> +            g_app_info_get_display_name (new_info)) == 0 &&
> 
> OK, though not actually used

Even if display names are not used by the shell, it seems more correct (and
future proof) to check them. Provided that they are less likely be changed,
maybe it's good to make this comparison the last one.

> @@ +143,3 @@
> +            g_app_info_get_display_name (new_info)) == 0 &&
> +    g_strcmp0 (g_app_info_get_description (old_info),
> +               g_app_info_get_description (new_info)) == 0 &&
> 
> Dto.
> 
> @@ +144,3 @@
> +    g_strcmp0 (g_app_info_get_description (old_info),
> +               g_app_info_get_description (new_info)) == 0 &&
> +    (old_icon == new_icon || (old_icon && g_icon_equal (old_icon,
> new_icon)));
> 
> It is odd to handle old_icon == NULL, but not new_icon == NULL. This should
> really just be g_icon_equal (old_icon, new_icon) though, as the method
> handles NULL just fine
> (https://git.gnome.org//browse/glib/tree/gio/gicon.c#n121)

Good point, I'll change this to just call out to g_icon_equal().

One question: would it make sense to move these comparisons into
g_app_info_equal()? It was shocking to check its code after quite some
debugging to see that it only compares the identifier.

(In reply to Adrian Perez from comment #11)
> (In reply to Florian Müllner from comment #10)
> > Clearly not wrong, but do we care?
> 
> Yes, this covers the case in which a file with the same identifier (for
> example “inkscape.desktop”) gets copied over ~/.local/share/applications
> with the intention of overriding some field from the system-installed
> version.

I get that, the question is why we care where the information comes from (as long as it's identical). Obviously once you start changing the copy, we want to pick up those changes (which is handled by the remaining bits).

But as I said, I don't really mind the check being there ...


> One question: would it make sense to move these comparisons into
> g_app_info_equal()? It was shocking to check its code after quite some
> debugging to see that it only compares the identifier.

That's what Cosimo was getting at in comment #8.

Created attachment 339674 [details] [review]
Properly detect changes in .desktop files (v3)

I have posted a patch for “g_app_info_equals()”, in bug #774302.

I am posting here an updated patch for the Shell. We can wait a bit and
see what feedback the GIO patch gets, or have this in the Shell first
and change later to use “g_app_info_equals()” when/if that gets fixed.

It got no reaction. We should probably land this in the meantime, since we're going to need it on GNOME 3.22 anyway as I doubt that commit is going to be backported in GIO.

(In reply to Michael Catanzaro from comment #14)
> It got no reaction. We should probably land this in the meantime, since
> we're going to need it on GNOME 3.22 anyway as I doubt that commit is going
> to be backported in GIO.

I also think it would be good to have this landed in GNOME Shell while there
is no response from the GIO developers. A version of this patch has been in
eos-desktop since last November, and it has been working perfectly ever since
(https://github.com/endlessm/eos-desktop/pull/196), so there shouldn't be
any risk in landing this in GNOME Shell, too.

Review of attachment 339674 [details] [review]:

(In reply to Adrian Perez from comment #15)
> I also think it would be good to have this landed in GNOME Shell

Fine by me.

::: src/shell-app-system.c
@@ +121,3 @@
 
+  if (!(info = g_desktop_app_info_new (shell_app_get_id (app))))
+    return TRUE;

I had to read this twice to confirm that there's no memory leak here, so I'd prefer different statements for assignment and NULL check.

Created attachment 344068 [details] [review]
Properly detect changes in .desktop files (v4)

New patch version uploaded, addresses Florian's comment by splitting
variable initialization out of the condition of the “if” statement.

In case you are waiting for a comment from me (you already set the patch status), feel free to push to master and gnome-3-22.