GNOME Bugzilla – Bug 635023
Various fixes to the applet library
Last modified: 2010-12-07 14:41:30 UTC
While testing for gnome-shell, I found some various errors in my code.
Created attachment 174631 [details] [review] Various fixes to the applet library Various bug fixes to ensure it works, including using creating the agent before trying to weak ref it, using g_object_notify_by_pspec and not treating a DBusGMethodInvocation as a GObject.
There was an indentation problem (2 spaces instead of a tab), and I'm not so sure that using g_object_notify_by_pspec() is worth the readability issue, but nonetheless committed.
More fixes are needed...
Created attachment 174969 [details] [review] Use BluetoothClient for tracking the default-adapter BluetoothClient already does all the work of tracking what adapter is the default, and whether it is powered or not. There is no need to duplicate that, and also we avoid searching the default adapter more often than needed, with the side effect of unregistering the agent (possibly in the middle of a transaction, making it just fail).
Created attachment 174970 [details] [review] Don't free the request_key and the passkey in agent reply functions Even though those parameters were marked (transfer full), we were freeing the strings outside. Just remove the marking, sink functions are horrible to see. Also, note that for BluetoothAgent, passkey is numeric and pincode is not. It is still the opposite for BluetoothApplet, as it makes more sense in English.
Review of attachment 174969 [details] [review]: ::: lib/bluetooth-client.c @@ +103,3 @@ GtkTreeStore *store; char *default_adapter; + GtkTreeIter *default_adapter_iter; Don't store TreeIters. Store a GtkTreePath if you must keep a reference. @@ +1064,2 @@ if (found == TRUE) { + if (was_default) { What's the point of doing that? We already get bluetoothd telling us that the default adapter changed. @@ +1295,3 @@ + g_object_class_install_property (object_class, PROP_DEFAULT_ADAPTER_DISCOVERABLE, + g_param_spec_boolean ("default-adapter-discoverable", NULL, NULL, + FALSE, G_PARAM_READABLE)); Given that there's already a function to set discoverable, you could as well make it read/write.
Review of attachment 174970 [details] [review]: > Also, note that for BluetoothAgent, passkey is numeric and pincode > is not. It is still the opposite for BluetoothApplet, as it makes > more sense in English. I would rather we made it clear that we follow the naming using in bluetoothd, and used that throughout. Whether or not it makes sense in English.
(In reply to comment #6) > Review of attachment 174969 [details] [review]: > > ::: lib/bluetooth-client.c > @@ +103,3 @@ > GtkTreeStore *store; > char *default_adapter; > + GtkTreeIter *default_adapter_iter; > > Don't store TreeIters. Store a GtkTreePath if you must keep a reference. Ok (but the applet has been storing TreeIters for ages, and this patch is exactly fixing that) > @@ +1064,2 @@ > if (found == TRUE) { > + if (was_default) { > > What's the point of doing that? We already get bluetoothd telling us that the > default adapter changed. We want code using the adapter model to drop it before it is removed from the TreeStore, since using a GtkTreeFilter at that point just segfaults. > > @@ +1295,3 @@ > + g_object_class_install_property (object_class, > PROP_DEFAULT_ADAPTER_DISCOVERABLE, > + g_param_spec_boolean ("default-adapter-discoverable", > NULL, NULL, > + FALSE, G_PARAM_READABLE)); > > Given that there's already a function to set discoverable, you could as well > make it read/write. Ok
(In reply to comment #8) > (In reply to comment #6) > > Review of attachment 174969 [details] [review] [details]: > > > > ::: lib/bluetooth-client.c > > @@ +103,3 @@ > > GtkTreeStore *store; > > char *default_adapter; > > + GtkTreeIter *default_adapter_iter; > > > > Don't store TreeIters. Store a GtkTreePath if you must keep a reference. > > Ok > (but the applet has been storing TreeIters for ages, and this patch is exactly > fixing that) Right. It doesn't mean it was the right way to go, still. > > @@ +1064,2 @@ > > if (found == TRUE) { > > + if (was_default) { > > > > What's the point of doing that? We already get bluetoothd telling us that the > > default adapter changed. > > We want code using the adapter model to drop it before it is removed from the > TreeStore, since using a GtkTreeFilter at that point just segfaults. I'm not sure what you mean here, but actually we should be using a GtkTreeRowReference, so that it's automatically invalidated when the row goes away (we just need to run gtk_tree_row_reference_valid() before doing something with it).
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > Review of attachment 174969 [details] [review] [details] [details]: > > > > > > ::: lib/bluetooth-client.c > > > @@ +103,3 @@ > > > GtkTreeStore *store; > > > char *default_adapter; > > > + GtkTreeIter *default_adapter_iter; > > > > > > Don't store TreeIters. Store a GtkTreePath if you must keep a reference. > > > > Ok > > (but the applet has been storing TreeIters for ages, and this patch is exactly > > fixing that) > > Right. It doesn't mean it was the right way to go, still. Will fix then. > > > @@ +1064,2 @@ > > > if (found == TRUE) { > > > + if (was_default) { > > > > > > What's the point of doing that? We already get bluetoothd telling us that the > > > default adapter changed. > > > > We want code using the adapter model to drop it before it is removed from the > > TreeStore, since using a GtkTreeFilter at that point just segfaults. > > I'm not sure what you mean here, but actually we should be using a > GtkTreeRowReference, so that it's automatically invalidated when the row goes > away (we just need to run gtk_tree_row_reference_valid() before doing something > with it). I mean that the tree model you get with bluetooth_client_get_adapter_model (client, NULL) is no more usable if the adapter is removed from the main tree store. Whatever gtk_tree_model_iter_first returns, it makes gtk_tree_iter_get segfault.
Created attachment 175277 [details] [review] Use BluetoothClient for tracking the default-adapter BluetoothClient already does all the work of tracking what adapter is the default, and whether it is powered or not. There is no need to duplicate that, and also we avoid searching the default adapter more often than needed, with the side effect of unregistering the agent (possibly in the middle of a transaction, making it just fail).
Created attachment 175278 [details] [review] Don't free the request_key and the passkey in agent reply functions Even though those parameters were marked (transfer full), we were freeing the strings outside. Just remove the marking, sink functions are horrible to see. Also, we had the distinction between passkey and pincode reversed. "pincode" is a string, and "passkey" is numeric. Fix that.
Review of attachment 175277 [details] [review]: ::: lib/bluetooth-client.c @@ +1639,3 @@ + * @client: a #BluetoothClient + * + * Gets the default adapter's discoverable status, cached in the adapter model Missing full stop.
Review of attachment 175278 [details] [review]: If we're not going to touch those items, any chance you could make the callbacks use "const char *" instead of simply "char *"?
Comment on attachment 175277 [details] [review] Use BluetoothClient for tracking the default-adapter Pushed with fix.
Created attachment 176008 [details] [review] Don't free the request_key and the passkey in agent reply functions Even though those parameters were marked (transfer full), we were freeing the strings outside. Just remove the marking, sink functions are horrible to see. Also, we had the distinction between passkey and pincode reversed. "pincode" is a string, and "passkey" is numeric. Fix that.