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 676583 - Add a class to parse and make XKB xml descriptions available
Add a class to parse and make XKB xml descriptions available
Status: RESOLVED FIXED
Product: gnome-desktop
Classification: Core
Component: libgnome-desktop
unspecified
Other All
: Normal normal
: ---
Assigned To: Desktop Maintainers
Desktop Maintainers
Depends on:
Blocks: 641531 662489 676102
 
 
Reported: 2012-05-22 17:50 UTC by Rui Matos
Modified: 2012-06-01 17:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnome-xkb-info: Added to parse and make XKB xml descriptions available (25.87 KB, patch)
2012-05-22 17:50 UTC, Rui Matos
rejected Details | Review
gnome-xkb-info: Added to parse and make XKB xml descriptions available (25.51 KB, patch)
2012-05-22 18:21 UTC, Rui Matos
none Details | Review
gnome-xkb-info: Added to parse and make XKB xml descriptions available (26.09 KB, patch)
2012-05-22 20:46 UTC, Rui Matos
reviewed Details | Review
gnome-xkb-info: Added to parse and make XKB xml descriptions available (27.63 KB, patch)
2012-05-24 20:44 UTC, Rui Matos
needs-work Details | Review
gnome-xkb-info: Added to parse and make XKB xml descriptions available (27.60 KB, patch)
2012-05-31 17:28 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2012-05-22 17:50:40 UTC
This is useful functionality to, at least, gnome-settings-daemon,
gnome-control-center and gnome-shell. See bug 662489 and bug 676102.
Comment 1 Rui Matos 2012-05-22 17:50:42 UTC
Created attachment 214680 [details] [review]
gnome-xkb-info: Added to parse and make XKB xml descriptions available

xkeyboard-config's xml descriptions contain useful information about
XKB layouts. This class makes it easily available to several core
components in the desktop.
Comment 2 Rui Matos 2012-05-22 18:00:50 UTC
Review of attachment 214680 [details] [review]:

Found a problem here, will attach a fixed one shortly.
Comment 3 Rui Matos 2012-05-22 18:21:49 UTC
Created attachment 214683 [details] [review]
gnome-xkb-info: Added to parse and make XKB xml descriptions available
Comment 4 Rui Matos 2012-05-22 18:37:09 UTC
(In reply to bug 676102#c6)
> Need a test application for the xkb-rules-db code as well, and it needs to be
> valgrinded clean.
> Where's the code to clean up the database's allocated memory? Could you use a
> database object instead, which would make checking the lifetime of the various
> allocated bits of memory much simpler?

Partly done here. I'll add a test later, gnome-desktop doesn't seem to have any tests yet.
Comment 5 Rui Matos 2012-05-22 20:46:33 UTC
Created attachment 214699 [details] [review]
gnome-xkb-info: Added to parse and make XKB xml descriptions available

--
XkbRF_GetNamesProp() allocates memory with libc's strdup() so we
shouldn't then use glib's functions to free it.
Comment 6 Bastien Nocera 2012-05-23 13:29:22 UTC
Review of attachment 214699 [details] [review]:

::: libgnome-desktop/gnome-xkb-info.c
@@ +34,3 @@
+#include "gnome-xkb-info.h"
+
+#ifndef DFLT_XKB_CONFIG_ROOT

Are those still needed?

@@ +70,3 @@
+};
+
+#define PRIV(p) (((GnomeXkbInfo *)(p))->priv)

Don't like that one bit.

@@ +77,3 @@
+free_layout (gpointer data)
+{
+  Layout *layout = data;

g_return_if_fail (data != NULL);

@@ +91,3 @@
+ * @var_defs: (out) (transfer full): location to store a
+ * #XkbRF_VarDefsRec pointer. Use gnome_xkb_info_free_var_defs() to
+ * free it

Missing since. and an explanation as to what this is supposed to do.

@@ +104,3 @@
+
+  *rules = NULL;
+  *var_defs = calloc (1, sizeof (XkbRF_VarDefsRec));

If you're allocating this yourself, and handling the freeing too, g_new0()

@@ +490,3 @@
+    return NULL;
+
+  g_hash_table_foreach (priv->layouts_table, add_name_to_list, &layout_names);

If you used a GList, you could do:
return g_hash_table_get_values (priv->layouts_table);
Comment 7 Rui Matos 2012-05-24 20:44:33 UTC
Created attachment 214895 [details] [review]
gnome-xkb-info: Added to parse and make XKB xml descriptions available

--

I think I addressed all your points.
Comment 8 Bastien Nocera 2012-05-31 14:40:31 UTC
Review of attachment 214895 [details] [review]:

Is it really necessary to parse and load all the translations when they won't be used, in gnome-settings-daemon for example?

::: libgnome-desktop/gnome-xkb-info.c
@@ +30,3 @@
+
+#ifdef GETTEXT_PACKAGE
+#undef GETTEXT_PACKAGE

That is 100% wrong.
Comment 9 Rui Matos 2012-05-31 17:28:59 UTC
Created attachment 215350 [details] [review]
gnome-xkb-info: Added to parse and make XKB xml descriptions available

--
(In reply to comment #8)
> Is it really necessary to parse and load all the translations when they won't
> be used, in gnome-settings-daemon for example?

It's not loading the translations until someone requests them, it's
just loading the english descriptions.

We can add a construct property to not load the descriptions but not
until the xkeyboard-config data gets consistent language metadata
added to all layouts. Right now I'm relying the descriptions to know
which of the various English layouts, for instance, is the one that
should be chosen when queried by language. What I'm doing now is quite
lame though, just returning the one with the shortest description...

> ::: libgnome-desktop/gnome-xkb-info.c
> +#ifdef GETTEXT_PACKAGE
> +#undef GETTEXT_PACKAGE
>
> That is 100% wrong.

I've defined a local macro for this now.
Comment 10 Bastien Nocera 2012-06-01 17:29:17 UTC
Added a test app too, and filed bug 677295 about a leak.

Attachment 215350 [details] pushed as db4d37b - gnome-xkb-info: Added to parse and make XKB xml descriptions available