Code review comment for lp://staging/~charlesk/gnome-bluetooth/indicators

Revision history for this message
Charles Kerr (charlesk) wrote :

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.

« Back to merge proposal