Merge lp://staging/~charlesk/gnome-bluetooth/indicators into lp://staging/~ubuntu-desktop/gnome-bluetooth/ubuntu

Thank you for the work, some comment from a first review:

304 ++ const gchar * const * schemas = g_settings_list_schemas ();
305 ++ for (i=0; !schema_exists && schemas && schemas[i]; i++)
306 ++ if (!g_strcmp0 (schema, schemas[i]))
307 ++ schema_exists = TRUE;

what's the point of checking if the schemas exists when it's part of the same source and we know it's being installed? if the schemas is not there the installation is corrupted and we better let the code error out...

286 ++on_settings_visible_changed (GSettings *settings, gchar *key, CcBluetoothPanel * panel)
287 ++{
288 ++ g_return_if_fail (!g_strcmp0 (key, VISIBLE_KEY));

why do you check the key? is the signal connected on another key than visible_key? that seems not required

289 ++
290 ++ GtkToggleButton *toggle = GTK_TOGGLE_BUTTON(panel->priv->indicator_check);
291 ++ gboolean oldval = gtk_toggle_button_get_active (toggle);
292 ++ gboolean newval = g_settings_get_boolean (settings, key);
293 ++ if (oldval != newval)
294 ++ gtk_toggle_button_set_active (toggle, newval);

do we need those checks? what do you try to avoid? if the visibility changed then it's likely that the checkbox state will change, it's also not obvious that all those checks spare any work over calling set_active in any case.

note that instead of connecting the callback to the gsettings value you can probably simplify the code using g_settings_bind() to bind the widget state to the key value

Seb, thanks for the useful feedback. Using g_settings_bind() did in fact remove all that extra code.

Likewise, I've removed the test-for-schema code before calling g_settings_new().

Fixed r64

Thanks, that looks better, small minor comments,questions though:

> ++ else if ((gsettings != NULL) && !g_settings_get_boolean (gsettings, GSETTINGS_VISIBLE_KEY))

the NULL checking is not needed there, g_settings_new() can't return null, it works or the install is broken it exits

> g_clear_object (&self->priv->indicator_settings);

shouldn't g_object_unref() be used rather there?

> ++++ b/applet/com.canonical.indicator.bluetooth.gschema.xml

you should use a .xml.in with _summary and _description keys and list it in POTFILES.in to make the summary and description translatable and the .xml should be generated at build time

Otherwise the changes look good and seem to work fine, the checkbox seems a bit too close from the ui frame in the capplet though and maybe it would be better with some margin but I will let design comment on that

Seb, thanks for the additional feedback; it's all been helpful.

>> ++ else if ((gsettings != NULL) && !g_settings_get_boolean (gsettings, GSETTINGS_VISIBLE_KEY))
> the NULL checking is not needed there, g_settings_new() can't return null, it works or the install is broken it exits

Fixed in r65

>> g_clear_object (&self->priv->indicator_settings);
> shouldn't g_object_unref() be used rather there?

g_clear_object calls g_object_unref() and NULLs the pointer. I suppose NULLing is unnecessary in _finalize() but IMO it's harmless.

>> ++++ b/applet/com.canonical.indicator.bluetooth.gschema.xml
> you should use a .xml.in with _summary and _description keys and list it in POTFILES.in

Great point; fixed in r65

> the checkbox seems a bit too close from the ui frame in the capplet though and maybe it would be better with some margin

Yeah, I think that's true. Adding a 6-pixel margin between them in r66 looks better.

Thanks, that looks great, I will merge it tomorrow. Bonus point if you open a bug on bugzilla.gnome.org about the string changes in case they want to take it.

Small note also, shouldn't gsettingsschema_in_files be added to EXTRA_DIST?

> Bonus point if you open a bug on bugzilla.gnome.org about the string changes in case they want to take it.

https://bugzilla.gnome.org/show_bug.cgi?id=669284

> also, shouldn't gsettingsschema_in_files be added to EXTRA_DIST?

Yep. r67.

Thanks, I'm merging that