Created attachment 225736 [details] [review]
keyboard: Convert old libgnomekbd and IBus settings

Review of attachment 225736 [details] [review]:

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +1436,3 @@
+        g_strfreev (options);
+        g_object_unref (libgnomekbd_settings);
+        g_strfreev ((gchar **) g_ptr_array_free (opt_array, FALSE));

This looks slightly odd - why not use g_ptr_array_new_full and have g_ptr_array_free free the elements.

Review of attachment 225736 [details] [review]:

Can I have tests for the conversion functions as well?

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +558,3 @@
+        gchar **engines, **e;
+
+        if (!schema_is_installed ("org.freedesktop.ibus.general"))

Given that both IBus (for org.freedesktop.ibus.general) and libgnomekbd (for org.gnome.libgnomekbd.keyboard) are requirements of the keyboard plugins (right now), is it necessary to check for them being installed?

I guess at least for libgnomekbd, that makes sense for 3.8.

@@ +1394,3 @@
+        GSettingsSchema *schema;
+
+        schema_source = g_settings_schema_source_get_default ();

I would use g_settings_list_schemas() instead. Could you cache whether those schemas are available as well?

Created attachment 225850 [details] [review]
keyboard: Convert old libgnomekbd and IBus settings

--
(In reply to comment #2)
> Review of attachment 225736 [details] [review]:
> +        g_strfreev ((gchar **) g_ptr_array_free (opt_array, FALSE));
>
> This looks slightly odd - why not use g_ptr_array_new_full and have
> g_ptr_array_free free the elements.

Fixed.

(In reply to comment #3)
> Review of attachment 225736 [details] [review]:
> +        if (!schema_is_installed ("org.freedesktop.ibus.general"))
>
> Given that both IBus (for org.freedesktop.ibus.general) and libgnomekbd (for
> org.gnome.libgnomekbd.keyboard) are requirements of the keyboard plugins (right
> now), is it necessary to check for them being installed?
>
> I guess at least for libgnomekbd, that makes sense for 3.8.

I'd prefer to keep checking since we have been telling people which
want to use other IM frameworks to remove the ibus-daemon package from
their systems and, at least in Fedora, that's where the schema lives.

> @@ +1394,3 @@
> +        GSettingsSchema *schema;
> +
> +        schema_source = g_settings_schema_source_get_default ();
>
> I would use g_settings_list_schemas() instead. Could you cache whether those
> schemas are available as well?

Yes, done.

Review of attachment 225850 [details] [review]:

Looks good after that change.

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +155,3 @@
+        const gchar * const *schemas;
+        const gchar * const *s;
+        static GHashTable *cache = NULL;

That would leak. I meant:
self->priv->has_ibus_schema = schema_is_installed(...);
etc. in start()

Created attachment 225895 [details] [review]
keyboard: Convert old libgnomekbd and IBus settings

--

Honestly, a bunch of string comparisons on a code path that normally
is only going to be hit once (on 1st login after upgrade or
installation) doesn't really warrant a cache or even a boolean IMO, so
I just removed it.

Review of attachment 225895 [details] [review]:

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +1488,3 @@
+        gchar *stamp_file_path = NULL;
+
+        stamp_dir_path = g_build_filename (g_get_user_data_dir (), PACKAGE_NAME, NULL);

config_dir() maybe?

@@ +1512,3 @@
+        g_strfreev (options);
+
+        fd = creat (stamp_file_path, 0644);

something like g_file_set_contents(stamp_file_path, "", 0, NULL); ?

(don't forget to remove the fcntl include)

(In reply to comment #7)
> +        stamp_dir_path = g_build_filename (g_get_user_data_dir (),
> PACKAGE_NAME, NULL);
>
> config_dir() maybe?

As noted on IRC, data_dir() is also what gsettings-data-convert uses
for its stamp file.

> @@ +1512,3 @@
> +        g_strfreev (options);
> +
> +        fd = creat (stamp_file_path, 0644);
>
> something like g_file_set_contents(stamp_file_path, "", 0, NULL); ?
>
> (don't forget to remove the fcntl include)

Yup, done. Thanks for the review!

Attachment 225895 [details] pushed as a131ce9 - keyboard: Convert old libgnomekbd and IBus settings