GNOME Bugzilla – Bug 676583
Add a class to parse and make XKB xml descriptions available
Last modified: 2012-06-01 17:29:22 UTC
This is useful functionality to, at least, gnome-settings-daemon, gnome-control-center and gnome-shell. See bug 662489 and bug 676102.
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.
Review of attachment 214680 [details] [review]: Found a problem here, will attach a fixed one shortly.
Created attachment 214683 [details] [review] gnome-xkb-info: Added to parse and make XKB xml descriptions available
(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.
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.
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);
Created attachment 214895 [details] [review] gnome-xkb-info: Added to parse and make XKB xml descriptions available -- I think I addressed all your points.
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.
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.
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