After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659351 - Duplicate brasero desktop file
Duplicate brasero desktop file
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.1.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 643149 659366 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-09-17 20:43 UTC by Bastien Nocera
Modified: 2013-09-07 01:01 UTC
See Also:
GNOME target: 3.2
GNOME version: ---


Attachments
appDisplay: Make the application display unique (1011 bytes, patch)
2011-09-17 21:05 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
apps: Unify code for apps/settings loading more (7.65 KB, patch)
2011-09-19 18:03 UTC, Colin Walters
committed Details | Review
apps: Uniquify application instances explicitly by id (12.35 KB, patch)
2011-09-19 18:03 UTC, Colin Walters
committed Details | Review

Description Bastien Nocera 2011-09-17 20:43:17 UTC
In the "Applications" overview, on Fedora 16, I can see 2 "Brasero" applications. Both of them are "brasero.desktop" (as checked by monitoring the favorite-apps GSettings key).
Comment 1 Jasper St. Pierre (not reading bugmail) 2011-09-17 21:05:14 UTC
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.
Comment 2 Adriano Moura 2011-09-18 02:19:23 UTC
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.
Comment 3 Colin Walters 2011-09-19 14:46:00 UTC
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.
Comment 4 Colin Walters 2011-09-19 18:03:39 UTC
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.
Comment 5 Colin Walters 2011-09-19 18:03:41 UTC
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.
Comment 6 Colin Walters 2011-09-19 18:05:38 UTC
*** Bug 659366 has been marked as a duplicate of this bug. ***
Comment 7 André Klapper 2011-09-19 18:07:22 UTC
(copying Target field from dup)
Comment 8 Ray Strode [halfline] 2011-09-20 15:33:04 UTC
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.
Comment 9 Ray Strode [halfline] 2011-09-20 15:43:40 UTC
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?
Comment 10 Ray Strode [halfline] 2011-09-20 16:10:54 UTC
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
Comment 11 Colin Walters 2011-09-20 17:58:16 UTC
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
Comment 12 Florian Müllner 2013-09-07 01:01:14 UTC
*** Bug 643149 has been marked as a duplicate of this bug. ***