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 690165 - shell: replace GMenu loading code with an hardcoded list of panels
shell: replace GMenu loading code with an hardcoded list of panels
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: shell
unspecified
Other All
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
: 641146 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-12-13 16:26 UTC by Giovanni Campagna
Modified: 2012-12-18 15:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
shell: replace GMenu loading code with an hardcoded list of panels (17.79 KB, patch)
2012-12-13 16:26 UTC, Giovanni Campagna
needs-work Details | Review
shell: replace GMenu loading code with an hardcoded list of panels (25.92 KB, patch)
2012-12-13 16:47 UTC, Giovanni Campagna
needs-work Details | Review
shell: replace GMenu loading code with an hardcoded list of panels (25.56 KB, patch)
2012-12-18 15:33 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2012-12-13 16:26:30 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.
Comment 1 Giovanni Campagna 2012-12-13 16:26:39 UTC
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.
Comment 2 Bastien Nocera 2012-12-13 16:34:52 UTC
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? :)
Comment 3 Giovanni Campagna 2012-12-13 16:47:44 UTC
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.
Comment 4 Bastien Nocera 2012-12-18 13:55:47 UTC
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.
Comment 5 Bastien Nocera 2012-12-18 15:03:51 UTC
*** Bug 641146 has been marked as a duplicate of this bug. ***
Comment 6 Giovanni Campagna 2012-12-18 15:33:21 UTC
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.
Comment 7 Bastien Nocera 2012-12-18 15:41:40 UTC
Review of attachment 231810 [details] [review]:

Looking good.
Comment 8 Giovanni Campagna 2012-12-18 15:47:46 UTC
Attachment 231810 [details] pushed as 0139f68 - shell: replace GMenu loading code with an hardcoded list of panels