GNOME Bugzilla – Bug 658176
Also respect NoDisplay semantics for applications menu
Last modified: 2013-02-20 15:18:26 UTC
Commit 10dcc10 removed the GMENU_TREE_FLAGS_INCLUDE_NODISPLAY flag for the settings tree, I believe it makes sense to also respect NoDisplay semantics for the main application menu. If we want to display an entry for Nautilus and Evince it should be done by modifying their .desktop file (it looks like it has already been done for Nautilus, no trace of NoDisplay=true in there). Such a patch is already carried by Debian <http://anonscm.debian.org/viewvc/pkg-gnome/packages/experimental/gnome-shell/debian/patches/03_hidden_applications.patch>
Created attachment 195624 [details] [review] Also respect NoDisplay semantics for applications menu
Note that ignoring NoDisplay settings leads to the insane duplication of entries in the shell, because the entries from the Debian menu (which should be ignored according to the default configuration) are shown along entries from the XDG menu.
(In reply to comment #2) > Note that ignoring NoDisplay settings leads to the insane duplication of > entries in the shell, because the entries from the Debian menu (which should be > ignored according to the default configuration) are shown along entries from > the XDG menu. Was this not fixed with http://git.gnome.org/browse/gnome-shell/commit/?id=5d138e1b7932daa8e6cba13074478c25a21e6003
(In reply to comment #3) > Was this not fixed with > http://git.gnome.org/browse/gnome-shell/commit/?id=5d138e1b7932daa8e6cba13074478c25a21e6003 I haven’t checked, but I don’t think it takes into account NoDisplay in entire submenus?
(In reply to comment #4) > (In reply to comment #3) > > Was this not fixed with > > http://git.gnome.org/browse/gnome-shell/commit/?id=5d138e1b7932daa8e6cba13074478c25a21e6003 > > I haven’t checked, but I don’t think it takes into account NoDisplay in entire > submenus? No, but this would be a separate patch to appDisplay.js. We still need to use INCLUDE_NODISPLAY so that we get proper application/window tracking with NoDisplay entries, though.
Created attachment 195645 [details] [review] appDisplay: Do not show NoDisplay directories either
Review of attachment 195645 [details] [review]: Looks fine.
Comment on attachment 195645 [details] [review] appDisplay: Do not show NoDisplay directories either Attachment 195645 [details] pushed as 3b6d907 - appDisplay: Do not show NoDisplay directories either
The bug is clearly not fixed in 3.2.1. All entries show up despite being in a NoDisplay directory.
Do you have an up to date version of gnome-menus?
Yup, 3.2.1. The way the patch is written makes me wonder whether it actually works recursively. The problem here is a menu containing several other submenus, and only the toplevel one is hidden.
(In reply to comment #11) > Yup, 3.2.1. > > The way the patch is written makes me wonder whether it actually works > recursively. The problem here is a menu containing several other submenus, and > only the toplevel one is hidden. The patch looks completely fine for me for recursion. Can you debug your problem?
(In reply to comment #1) > Created an attachment (id=195624) [details] [review] > Also respect NoDisplay semantics for applications menu According to the Debian Patch Tracker, this patch is being applied in Debian: http://patch-tracker.debian.org/patch/series/view/gnome-shell/3.2.2.1-1/09-respect-NoDisplay-semantics-for-app-menu.patch This will break application tracking for all apps marked as NoDisplay. Please remove this patch.
Please remove this patch. It is breaking the gnome-shell-extension-prefs tool, as well as multiple other cases. If you see issues with NoDisplay applications accidentally leak into the Applications chooser or other places, please show us where and we'll try to fix them.
Yes, applications in NoDisplay submenus leak. This is why we apply this patch.
Can you give me a screenshot?
Yeah, I've triple-checked the code and we shouldn't even be parsing nodisplay submenus. Can you make sure that dir.get_is_nodisplay() returns true for the submenus you want to make sure are hidden?
A wild guess: We set the NoDisplay=false for the toplevel Debian menu directory, but the actual subdirectories holding the menu entries are not hidden. So maybe gnome-shell just uses the NoDisplay=true property of the lower directories instead of inheriting false from the toplevel directory? I've created a small screencast of alcarte (alacarte-debian-menu.ogv) which correctly shows the Debian menu as disabled/unchecked and if you go Debian/Grafik it is enabled/checked. gnome-shell without 09-respect-NoDisplay-semantics-for-app-menu.patch shows entries from Debian/*. See the attached screencast (gnome-shell-debian-menu.webm) with all the duplicated entries or menu entries without proper icons.
Created attachment 216073 [details] screencast alacarte debian menu
Created attachment 216074 [details] screencast gnome-shell debian menu
As you can see from the gnome-shell screencast, the applications menu without the patch becomes very crowded, super ugly and is barely usable. That's why we can't remove this patch just yet until this issue is fixed in gnome-shell proper.
Thank you. I think I see what the issue is now. get_all() doesn't filter based on a parent NoDisplay. I don't know if that should be fixed in gnome-menus or not. I'm not too happy with the current code, so maybe we could refactor it.
Created attachment 216120 [details] [review] appDisplay: Don't show apps in NoDisplay categories in the All view We explicitly include NoDisplay applications in the ShellAppSystem because we want app tracking for them, but we explicitly filter NoDisplay applications out when showing them to the user because we don't want to show them to the user. We also based our "All" apps view on a flattened list of apps. While we did check for NoDisplay on the app item itself, we didn't check against its parents. Refactor the app display view to not use a separate flat list of applications, but instead a concatenation of all the applications in all the loaded categories. This patch should fix it. Can you test it, without your Debian patch?
Review of attachment 216120 [details] [review]: erm, hold on, this will require some extra work.
Created attachment 216139 [details] [review] wal # Attachment to Bug 658176 - Also respect NoDisplay semantics for applications menu appDisplay: Don't show apps in NoDisplay categories in the All view We explicitly include NoDisplay applications in the ShellAppSystem because we want app tracking for them, but we explicitly filter NoDisplay applications out when showing them to the user because we don't want to show them to the user. We also based our "All" apps view on a flattened list of apps. While we did check for NoDisplay on the app item itself, we didn't check against its parents. Refactor the app display view to not use a separate flat list of applications, but instead a concatenation of all the applications in all the loaded categories. OK, this one should work.
Review of attachment 216139 [details] [review]: LG.
OK, this should be fixed.
(In reply to comment #27) > OK, this should be fixed. I've backported 196f6c2 for 3.4.1 (which we currently have in Debian unstable) and it seems to work fine. Will drop the Debian specific patch which broke the app tracking as soon as possible. Thanks a lot for the fix!
(In reply to comment #27) > OK, this should be fixed. It looks like this issue is not fully fixed, unfortunately. If I go to Activities → Applications, then the .desktop files from the hidden Debian/ menu directory are not shown. As soon as I start searching though, those turn up again (new screencast attached)
Created attachment 216596 [details] screencast gnome-shell menu with patch applied
The patch is taking a little bit longer than expected. Just want to say that it's not ignored and that I'm working on it.
Created attachment 216859 [details] [review] app-system: Clean up imports I'm entirely sorry for this. We don't test recursive directories that much.
Created attachment 216860 [details] [review] app-system: Use g_slist_free_full
Created attachment 216861 [details] [review] app-system: Don't show items with NoDisplay parents in the search
Created attachment 216862 [details] [review] appDisplay: Fix recursive directory NoDisplay testing We were accidentally testing NoDisplay on the wrong directory.
Review of attachment 216861 [details] [review]: Seeing that the patch uses gmenu_tree_entry_get_is_nodisplay_recurse(), shouldn't we have a new gnome-menus release (3.4 and/or 3.5?) and the corresponding pkg-config check for libgnome-menu-3.0 be bumped accordingly?
Review of attachment 216859 [details] [review]: Sure, I guess.
Review of attachment 216860 [details] [review]: Sure.
Review of attachment 216861 [details] [review]: Other than Michael's comments, looks structurually correct. I didn't investigate the magical new function gmenu_tree_entry_get_is_nodisplay_recurse(). Though I do wonder if we could export this hash table, and inside appDisplay.js do a lookup to see whether or not to show the app, instead of checking the GMenu state again.
Review of attachment 216862 [details] [review]: Makes sense.
(In reply to comment #36) > Review of attachment 216861 [details] [review]: > > Seeing that the patch uses gmenu_tree_entry_get_is_nodisplay_recurse(), > shouldn't we have a new gnome-menus release (3.4 and/or 3.5?) and the > corresponding pkg-config check for libgnome-menu-3.0 be bumped accordingly? How do you feel about carrying a patch to gnome-menus for now? 3.5.3 should be released fairly soon.
Attachment 216859 [details] pushed as 3b4ad5c - app-system: Clean up imports Attachment 216860 [details] pushed as 96cdc9c - app-system: Use g_slist_free_full Attachment 216861 [details] pushed as df56ff4 - app-system: Don't show items with NoDisplay parents in the search Attachment 216862 [details] pushed as 0c4692a - appDisplay: Fix recursive directory NoDisplay testing
*** Bug 657094 has been marked as a duplicate of this bug. ***