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

Revision history for this message
Sebastien Bacher (seb128) wrote :

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.

review: Needs Fixing

« Back to merge proposal