Created attachment 221370 [details] [review]
gnome-xkb-info: Parse XKB options

And offer public API to get at them.

Review of attachment 221370 [details] [review]:

::: libgnome-desktop/gnome-xkb-info.c
@@ +58,3 @@
 };
 
+typedef struct _Option Option;

Please namespace those.

@@ +65,3 @@
+};
+
+typedef struct _OptionGroup OptionGroup;

Namespace really required (OptionGroup, GOptionGroup?)

@@ +111,3 @@
+  Option *option = data;
+
+  g_return_if_fail (option != NULL);

Either return silently, or assert if it's NULL, depending on whether it might happen or not.

@@ +309,3 @@
+        }
+
+      priv->current_parser_group = g_new0 (OptionGroup, 1);

I'd use g_slice_* functions instead, as you're creating loads of small items.

::: libgnome-desktop/gnome-xkb-info.h
@@ +77,3 @@
+GList          *gnome_xkb_info_get_all_option_groups            (GnomeXkbInfo *self);
+GList          *gnome_xkb_info_get_options_for_group            (GnomeXkbInfo *self,
+                                                                 const gchar  *id);

Why is it "*id" here, and "*group_id" below? Is it the same variable?

Created attachment 221535 [details] [review]
gnome-xkb-info: Parse XKB options

--
(In reply to comment #2)
> Namespace really required (OptionGroup, GOptionGroup?)

Ack.

> @@ +111,3 @@
> +  g_return_if_fail (option != NULL);
>
> Either return silently, or assert if it's NULL, depending on whether it might
> happen or not.

Well, I was just being consistent with what was already there. You
suggested it in bug 676583 comment 6.

I am sure these can never be NULL but...

> ::: libgnome-desktop/gnome-xkb-info.h
> @@ +77,3 @@
> +GList          *gnome_xkb_info_get_all_option_groups            (GnomeXkbInfo
> *self);
> +GList          *gnome_xkb_info_get_options_for_group            (GnomeXkbInfo
> *self,
> +                                                                 const gchar
> *id);
>
> Why is it "*id" here, and "*group_id" below? Is it the same variable?

Renamed it to be clearer.

Created attachment 221536 [details] [review]
gnome-xkb-info: Use glib's slice allocator for small structures

--
(In reply to comment #2)
> I'd use g_slice_* functions instead, as you're creating loads of small items.

Ok, here it is.

FWIW, I've heard people saying that glibc's malloc these days is
better and glib's slice allocator should be deprecated. Anyway, I've
never investigated it.

Review of attachment 221535 [details] [review]:

Did you check that descriptions would get translated? I don't see any calls to bindtextdomain() for example.

Would be good to have a small utility that dumps all the known options in the current locale, just to check that it's working.

Rest of the code looks good.

Created attachment 221697 [details] [review]
gnome-xkb-info: Parse XKB options

--
(In reply to comment #5)
> Did you check that descriptions would get translated? I don't see any calls to
> bindtextdomain() for example.

It uses the g_dgettext() based macro defined at the top of the file.

> Would be good to have a small utility that dumps all the known options in the
> current locale, just to check that it's working.

Sure, here's the same patch plus a change to test-xkb-info exercising
the new API.

Attachment 221536 [details] pushed as 21b7df0 - gnome-xkb-info: Use glib's slice allocator for small structures
Attachment 221697 [details] pushed as 8716011 - gnome-xkb-info: Parse XKB options