Created attachment 196831 [details] [review]
appDisplay: Make the application display unique

Here's a slow fix. The problem is that Brasero is in two categories. The gnome-menus code treats these as two separate directory entries, so when we "flatten" the list, we get two separate application icons. The best way to do this would be to fix up gnome-menus to use the same entry pointer for both categories.

Seems to be related to this bug:
https://bugzilla.gnome.org/show_bug.cgi?id=659366

Once you reduce the number of categories registered at the desktop file (maybe string size), it goes back to normal.

Review of attachment 196831 [details] [review]:

While this hack would work, it's a gross fix.  We should be fixing shell_app_system_get_all().

Note that gnome-menus can't return the same entry for the app (at least right now) because entries have a _get_parent() function, i.e. they occupy a unique location in the tree.

I'm working on the ShellAppSystem fix.

Created attachment 196980 [details] [review]
apps: Unify code for apps/settings loading more

The apps and settings loading code duplicated the part to traverse a
GMenuTree.  Unify this by adding a new function to return a flattened
set.

This will also be useful for a future change to how we store apps -
this way we can look at both the current set of apps and the new set.

Created attachment 196981 [details] [review]
apps: Uniquify application instances explicitly by id

Commit 0af108211c4f5d3511b085313587e8d5541e51bb introduced a
regression where applications that appear in multiple categories were
duplicated in the "All Apps" list, because we switched from
uniquifying on desktop file ID to the GMenuTreeEntry.

Switch back to keeping the set of apps based on ID.  To flesh this
out, we keep the ShellApp instance for a given ID around forever, and
when we're loading new contents, we replace the GMenuTreeEntry inside
the app. That means callers still get new data.

We still keep around the running app list, though we could just
recompute it from the app list now.

*** Bug 659366 has been marked as a duplicate of this bug. ***

(copying Target field from dup)

Review of attachment 196980 [details] [review]:

This was a little hard to look at, at first glance because of the function interleaving in the generated diff.  There's not much you can do about that I guess, though (except maybe reorder where the new functions land so they are in a distinct part of the source from where the old stuff got removed).  The concept of the patch doesn't seem that unreasonable.  It adds an extra transient hash table to the mix (and an extra loop through that hash table), but that's no big deal.  If you need this for the next patch, it makes sense to land.

It does seem like a useful enough function that it really belongs in gnome-menus (using the iter api already there).

Minor nits you can ignore below.

::: src/shell-app-system.c
@@ +261,3 @@
           {
+            GMenuTreeEntry *entry;
+            item = entry = gmenu_tree_iter_get_entry (iter);

should use the cast macro when setting entry so we get type checking.

@@ +264,3 @@
+            /* Key is owned by entry */
+            g_hash_table_insert (entry_set,
+                                 (char*)gmenu_tree_entry_get_desktop_file_id (entry),

So i'm assuming this cast is to cast away the constness.  If you're going to cast anyway, might as well cast to (gpointer), directly, since that's what g_hash_table_insert expects.

@@ +337,3 @@
+      prefix = get_prefix_for_entry (entry);
+      
+      if (prefix

i'd explicitly put != NULL here after prefix, since you do app != NULL lower in the code. I realize this is just existing code getting shuffled around though, so it might be better to leave it alone for clarity.

Review of attachment 196981 [details] [review]:

::: src/shell-app-system.c
@@ +262,3 @@
+            g_hash_table_replace (entry_set,
+                                  (char*)gmenu_tree_entry_get_desktop_file_id (entry),
+                                  gmenu_tree_item_ref (entry));

oh looks like this change actually belongs squashed with the previous patch?

Review of attachment 196981 [details] [review]:

So the patch seems to work okay in brief testing.  I only noticed one problem where you didn't update an access to the runn_apps hash table to reflect the fact it uses a different key now.

::: src/shell-app-system.c
@@ +311,3 @@
   gpointer key, value;
+  GSList *removed_apps = NULL;
+  GSList *removed_iter;

generally lists talk about nodes not iters.

@@ +356,3 @@
+          gmenu_tree_item_ref (old_entry);
+          _shell_app_set_entry (app, entry);
+           * the new data.

have you thought about just doing g_hash_table_steal here?  then the old entry will go away on _set_entry and the old id won't be in the hash anymore.  Just an idea, what you have is fine.

@@ +629,3 @@
       break;
     case SHELL_APP_STATE_STARTING:
       break;

looks like you need to fix up the SHELL_APP_STATE_STOPPING case too

Attachment 196980 [details] pushed as e5e1b52 - apps: Unify code for apps/settings loading more
Attachment 196981 [details] pushed as 3833124 - apps: Uniquify application instances explicitly by id

*** Bug 643149 has been marked as a duplicate of this bug. ***