GNOME Bugzilla – Bug 690165
shell: replace GMenu loading code with an hardcoded list of panels
Last modified: 2012-12-18 15:47:50 UTC
It has always bugged me that gnome-control-center used libgnome-menus despite having a fixed list of panels. Recently removing dynamic loading prompted me to write this cleanup patch. No functional changes. It should reduce startup time, as we no longer parse each and every .desktop file in standard directories. It also no longer installs inotify watches, which is good because they're a limited resource.
Created attachment 231487 [details] [review] shell: replace GMenu loading code with an hardcoded list of panels Now that we don't allow or load external panels, using libgnome-menu is just overengineering. We can get the same results with less code by keeping a static list of function pointers. This reduces the number of places one needs to patch to add a new panel. Also, this way we avoid registering all types at startup, and if we want we can switch to load panel desktop files in a separate thread.
Review of attachment 231487 [details] [review]: I would rather have a list of plugins declared in configure.ac, and checks done there. This would allow generating the list of sub-directories for plugins/Makefile.am on-the-fly. ::: shell/cc-shell-model.c @@ +148,3 @@ COL_ID, id, COL_PIXBUF, pixbuf, + COL_CATEGORY, (guint)category, why the cast? ::: shell/gnome-control-center.c @@ +38,3 @@ #include "cc-shell-category-view.h" #include "cc-shell-model.h" +#include "cc-panel-loader.h" Where's that file? :)
Created attachment 231490 [details] [review] shell: replace GMenu loading code with an hardcoded list of panels Now that we don't allow or load external panels, using libgnome-menu is just overengineering. We can get the same results with less code by keeping a static list of function pointers. This reduces the number of places one needs to patch to add a new panel. Also, this way we avoid registering all types at startup, and if we want we can switch to load panel desktop files in a separate thread. Heh, sorry, here's the complete patch. Regarding comments: - Computing the list of panels in configure.ac would be interesting but, letting aside naming inconsistencies (ua vs universal-access, goa vs online-accounts...), configure would need to generate a non trivial amount of code. Not saying it's impossible, just not very nice from bash... - The (guint) is just being paranoic, in case the compiler decides for a different enum size.
Review of attachment 231490 [details] [review]: Looks good apart from that. ::: shell/cc-panel-loader.c @@ +97,3 @@ +{ + const char *categories; + char **splitted; split, not splitted. @@ +116,3 @@ + else + { + /* Invalid category. We should warn for anything except No, we shouldn't warn. ::: shell/cc-panel-loader.h @@ +24,3 @@ +#include <glib.h> +#include <glib-object.h> +#include <gio/gio.h> Why GIO? ::: shell/gnome-control-center.c @@ +506,3 @@ gtk_tree_model_get (model, iter, COL_CATEGORY, &category, -1); + return category == filter; Please put brackets around that. @@ +742,2 @@ + /* Add categories */ + add_category_view (shell, CC_CATEGORY_PERSONAL, _("Personal")); Please add a context here.
*** Bug 641146 has been marked as a duplicate of this bug. ***
Created attachment 231810 [details] [review] shell: replace GMenu loading code with an hardcoded list of panels Now that we don't allow or load external panels, using libgnome-menu is just overengineering. We can get the same results with less code by keeping a static list of function pointers. This reduces the number of places one needs to patch to add a new panel. Also, this way we avoid registering all types at startup, and if we want we can switch to load panel desktop files in a separate thread.
Review of attachment 231810 [details] [review]: Looking good.
Attachment 231810 [details] pushed as 0139f68 - shell: replace GMenu loading code with an hardcoded list of panels