GNOME Bugzilla – Bug 701399
improvements to gnome-bluetooth
Last modified: 2013-06-18 19:37:41 UTC
A few patches to improve gnome-bluetooth.
Created attachment 245788 [details] [review] sendto: update status on transfer completed When all transfers have been completed, show a label saying so.
Created attachment 245789 [details] [review] sendto: update status on transfer completed When all transfers have been completed, show a label saying so.
Created attachment 245790 [details] [review] sendto: remove Sending files label We can change the window title to say "Bluetooth File Transfer" and the remove the label "Sending files via Bluetooth". We can get a smaller window size with this change.
Created attachment 245791 [details] [review] wizard: Hide dialog if the selected device changes If the device the user is trying to pair disappears, we should hide its PIN dialog, otherwise a PIN options dialog from another device will appear.
Created attachment 245792 [details] [review] client: remove unused GError
Created attachment 245794 [details] [review] wizard: make adapter visible while running the wizard
Created attachment 245796 [details] [review] client: do not call set discovering if value is the same We now check if the discovering value we get is equal to the current one. Before this check the code was calling org.bluez.Adapter1.StopDiscovery() with Discovering already set to False
Review of attachment 245792 [details] [review]: Looks good, but the prefix is "lib: " not "client: "
Review of attachment 245794 [details] [review]: This isn't the way I'd like to have this done. The wizard will force the adapter visibility even if it was already set to "on", and if it was turned off and the wizard crashed, it wouldn't be turned to off. The best way to do this would be for BlueZ to have a "SetPoweredForClient()" and automatically turn off visible if the client crashed/went away.
Review of attachment 245796 [details] [review]: See previous patch, I don't think it should be implemented that way.
Review of attachment 245791 [details] [review]: Why is this bad? The options should be reset if you changed the selected device.
Review of attachment 245789 [details] [review]: ::: sendto/main.c @@ +680,3 @@ + gtk_progress_bar_set_text(GTK_PROGRESS_BAR(progress), ""); + + if (file_count == 1) Use ngettext instead.
Review of attachment 245790 [details] [review]: Looks good.
Created attachment 245884 [details] [review] lib: remove unused GError
(In reply to comment #9) > Review of attachment 245794 [details] [review]: > > This isn't the way I'd like to have this done. > The wizard will force the adapter visibility even if it was already set to > "on", and if it was turned off and the wizard crashed, it wouldn't be turned to > off. > > The best way to do this would be for BlueZ to have a "SetPoweredForClient()" > and automatically turn off visible if the client crashed/went away. I agree that BlueZ should track this as it does with Discovering. Discovering stops if all clients that requested it goes away. I thought about this while implementing this. But even if BlueZ supports this it doesn't prevent us from calling turn Discoverable off in this patch. I should work on patches in BlueZ to implement this.
(In reply to comment #10) > Review of attachment 245796 [details] [review]: > > See previous patch, I don't think it should be implemented that way. No sure what you want here, this is a bug fix, we were calling StopDiscovery() when there was no Discovering ongoing and we need to call this in the wizard. After we select the device we want to pair and start its setup.
(In reply to comment #11) > Review of attachment 245791 [details] [review]: > > Why is this bad? The options should be reset if you changed the selected > device. If for some reason the selected devices disappears an new device is selected automatically and if we have the PIN option dialog open the dialog will refer to a device that we may not want to pair. Other than that if we find only one device and it disappears the dialog will still appear if that device disappear and no other device is selected, address == NULL in select_device_changed()
Comment on attachment 245884 [details] [review] lib: remove unused GError (In reply to comment #8) > Review of attachment 245792 [details] [review]: > > Looks good, but the prefix is "lib: " not "client: " Pushed with that fixed.
Created attachment 246546 [details] [review] killswitch: reuse kill state we already got We don't need to call bluetooth_killswitch_get_state again since new_state is storing this value for us.
Created attachment 246548 [details] [review] lib: power the adapter when un-killing adapter Since bluetooth doesn't remember powered state anymore we should Power the device ourselves when we rfkill-unblock the device. this moves some notification around, e.g., we only notify "default-adapter", "default-adapter-powered" and "default-adapter-discoverable" when the adapter is actually powered.
Created attachment 246549 [details] [review] lib: only flag the first adapter a default adapter BlueZ has removed the DefaultAdapter concept so now we use the first adapter plugged as the default adapter.
Created attachment 246550 [details] [review] lib: improve default_adapter_changed() we don't the iter on the tree to get the adapter, get_iter_from_path() just do it for us.
Created attachment 246551 [details] [review] lib: replaces default adapter if the first is removed If the default adapter is removed we need to look for a another one to replace as default adapter.
Created attachment 246552 [details] [review] lib: add bluetooth_client_get_connectable() tells if a device can be connected directly by the user or if only other components can trigger connections. The implementation for BlueZ 4 used to have the same information but based on D-Bus interface available. Now we just rely on the uuids to get the same behavior.
Review of attachment 246546 [details] [review]: _get_state() will compute the state for all the killswitches of the same class. So a killswitch could change state while another (from the hardware platform for example) might override it.
Review of attachment 246548 [details] [review]: ::: lib/bluetooth-client.c @@ +408,3 @@ + NULL, &error); + if (ret == FALSE) { + That linefeed isn't needed. @@ +425,3 @@ BluetoothClientPrivate *priv = BLUETOOTH_CLIENT_GET_PRIVATE(client); GtkTreeIter iter; + gboolean cont, powered = FALSE, found = FALSE; separate lines when you assign values please. @@ +465,3 @@ + g_object_notify (G_OBJECT (client), "default-adapter-discoverable"); + return; + } else { set_powered() } ? And add a comment that in the else case the notifications will be sent when the power status has changed.
Review of attachment 246549 [details] [review]: Fine.
Review of attachment 246549 [details] [review]: Hmm, how does it work when I plug in 2 adapters, and remove the first one I plugged in? Looks to me like the default adapter would be changed.
Review of attachment 246550 [details] [review]: ::: lib/bluetooth-client.c @@ +433,2 @@ + tree_path = gtk_tree_model_get_path (GTK_TREE_MODEL (priv->store), &iter); + priv->default_adapter = gtk_tree_row_reference_new (GTK_TREE_MODEL (priv->store), tree_path); When do you free priv->default_adapter?
Review of attachment 246551 [details] [review]: This will need to be merged into an earlier patch, I don't want to end up with a commit that regresses.
Review of attachment 246552 [details] [review]: Looks fine.
Review of attachment 246548 [details] [review]: ::: lib/bluetooth-client.c @@ +465,3 @@ + g_object_notify (G_OBJECT (client), "default-adapter-discoverable"); + return; + } The if returns, which means we don't need an else here. I'll add the comment.
Comment on attachment 246552 [details] [review] lib: add bluetooth_client_get_connectable() Attachment 246552 [details] pushed as e0a3511 - lib: add bluetooth_client_get_connectable()
Review of attachment 245791 [details] [review]: It's needed when BlueZ removes the device from the list. Blessed be the day we remove that dialogue...
(In reply to comment #29) > Review of attachment 246550 [details] [review]: > > ::: lib/bluetooth-client.c > @@ +433,2 @@ > + tree_path = gtk_tree_model_get_path (GTK_TREE_MODEL (priv->store), &iter); > + priv->default_adapter = gtk_tree_row_reference_new (GTK_TREE_MODEL > (priv->store), tree_path); > > When do you free priv->default_adapter? It is freed when the adapter is removed, or everything is vanished.
Created attachment 246734 [details] [review] ib: add macros for Bluetooth Profiles UUIDs
Created attachment 246735 [details] [review] lib: Replace uuid number by macros in uuid16_to_string()
Created attachment 246736 [details] [review] lib: power the adapter when un-killing adapter Since bluetooth doesn't remember powered state anymore we should Power the device ourselves when we rfkill-unblock the device. This moves some notification around, e.g., we only notify "default-adapter", "default-adapter-powered" and "default-adapter-discoverable" when the adapter is actually powered.
Created attachment 246737 [details] [review] lib: make default adapter work properly again BlueZ has removed the DefaultAdapter concept so now we use the first adapter plugged as the default adapter. In case the default adapter is removed we iter to find a new adapter to replace as default adapter, in case it exists we call default_adapter_changed() to setup it as the new default. We were able to get rid of some loops in the code in favor to a better implementation using get_iter_from_path() to get the desired iter.
Review of attachment 246737 [details] [review]: ::: lib/bluetooth-client.c @@ +459,2 @@ + tree_path = gtk_tree_model_get_path (GTK_TREE_MODEL (priv->store), &iter); + priv->default_adapter = gtk_tree_row_reference_new (GTK_TREE_MODEL (priv->store), tree_path); You're assuming that priv->default_adapter will already be NULL. In this case, add a g_assert (priv->default_adapter); before this. @@ +626,1 @@ + gtk_tree_model_get(GTK_TREE_MODEL(priv->store), &iter, space before bracket. @@ +630,3 @@ + return; + + gtk_tree_row_reference_free (priv->default_adapter); Use g_clear_pointer (); for this.
Created attachment 246738 [details] [review] lib: do not call set discovering if value is the same We now check if the discovering value we get is equal to the current one. Before this check the code was calling org.bluez.Adapter1.StopDiscovery() with Discovering already set to False which leads bluetoothd to return a error to us.
Review of attachment 246734 [details] [review]: ::: lib/bluetooth-utils.h @@ +31,3 @@ G_BEGIN_DECLS +#define BLUETOOTH_UUID_SPP 0x1101 Where's the canonical location for this list?
Review of attachment 246735 [details] [review]: Looks good.
Review of attachment 246736 [details] [review]: The rest looks fine. ::: lib/bluetooth-client.c @@ +422,3 @@ + g_return_val_if_fail (BLUETOOTH_IS_CLIENT (client), FALSE); + + properties = properties_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, I'd rather not add any new sync calls, we've got enough of them already.
Review of attachment 246738 [details] [review]: Fine.
Created attachment 246741 [details] [review] sendto: update status on transfer completed When all transfers have been completed, show a label saying so.
Created attachment 246742 [details] [review] sendto: remove unused error handling
Created attachment 246744 [details] [review] sendto: fix check for current_transfer proxy
(In reply to comment #44) > Review of attachment 246734 [details] [review]: > > ::: lib/bluetooth-utils.h > @@ +31,3 @@ > G_BEGIN_DECLS > > +#define BLUETOOTH_UUID_SPP 0x1101 > > Where's the canonical location for this list? The Bluetooth.org website.
Created attachment 246745 [details] [review] lib: make default adapter work properly again BlueZ has removed the DefaultAdapter concept so now we use the first adapter plugged as the default adapter. In case the default adapter is removed we iter to find a new adapter to replace as default adapter, in case it exists we call default_adapter_changed() to setup it as the new default. We were able to get rid of some loops in the code in favor to a better implementation using get_iter_from_path() to get the desired iter.
Created attachment 246748 [details] [review] lib: power the adapter when un-killing adapter Since bluetooth doesn't remember powered state anymore we should Power the device ourselves when we rfkill-unblock the device. This moves some notification around, e.g., we only notify "default-adapter", "default-adapter-powered" and "default-adapter-discoverable" when the adapter is actually powered.
Created attachment 246749 [details] [review] lib: make default adapter work properly again BlueZ has removed the DefaultAdapter concept so now we use the first adapter plugged as the default adapter. In case the default adapter is removed we iter to find a new adapter to replace as default adapter, in case it exists we call default_adapter_changed() to setup it as the new default. We were able to get rid of some loops in the code in favor to a better implementation using get_iter_from_path() to get the desired iter.
Created attachment 246750 [details] [review] lib: Remove pointless assignment
Created attachment 246763 [details] [review] wizard: remove duplicated get device name
Created attachment 246764 [details] [review] wizard: get target_name from Device proxy If the we wizard is still searching for devices and a incoming pairing request arrives from another device we should get its name to show in the many messages. This works nicely for both incoming and outcoming pairing requests.
Review of attachment 246741 [details] [review]: ::: sendto/main.c @@ +675,3 @@ + "%u transfers complete", + file_count), file_count); + gtk_label_set_text (GTK_LABEL (label_status), complete); g_free (complete);
Review of attachment 246742 [details] [review]: Pochu said that this code would be reinstated, see bug701280 .
Review of attachment 246744 [details] [review]: Yes.
Review of attachment 246748 [details] [review]: ::: lib/bluetooth-client.c @@ +436,3 @@ + g_return_val_if_fail (BLUETOOTH_IS_CLIENT (client), FALSE); + + properties = properties_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, This is still sync though.
Review of attachment 246749 [details] [review]: Looks good.
Review of attachment 246750 [details] [review]: Indeed.
Review of attachment 246763 [details] [review]: Fine.
Review of attachment 246764 [details] [review]: ::: wizard/main.c @@ +197,3 @@ gtk_assistant_set_current_page (window_assistant, PAGE_SETUP); + g_free(target_name); space. Everywhere.
Attachment 246738 [details] pushed as d10e9e4 - lib: do not call set discovering if value is the same Attachment 246741 [details] pushed as d4b0252 - sendto: update status on transfer completed (with the leak fix) Attachment 246744 [details] pushed as e522ab4 - sendto: fix check for current_transfer proxy Attachment 246750 [details] pushed as 502240a - lib: Remove pointless assignment Attachment 246763 [details] pushed as d4f7590 - wizard: remove duplicated get device name
Created attachment 246795 [details] [review] wizard: get target_name from Device proxy If the we wizard is still searching for devices and a incoming pairing request arrives from another device we should get its name to show in the many messages. This works nicely for both incoming and outcoming pairing requests.
Created attachment 246797 [details] [review] wizard: get target_name from Device proxy If the we wizard is still searching for devices and a incoming pairing request arrives from another device we should get its name to show in the many messages. This works nicely for both incoming and outcoming pairing requests.
Comment on attachment 246797 [details] [review] wizard: get target_name from Device proxy Attachment 246797 [details] pushed as 5698fb5 - wizard: get target_name from Device proxy
Created attachment 246813 [details] [review] lib: add macros for Bluetooth Profiles UUIDs This list comes from the Bluetooth SIG official website.
Review of attachment 246813 [details] [review]: Could you please put a link in the code as a comment?
Created attachment 246826 [details] [review] lib: add macros for Bluetooth Profiles UUIDs
Review of attachment 246826 [details] [review]: Looks good.
Created attachment 246827 [details] [review] lib: store Properties proxy in GtkTree too Since it is used more than once in the code we store it to easier access.
Created attachment 246828 [details] [review] lib: power the adapter when un-killing adapter Since bluetooth doesn't remember powered state anymore we should Power the device ourselves when we rfkill-unblock the device. This moves some notification around, e.g., we only notify "default-adapter", "default-adapter-powered" and "default-adapter-discoverable" when the adapter is actually powered.
Review of attachment 246827 [details] [review]: That looks fine.
Review of attachment 246828 [details] [review]: Looks good.
Attachment 246735 [details] pushed as f0c13cb - lib: Replace uuid number by macros in uuid16_to_string()