GNOME Bugzilla – Bug 698486
Rewrite the app system to remove gnome-menus and other things
Last modified: 2014-01-17 14:05:05 UTC
This finishes a branch I started long ago. Ryan eventually wants to make a desktop file cache to improve GNOME's startup speed, but that requires effectively dropping gnome-menus, so let's first get rid of it here. The major change is the solidifying of ignoring NoDisplay for categories. All other major GNOME apps use g_app_info_get_all which does not parse the menu files. Note that appDisplay.js has been modified, but still uses gnome-menus for ease of implementation for now. It's possible that the categories and Sundry grouping might move to a simpler format than the LispXML(tm) definitions we have right now. Some other minor changes have been made to accomodate the switch or take into Ryan's many nitpicks of the gnome-shell code.
Created attachment 242053 [details] [review] app-system: Fix some enum warnings
Created attachment 242054 [details] [review] app-system: Kill an unused warning
Created attachment 242055 [details] [review] app-system: Use g_hash_table_iter_remove glib now provides an iteration-safe way to remove items from the hash table, so use it instead of building a separate list.
Created attachment 242056 [details] [review] app-system: Remove lookup_app_by_tree_entry We want to move away from gnome-menus eventually, so the simple utility method isn't really worth keeping around. Reimplement it in the one place that uses it.
Created attachment 242057 [details] [review] app-system: Remove last real use of shell_app_get_tree_entry We want to replace our GMenuTreeEntries with GDesktopAppInfos
Created attachment 242058 [details] [review] app: Port to be based on GDesktopAppInfo We weren't using the GMenuTreeEntry for anything special anymore, so remove it.
Created attachment 242059 [details] [review] app: Remove the use of g_desktop_app_info_launch_uris_as_manager ... and at the same time, remove tracking windows by launched PID. It will fail working after a Shell restart, and Ryan wants to remove the method as it's racy and not a great idea.
Created attachment 242060 [details] [review] app-system: Remove get_tree Make clients construct their own gmenu tree if they need it.
Created attachment 242061 [details] [review] app-system: Remove known_vendor_prefixes This does remove support for legacy prefixed app infos with subdirs, but since we want to remove support for the menu spec, let's not even bother.
Created attachment 242062 [details] [review] app-system: Remove lookup_app_for_path It's absurdly silly. Just modify the one place that uses it to be better.
Created attachment 242063 [details] [review] app-system: Remove visible_id_to_app Since appDisplay.js makes its own GMenu tree, it's not necessary anymore. This does mean that searches will show apps in NoDisplay categories, but that's an obscure enough edge case not to matter.
Created attachment 242064 [details] [review] appDisplay: Ignore NoDisplay categories This makes us match the native app search.
Created attachment 242065 [details] [review] app-system: Remove use of gnome-menus internally We want to transition to a system in the future where we have a desktop file cache. As we no longer differentiate categories or similar, it no longer makes sense to have app visibility based on categories. Thus, we no longer need to use gnome-menus to list all apps. The potential issue here is reloading all desktop files when new files are created, but this can be dealt with individually. The "All Applications" view still uses gnome-menus.
Created attachment 242066 [details] [review] app-system: Add file monitors to monitor changes to application dirs ourselves Cheaply and easily monitor changes to the application dirs to tell us when a desktop file has been created, changed, or removed. Note that this also not 100% spec compliant -- subdirectories of application dirs aren't recognized and properly prefixed. Given the relative obscurity of this scenario in the specification, and that restarting gnome-shell, I'm fine with the code clarity of not having to recursively set up file monitors.
May I say I disagree with this whole patchset? - A category based menu is still needed in classic mode, so GMenu is a hard dependency of GNOME anyway - NoDisplay categories are heavily used in Debian systems, and removing support for them would cause duplicate applications - All KDE applications install themselves in /usr/share/applications/kde4, so recursive file monitors and known_vendor_prefixes are needed - launch_uris_as_manager is the only way to associate apps that don't have a desktop file, or that use the wrong wm_class; it's not racy at all: yes the app can map the window before we get scheduled from fork(), but we won't get the MapRequest event until long after that - The XML menu spec is a standard that all desktops and ISVs support, having a new GNOME specific thing for folders would not benefit us or anyone
(In reply to comment #15) > May I say I disagree with this whole patchset? > > - A category based menu is still needed in classic mode, so GMenu is a hard > dependency of GNOME anyway And we can use it from GIR, as we do right now with appDisplay. But like some other Classic-only technologies, we can use it from Classic and nothing else. The app system specifically does not need to use GMenu. > - NoDisplay categories are heavily used in Debian systems, and removing support > for them would cause duplicate applications Should we fix this in every app that currently uses g_app_info_get_all()? That doesn't parse the menu spec, so you'll potentially get duplicate applications in e.g. the Notifications panel as well, right? I don't know much about how Debian has their crazy menu stuff set out; but that sounds to me more like an OnlyShowIn/NotShowIn thing. What's their rationale for using NoDisplay categories? > - All KDE applications install themselves in /usr/share/applications/kde4, so > recursive file monitors and known_vendor_prefixes are needed I didn't know about this, but I can re-add it. I'll probably add the APIs to watch the directories in gio, as would be getting a bit more complex. > - launch_uris_as_manager is the only way to associate apps that don't have a > desktop file, or that use the wrong wm_class; it's not racy at all: yes the app > can map the window before we get scheduled from fork(), but we won't get the > MapRequest event until long after that But it breaks when we restart the shell, or if we have apps that are started from any other service (DBus activation, systemd, gnome-terminal, anything which launches "a file"), which we'll have more of in the future. Unless you think Firefox should be changed to tell the shell what to spawn. The race that can happen is that if somebody uses GtkApplication, it quickly starts up, sends a DBus message, and then quits again. It's likely that the child has exited before we have installed the child watch on its PID, meaning we have a rogue PID around. Ryan also wants to make that function basically break in the future, so even if nothing else lands, it's still one thing I think we should land. > - The XML menu spec is a standard that all desktops and ISVs support, having a > new GNOME specific thing for folders would not benefit us or anyone I don't think the XML menu spec is really applicable to the overview in terms of where we're going, where we want users to be able to make folders on their own. I'd rather have that tracked in GSettingsList than trying to manipulate the XML menu spec to do what we want. I maintain alacarte; I know exactly what kind of crazy workarounds it employs to make simple things like reordering items happen, and I don't think it's a good idea. The XML menu spec isn't really an ISV API: Wine and alacarte both want to write to the applications.menu override file, and they're both super careful not to trample on anything else. The way they have to do this is to read the entire menu (/etc/xdg/menus/applications.menu and ~/.local/share/xdg/menus/applications.menu) and try to figure out what it wants to write to not mess up the user's existing system. It's an extremely fragile system, and Wine does it so it can install a submenu in the GNOME 2 menu, which we ignore in GNOME 3. And *if* the ISV manages to write the extremely fragile code that does all that stuff, it won't appear in the overview anyway unless they tweak a default key right now. I'd say that we've already been on life support for supporting much of the menu spec, and it's much better off forgotten about.
JS ERROR: !!! Exception was: TypeError: itemB is null JS ERROR: !!! message = '"itemB is null"' JS ERROR: !!! fileName = '"/opt/gnome/share/gnome-shell/js/ui/appDisplay.js"' JS ERROR: !!! lineNumber = '255' JS ERROR: !!! stack = '"AllView<._compareItems@/opt/gnome/share/gnome-shell/js/ui/appDisplay.js:255 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 AlphabeticalView<.loadGrid@/opt/gnome/share/gnome-shell/js/ui/appDisplay.js:101 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 AppDisplay<._redisplayAllApps@/opt/gnome/share/gnome-shell/js/ui/appDisplay.js:452 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 _runDeferredWork@/opt/gnome/share/gnome-shell/js/ui/main.js:703 _runBeforeRedrawQueue@/opt/gnome/share/gnome-shell/js/ui/main.js:718 @/opt/gnome/share/gnome-shell/js/ui/main.js:727
Giovanni, can you review some of the initial cleanup patches? Ryan has assured me he'll introduce a desktop file cache upstream sometime soon, so I'll be rewriting most of this to use that instead.
Ok, I'll review, but I've seen crashes in libgnome-menus testing this, so we may want to hold on a bit (unfortunately I didn't have time to debug them)
Review of attachment 242053 [details] [review]: ::: src/shell-app-system.c @@ +566,3 @@ { case SHELL_APP_STATE_RUNNING: + case SHELL_APP_STATE_BUSY: If the app goes from running to busy, it's already in the table, no?
Review of attachment 242054 [details] [review]: Yes
Review of attachment 242055 [details] [review]: Nice.
Review of attachment 242056 [details] [review]: ::: js/ui/appDisplay.js @@ +45,3 @@ let entry = iter.get_entry(); + let appInfo = entry.get_app_info(); + let app = appSystem.lookup_app(appInfo.get_id()); This one is broken until we get appInfo.get_id() to respect subdirectories in /usr/share/applications. And that's a blocker for me.
Review of attachment 242057 [details] [review]: Ok
Review of attachment 242058 [details] [review]: ::: src/shell-app.c @@ +895,2 @@ { + g_clear_object (&app->info); Can it happen that info is set twice on the same app? If not, it might be nice to avoid the atomic set, we call this a lot at startup.
Review of attachment 242059 [details] [review]: As noted before, the method is not racy, and it's not a common use case to restart the shell, while it is common to have apps with wrong WM_CLASS. I'm against this.
Review of attachment 242060 [details] [review]: Uhm squash this with the patch that removes internal gmenu usage.
Review of attachment 242061 [details] [review]: Virtually all KDE4 applications have a desktop file id like 'kde4-something.desktop', but they're wm class is just Something, so they're going to regress if you remove this. At least, please extend the hard coded vendor_prefixes with "kde-", "kde4-", "crx-" (chrome) and "wine-".
Review of attachment 242062 [details] [review]: ::: src/shell-window-tracker.c @@ +677,3 @@ appsys = shell_app_system_get_default (); + app = shell_app_system_lookup_app (appsys, basename); + g_free (basename); According to the startup notification spec, the application_id field is a full path name only when the desktop file is in a nonstandard location, so calling get_basename is never useful here.
The rest are not cleanups, they're a full refactoring, and they change behavior too, so I'll leave them for now.
Attachment 242054 [details] pushed as c8792cc - app-system: Kill an unused warning Attachment 242055 [details] pushed as 0fd6ae5 - app-system: Use g_hash_table_iter_remove
Comment on attachment 242057 [details] [review] app-system: Remove last real use of shell_app_get_tree_entry Attachment 242057 [details] pushed as c72ae37 - app-system: Remove last real use of shell_app_get_tree_entry
(In reply to comment #26) > Can it happen that info is set twice on the same app? Yes. If somebody modifies the .desktop file, we'll re-create the GDesktopAppInfo since our file monitor went off, but we won't recreate the ShellApp.
Comment on attachment 242053 [details] [review] app-system: Fix some enum warnings Attachment 242053 [details] pushed as 3a4782c - app-system: Fix some enum warnings
Current Continuous buildmaster is crashing with the following stack, pretty sure it's these changes.
+ Trace 232704
Thread 1 (Thread 0x7ffff7fae900 (LWP 1427))
Fixed by https://git.gnome.org/browse/gnome-shell/commit/?id=78343f483749c3637f873678bbe80ce04c39ed4f