GNOME Bugzilla – Bug 685717
Add support for BlueZ 5.0
Last modified: 2013-05-30 14:38:30 UTC
The GetProperties() calls are now on the Properties interface instead.
Any idea when BlueZ 5.0 is being released?
The target is before the end of this year.
Porting guide: http://www.bluez.org/bluez-5-api-introduction-and-porting-guide/
Created attachment 244917 [details] [review] Patch 1
Created attachment 244918 [details] [review] Patch 2
Created attachment 244919 [details] [review] Patch 3
Created attachment 244920 [details] [review] Patch 4
Created attachment 244941 [details] [review] lib: Add introspection XML to the BlueZ API's
Created attachment 244942 [details] [review] bluetooth-client: Port to BlueZ 5 Port the client part of gnome-bluetooth to BlueZ 5. The new API is a way more simple than the old one, a consequence of this is the diffstat of this patch is the removal of more than 500 lines of code. The new API uses freedesktop DBus ObjectMananer and Properties, thus we don't need to watch on many different interfaces for signals, for example.
Created attachment 244943 [details] [review] agent: Port agent code to BlueZ 5 The biggest difference of the new Agent API is that BlueZ 5 doesn't register one agent per adapter anymore. There is now only one agent, to handle all adapters at the same time and most of changes in this commit are related to that. The new API also introduces a new Agent method, RequestAuthorization(), to give power to the user allow/deny JustWorks pairing, so now bluetooth_agent_set_authorize_func is related to that method and there is new bluetooth_agent_set_authorize_service_func which is sets the callback to the function to authorize services to trying to connect.
Created attachment 244944 [details] [review] wizard: update to the new libgnome-bluetooth After this change the wizard has simpler logic and implementation. The target_ssp global var now became legacypairing, the new name reflects better what this really means. automatic_pincode is now gone, since user_pincode can tell the same thing. the char *pincode is gone, we store pincode only in user_pincode now. The PIN Options button now only appears for legacy pairing since it doesn't make sense for SSP pairing. Let this button there only confuses the user and can lead to errors when pairing (at least in the BlueZ 4 UI the pairing could fail if the user chooses the wrong options)
Created attachment 244945 [details] [review] wizard: add callback to Agent's RequestAuthorization()
Created attachment 244946 [details] [review] wizard: hide device filter from the chooser
Created attachment 244947 [details] [review] sendto: hide device and category filters from the chooser
Created attachment 244948 [details] [review] sendto: filter devices that support Object Push Profile
Created attachment 244949 [details] [review] Port bluetooth-sendto to BlueZ 5
Created attachment 244950 [details] [review] Remove obsolete sources
Created attachment 244951 [details] [review] sendto: set the From label as soon as we create the window
Created attachment 244953 [details] [review] Clear the progress bar text when there's an error
Created attachment 244954 [details] [review] sendto: fix 'Retry' button We were trying to create a session unconditionally but that fails if we already have one.
Created attachment 244955 [details] [review] sendto: check if we could create the proxy
Created attachment 244956 [details] [review] WIP: update of bluetooth applet
Comment on attachment 244956 [details] [review] WIP: update of bluetooth applet This obviously needs work :-)
Review of attachment 244941 [details] [review]: Looks fine.
Review of attachment 244942 [details] [review]: The indentation is to be done again (I didn't mark all the places where it happens). ::: lib/bluetooth-client-private.h @@ +37,1 @@ +gboolean bluetooth_client_pair(BluetoothClient *client, When replacing the code, please add a space between function name and opening bracket. @@ +37,2 @@ +gboolean bluetooth_client_pair(BluetoothClient *client, + const char *device_path, const char *agent, Line the arguments on separate lines with the character after the opening bracket, line up the arguments, and one argument per line. (I know it's not like that, some of that code grew from a different coding style). ::: lib/bluetooth-client.c @@ +53,3 @@ +#define BLUEZ_ADAPTER_INTERFACE "org.bluez.Adapter1" +#define BLUEZ_DEVICE_INTERFACE "org.bluez.Device1" +#define FDO_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties" Line this up please. @@ +237,3 @@ + + gtk_tree_store_set(priv->store, &iter, + BLUETOOTH_COLUMN_NAME, name, -1); What happened there with the indentation? @@ +296,3 @@ + + device = device1_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, + G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, Indentation again. @@ +305,3 @@ + dict = g_variant_lookup_value (variant, BLUEZ_DEVICE_INTERFACE, + G_VARIANT_TYPE_DICTIONARY); Indentation. @@ +429,3 @@ } + + /* Record the new default adapter */ We only do that when they *change*, not every time the properties for an adapter change. @@ +1294,3 @@ devdata->client = g_object_ref (client); + device1_call_pair (DEVICE1(device), Doesn't this only *pair* devices? What about devices that don't use pairing (which we want to connect to and add as known and trusted)? @@ +1452,2 @@ if (connect) { + device1_call_connect(DEVICE1(device), Doesn't just connect the L2CAP, or will it do the magic to connect to the device's services as well? (connecting audio for headsets, input for keyboards, etc.) @@ -2028,3 @@ g_print ("\tType: %s Icon: %s\n", bluetooth_type_to_string (type), icon); g_print ("\tPaired: %s Trusted: %s Connected: %s\n", BOOL_STR(paired), BOOL_STR(trusted), BOOL_STR(connected)); - if (services != NULL) { Why can't we have a list of the advertised services anymore?
Review of attachment 244943 [details] [review]: What's "JustWorks" pairing? Please use the correct terms if there are any. There's problems with the indentation again. ::: lib/bluetooth-agent.c @@ +34,3 @@ +#define BLUEZ_SERVICE "org.bluez" +#define BLUEZ_AGENT_PATH "/org/bluez/agent/gnome" line up with the other definitions. @@ +437,3 @@ + g_object_unref(priv->agent_manager); + priv->agent_manager = NULL; g_clear_pointer (&priv->agent_manager); ::: lib/bluetooth-agent.h @@ +64,2 @@ typedef gboolean (*BluetoothAgentPasskeyFunc) (GDBusMethodInvocation *invocation, + const char *device_path, gpointer data); Please re-do the indentation as mentioned in the previous patch. @@ +73,3 @@ + const char *device_path, gpointer data); +typedef gboolean (*BluetoothAgentAuthorizeServiceFunc) (GDBusMethodInvocation *invocation, + const char *device_path, const char *uuid, Why change the GDBusProxies to device paths? That seems like an unnecessary API break (and would require gnome-shell changes bigger than adding a new callback). ::: lib/test-agent.c @@ +71,3 @@ agent = bluetooth_agent_new(); + bluetooth_agent_set_confirm_func(agent, agent_confirm, NULL); Why change what the test-agent does?
Review of attachment 244944 [details] [review]: > The target_ssp global var now became legacypairing, the new name reflects > better what this really means. It also reverses the meaning, and you don't say that. I would also prefer the change to be made separately > automatic_pincode is now gone, since user_pincode can tell the same thing. Same thing about the changes, and I doubt that's the case, see line 771: if (is_custom_pin) automatic_pincode = FALSE; else automatic_pincode = user_pincode != NULL; If custom_pin is set, automatic_pincode is FALSE whatever the value of user_pincode. > the char *pincode is gone, we store pincode only in user_pincode now. ::: wizard/wizard.ui @@ +294,3 @@ <object class="GtkDialog" id="pin_dialog"> <property name="border_width">5</property> + <property name="title" translatable="yes">Passkey Options</property> You're renaming passkey to PIN everywhere in the UI for no reason. Don't do that. @@ -327,3 @@ - <property name="visible">True</property> - <property name="xalign">0</property> - <property name="label" translatable="yes" comments="Translators: this is a PIN with a set value, such as 1111, or 0000">Fixed PIN</property> Why did you remove this label?
Review of attachment 244945 [details] [review]: Fine apart from indentation. ::: wizard/main.c @@ +304,3 @@ + g_dbus_method_invocation_return_value (invocation, NULL); + + return TRUE; Indentation doesn't match the line above.
Review of attachment 244946 [details] [review]: Why? It's useful, just like the other filtering options in the chooser, in places with loads of Bluetooth devices (such as some offices).
Review of attachment 244947 [details] [review]: Ditto the previous wizard change.
Review of attachment 244948 [details] [review]: As mentioned by e-mail, this won't work if the services list aren't populated (which they might not be for non-paired devices). This also doesn't refresh the device list so devices with obexpush turned off will still show up.
Review of attachment 244949 [details] [review]: ::: sendto/main.c @@ +70,3 @@ static gint64 last_update = 0; +static gchar *get_error_message(GError *error); Space between function name and bracket. Use char, not gchar. @@ +115,3 @@ + while (g_variant_iter_next (&iter, "{&sv}", &key, &value)) { + if (g_str_equal (key, "Status")) { + const gchar *status = g_variant_get_string (value, NULL); Seperate the declaration from the assignment. @@ +139,3 @@ + current_transfer = g_dbus_proxy_new_finish (res, &error); + + if (error) { You need to check for cancelled dbus calls. @@ +141,3 @@ + if (error) { + g_printerr ("Creating the transfer proxy failed: %s\n", + error->message); Why show the error on the command-line if you're already going to tell the user about it? @@ +161,3 @@ + if (error) { + g_printerr ("Creating the transfer failed: %s\n", + error->message); Ditto above. @@ +166,3 @@ + } + + gtk_progress_bar_set_text(GTK_PROGRESS_BAR(progress), NULL); Space before bracket. @@ +175,3 @@ + + g_dbus_proxy_new (conn, + G_DBUS_PROXY_FLAGS_NONE, Indentation.
Review of attachment 244950 [details] [review]: Fine.
Review of attachment 244951 [details] [review]: ::: sendto/main.c @@ +90,2 @@ static void +update_from_label (void) That ugly code comes from on_transfer_properties(). Separate the commits to put it in a function from the additional calls.
Review of attachment 244953 [details] [review]: Looks fine.
Review of attachment 244954 [details] [review]: Looks fine.
Review of attachment 244955 [details] [review]: Fine.
Review of attachment 244956 [details] [review]: ::: applet/bluetooth-applet.c @@ +243,2 @@ if (auth) { + //if (trusted) What's the problem here? @@ +304,3 @@ char *name; char *long_name = NULL; + GDBusProxy *device; Indentation.
Please also make sure that you correctly prefix the commit messages' subject lines (sendto:, applet:, etc.).
Created attachment 245131 [details] [review] applet: fix method documentation
Created attachment 245132 [details] [review] applet: port to BlueZ 5 Based on an earlier patch by Gustavo Padovan.
Comment on attachment 244956 [details] [review] WIP: update of bluetooth applet obsoleted by: applet: port to BlueZ 5
Created attachment 245134 [details] [review] Bump the SONAME The some agent and applet APIs have been changed, so bump the SONAME accordingly.
Review of attachment 245131 [details] [review]: Fine.
Review of attachment 245132 [details] [review]: ::: applet/bluetooth-applet.c @@ +246,3 @@ + error = g_error_new (AGENT_ERROR, AGENT_ERROR_REJECT, + "Authentication request rejected"); + g_dbus_method_invocation_return_gerror (invocation, error); Use g_dbus_method_invocation_return_error() directly. @@ +298,3 @@ + v = g_dbus_proxy_get_cached_property (proxy, "Alias"); + alias = g_variant_dup_string (v, NULL); You're not checking whether v is non-NULL, so I would keep the variants around, and do the long_name creation based on those, rather than dup'ing the strings and freeing them a little bit after. @@ +349,3 @@ + GDBusProxy *device; + + device = bluetooth_client_get_device (self->client, path); Rather than repeating this code in all the callbacks, make device_get_name() take a path.
Review of attachment 245134 [details] [review]: Yep.
Created attachment 245144 [details] [review] sendto: port to BlueZ 5
Created attachment 245145 [details] [review] sendto: split the code to update the From label Move it into its own function as we need to call that from various places.
Created attachment 245146 [details] [review] sendto: set the From label as soon as we create the window
(In reply to comment #34) > Review of attachment 244951 [details] [review]: > > ::: sendto/main.c > @@ +90,2 @@ > static void > +update_from_label (void) > > That ugly code comes from on_transfer_properties(). > Separate the commits to put it in a function from the additional calls. Done.
(In reply to comment #32) > @@ +139,3 @@ > + current_transfer = g_dbus_proxy_new_finish (res, &error); > + > + if (error) { > > You need to check for cancelled dbus calls. This is done by listening to properties-changed and checking the status. If the transfer is cancelled by the remote device, the status will change to "error". I have tested this and it works fine, the only issue is that we can't distinguish between transfer cancelled and other errors to display a useful error message. All the other issues you mentioned are now fixed.
(In reply to comment #51) > (In reply to comment #32) > > @@ +139,3 @@ > > + current_transfer = g_dbus_proxy_new_finish (res, &error); > > + > > + if (error) { > > > > You need to check for cancelled dbus calls. > > This is done by listening to properties-changed and checking the status. As mentioned on IRC, cancelled dbus call != cancelled transfer.
Review of attachment 245144 [details] [review]: Looks good apart from the error checks. ::: sendto/main.c @@ +140,3 @@ + current_transfer = g_dbus_proxy_new_finish (res, &error); + + if (error) { if (current_transfer == NULL) actually. That goes for all the error checks you do in gnome-bluetooth. @@ +158,3 @@ + variant = g_dbus_proxy_call_finish (proxy, res, &error); + + if (error) { Ditto. @@ +206,3 @@ + session = g_dbus_proxy_new_finish (res, &error); + + if (error) { Ditto.
Review of attachment 245145 [details] [review]: Looks good.
Review of attachment 245146 [details] [review]: Looks good.
Created attachment 245155 [details] [review] applet: port to BlueZ 5 Based on an earlier patch by Gustavo Padovan.
Review of attachment 245155 [details] [review]: Looks fine.
Created attachment 245156 [details] [review] sendto: port to BlueZ 5
Comment on attachment 245156 [details] [review] sendto: port to BlueZ 5 I'll look at the cancel issue and resubmit.
Created attachment 245160 [details] [review] sendto: port to BlueZ 5
(In reply to comment #25) > Review of attachment 244942 [details] [review]: > > The indentation is to be done again (I didn't mark all the places where it > happens). > > ::: lib/bluetooth-client-private.h > @@ +37,1 @@ > +gboolean bluetooth_client_pair(BluetoothClient *client, > > When replacing the code, please add a space between function name and opening > bracket. > > @@ +37,2 @@ > +gboolean bluetooth_client_pair(BluetoothClient *client, > + const char *device_path, const char *agent, > > Line the arguments on separate lines with the character after the opening > bracket, line up the arguments, and one argument per line. (I know it's not > like that, some of that code grew from a different coding style). > > ::: lib/bluetooth-client.c > @@ +53,3 @@ > +#define BLUEZ_ADAPTER_INTERFACE "org.bluez.Adapter1" > +#define BLUEZ_DEVICE_INTERFACE "org.bluez.Device1" > +#define FDO_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties" > > Line this up please. > > @@ +237,3 @@ > + > + gtk_tree_store_set(priv->store, &iter, > + BLUETOOTH_COLUMN_NAME, name, > -1); > > What happened there with the indentation? > > @@ +296,3 @@ > + > + device = device1_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, > + > G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, > > Indentation again. > > @@ +305,3 @@ > > + dict = g_variant_lookup_value (variant, BLUEZ_DEVICE_INTERFACE, > + > G_VARIANT_TYPE_DICTIONARY); > > Indentation. All indentation issues were fixed. > > @@ +429,3 @@ > } > + > + /* Record the new default adapter */ > > We only do that when they *change*, not every time the properties for an > adapter change. default_adapter_changed() is only called when the adapter is added, so we only do that once. > > @@ +1294,3 @@ > devdata->client = g_object_ref (client); > > + device1_call_pair (DEVICE1(device), > > Doesn't this only *pair* devices? What about devices that don't use pairing > (which we want to connect to and add as known and trusted)? Yes, this funcion only pair. The code to connect directly to a device is in the wizard now. > > @@ +1452,2 @@ > if (connect) { > + device1_call_connect(DEVICE1(device), > > Doesn't just connect the L2CAP, or will it do the magic to connect to the > device's services as well? (connecting audio for headsets, input for keyboards, > etc.) This is a magic that connect any services you have enabled. > > @@ -2028,3 @@ > g_print ("\tType: %s Icon: %s\n", bluetooth_type_to_string (type), > icon); > g_print ("\tPaired: %s Trusted: %s Connected: %s\n", BOOL_STR(paired), > BOOL_STR(trusted), BOOL_STR(connected)); > - if (services != NULL) { > > Why can't we have a list of the advertised services anymore? Because we don't have ifaces like org.bluez.Audio,Input etc.. anymore. There is a default Profile interface and we connect everything through there. We now rely only on UUIDs to see the available services. That's not a problem since the Services used to rely on UUID too.
(In reply to comment #26) > Review of attachment 244943 [details] [review]: > > What's "JustWorks" pairing? Please use the correct terms if there are any. That is the correct name, JustWorks pairing procedure where both side should automatically accept pairing without asking the user. However, BlueZ ask the users for security reasons, not allowing devices to pair without an authorization. The notification doesn't show any Passkey for example, only if we Allow or Deny. > > There's problems with the indentation again. > > ::: lib/bluetooth-agent.c > @@ +34,3 @@ > > +#define BLUEZ_SERVICE "org.bluez" > +#define BLUEZ_AGENT_PATH "/org/bluez/agent/gnome" > > line up with the other definitions. > > @@ +437,3 @@ > > + g_object_unref(priv->agent_manager); > + priv->agent_manager = NULL; > > g_clear_pointer (&priv->agent_manager); > > ::: lib/bluetooth-agent.h > @@ +64,2 @@ > typedef gboolean (*BluetoothAgentPasskeyFunc) (GDBusMethodInvocation > *invocation, > + const char *device_path, gpointer data); > > Please re-do the indentation as mentioned in the previous patch. > > @@ +73,3 @@ > + const char *device_path, gpointer data); > +typedef gboolean (*BluetoothAgentAuthorizeServiceFunc) (GDBusMethodInvocation > *invocation, > + const char *device_path, const char *uuid, > > Why change the GDBusProxies to device paths? That seems like an unnecessary API > break (and would require gnome-shell changes bigger than adding a new > callback). The wizard code doesn't use the proxy for nothing, so the agent code we create it for no reason. Only the applet needs it and then it uses bluetooth_client_get_device() to get access to the device proxy. The changes in the applet are minimal. We are breaking the API anyway and I think that the API like this makes sense. Do you agree with it? Or want me to redo this? > ::: lib/test-agent.c > @@ +71,3 @@ > agent = bluetooth_agent_new(); > > + bluetooth_agent_set_confirm_func(agent, agent_confirm, NULL); > > Why change what the test-agent does? because the old one was using the pincode notification, and most of the devices out there today use the confirmPassky notification.
Created attachment 245179 [details] [review] bluetooth-client: Port to BlueZ 5 Port the client part of gnome-bluetooth to BlueZ 5. The new API is a way more simple than the old one, a consequence of this is the diffstat of this patch is the removal of more than 500 lines of code. The new API uses freedesktop DBus ObjectMananer and Properties, thus we don't need to watch on many different interfaces for signals, for example.
Created attachment 245180 [details] [review] agent: Port agent code to BlueZ 5 The biggest difference of the new Agent API is that BlueZ 5 doesn't register one agent per adapter anymore. There is now only one agent, to handle all adapters at the same time and most of changes in this commit are related to that. The new API also introduces a new Agent method, RequestAuthorization(), to give power to the user allow/deny JustWorks pairing(when both sides doesn't require user interaction), so now bluetooth_agent_set_authorize_func is related to that method and there is new bluetooth_agent_set_authorize_service_func which is sets the callback to the function to authorize services to trying to connect.
Created attachment 245181 [details] [review] wizard: rename target_ssp to legacypairing legacypairing tells if a device uses LegacyPairing or Secure Simple Pairing, it has the opposite meaning of target_ssp.
Created attachment 245182 [details] [review] wizard: update to the new libgnome-bluetooth After this change the wizard has simpler logic and implementation. automatic_pincode is now gone, since user_pincode can tell the same thing. the char *pincode is gone, we store pincode only in user_pincode now. The PIN options dialog now only the radios that makes sense. for example, for an SSP device the only two option available are "Automatic PIN code" an "Do not Pair"
Created attachment 245183 [details] [review] wizard: add callback to Agent's RequestAuthorization()
(In reply to comment #27) > Review of attachment 244944 [details] [review]: > > > The target_ssp global var now became legacypairing, the new name reflects > > better what this really means. > > It also reverses the meaning, and you don't say that. I would also prefer the > change to be made separately There is a new patch for this now. > > > automatic_pincode is now gone, since user_pincode can tell the same thing. > > Same thing about the changes, and I doubt that's the case, see line 771: > if (is_custom_pin) > automatic_pincode = FALSE; > else > automatic_pincode = user_pincode != NULL; > If custom_pin is set, automatic_pincode is FALSE whatever the value of > user_pincode. I think the old behaviour is preserved in this patch > > > the char *pincode is gone, we store pincode only in user_pincode now. > > ::: wizard/wizard.ui > @@ +294,3 @@ > <object class="GtkDialog" id="pin_dialog"> > <property name="border_width">5</property> > + <property name="title" translatable="yes">Passkey Options</property> > > You're renaming passkey to PIN everywhere in the UI for no reason. Don't do > that. undone > > @@ -327,3 @@ > - <property name="visible">True</property> > - <property name="xalign">0</property> > - <property name="label" translatable="yes" > comments="Translators: this is a PIN with a set value, such as 1111, or > 0000">Fixed PIN</property> > > Why did you remove this label? undone
Created attachment 245197 [details] [review] wizard: Remove matches/does not matches buttons To improve user experience we auto confirm the Passkey on our side, letting the decision to pair or not to the remote device.
Created attachment 245198 [details] [review] Remove deprecated g_type_init()
Created attachment 245199 [details] [review] Remove deprecated gtk_widget_pop/push_composite_child()
Created attachment 245200 [details] [review] client: add bluetooth_client_remove_device() gnome-control-center was accessing the BlueZ API's directly to remove a device and since we are already using libgnome-bluetooth for all others tasks it is a good idea if we create an API for this too.
Created attachment 245202 [details] [review] wizard: Remove matches/does not matches buttons To improve user experience we auto confirm the Passkey on our side, letting the decision to pair or not to the remote device.
Created attachment 245204 [details] [review] wizard: only enable "PIN options" button if device is selected Do not allow the user to configure PIN Options if no device is selected
Created attachment 245205 [details] [review] wizard: remove 0000, 1111 and 1234 default PINs Starting from BlueZ 5.5 we now have a autopair feature for mouse and headsets that have PIN 0000. We can now remove this option from the UI. Since 0000 is the most used, it might make sense remove the 1111 and 1234 options too and let the user configure it through the Custom PIN field.
Created attachment 245206 [details] [review] wizard: remove the "Fixed PIN" label Since we only have the Custom PIN radio, the UI looks better if we remove the Fixed PIN label.
Created attachment 245207 [details] [review] wizard: Hide dialog if the selected device changes If the user is configuring disappears, we should hide its PIN dialog, otherwise a PIN options from another device will appear.
Review of attachment 245202 [details] [review]: Security be damned.
Review of attachment 245181 [details] [review]: Looks fine otherwise. ::: wizard/main.c @@ +510,3 @@ complete = FALSE; + if (automatic_pincode == FALSE && legacypairing == TRUE) { == TRUE isn't a correct check. Check for != FALSE.
Review of attachment 245200 [details] [review]: It was using Bluez directly so I didn't have to add API. I don't really want to see device management APIs in libgnome-bluetooth, which is an end-user application.
Review of attachment 245199 [details] [review]: ::: lib/bluetooth-chooser.c @@ -769,3 @@ GtkWidget *hbox; - gtk_widget_push_composite_child (); You'll need to port them to the composite widget templates if you want to do that: http://blogs.gnome.org/tvb/2013/04/09/announcing-composite-widget-templates/
Review of attachment 245160 [details] [review]: Looks fine otherwise. ::: sendto/main.c @@ +895,3 @@ gtk_main(); + g_clear_object (&cancellable); g_cancellable_cancel() first.
Review of attachment 245179 [details] [review]: Still needs-work as per previous review.
Review of attachment 245182 [details] [review]: ::: wizard/main.c @@ +501,3 @@ + g_free(target_name); + target_name = bluetooth_chooser_get_selected_device_name (selector); Indentation.
Review of attachment 245180 [details] [review]: As per previous review.
Review of attachment 245183 [details] [review]: ::: wizard/main.c @@ +300,3 @@ static gboolean +authorize_callback (GDBusMethodInvocation *invocation, + const char *device_path, Line up the argument names. @@ +305,3 @@ + g_dbus_method_invocation_return_value (invocation, NULL); + + return TRUE; Indentation.
Review of attachment 245198 [details] [review]: Add "lib: " to the commit subject and mention that it's in test-agent.
Review of attachment 245204 [details] [review]: Looks good.
Review of attachment 245205 [details] [review]: How is this going to work with devices that aren't headsets or mice? If we were going to remove those options, I'd rather they *all* went, and ask the user when needed, as part of the pairing, but this doesn't really solve any problems.
Review of attachment 245206 [details] [review]: This is dependent on the previous patch that was rejected.
Review of attachment 245207 [details] [review]: Why is that a problem?
(In reply to comment #31) > Review of attachment 244948 [details] [review]: > > As mentioned by e-mail, this won't work if the services list aren't populated > (which they might not be for non-paired devices). This also doesn't refresh the > device list so devices with obexpush turned off will still show up. Right. Let's keep this out for now. If we can make bluez send services information for every device and not only paired ones (Gustavo said this may be a bluez bug) I'll fix the refresh issue and resubmit.
Created attachment 245236 [details] [review] sendto: port to BlueZ 5
(In reply to comment #82) > Review of attachment 245160 [details] [review]: > > Looks fine otherwise. > > ::: sendto/main.c > @@ +895,3 @@ > gtk_main(); > > + g_clear_object (&cancellable); > > g_cancellable_cancel() first. I didn't add that as I was cancelling fro response_callback() before destroying the dialog and quitting the loop. But this doesn't hurt, so added. Thanks!
Review of attachment 245236 [details] [review]: Looks good.
Created attachment 245261 [details] [review] wizard: rename target_ssp to legacypairing legacypairing tells if a device uses LegacyPairing or Secure Simple Pairing, it has the opposite meaning of target_ssp.
Created attachment 245262 [details] [review] lib: Remove deprecated g_type_init() in test-agent
Created attachment 245266 [details] [review] bluetooth-client: Port to BlueZ 5 Port the client part of gnome-bluetooth to BlueZ 5. The new API is a way more simple than the old one, a consequence of this is the diffstat of this patch is the removal of more than 500 lines of code. The new API uses freedesktop DBus ObjectMananer and Properties, thus we don't need to watch on many different interfaces for signals, for example. In this patch bluetooth_client_pair() has a new parameter to tell if we will pair the device or only connect to it then set it as trusted.
Created attachment 245267 [details] [review] wizard: update to the new libgnome-bluetooth After this change the wizard has simpler logic and implementation. automatic_pincode is now gone, since user_pincode can tell the same thing. the char *pincode is gone, we store pincode only in user_pincode now. The PIN options dialog now only the radios that makes sense. for example, for an SSP device the only two option available are "Automatic PIN code" an "Do not Pair"
Comment on attachment 245262 [details] [review] lib: Remove deprecated g_type_init() in test-agent >- g_type_init(); >- g_type_init is deprecated since 2.36 but was necessary before that, so either bump the configure requirement, or only stop calling it conditionally with a build time check such as #if (!GLIB_CHECK_VERSION (2, 36, 0)) g_type_init (); #endif
Review of attachment 245261 [details] [review]: ::: wizard/main.c @@ +71,3 @@ static guint target_max_digits = 0; static PairingUIBehaviour target_ui_behaviour = PAIRING_UI_NORMAL; +static gboolean legacypairing = FALSE; This will shadow the variable declared in select_device_changed(). @@ -748,2 @@ target_type = bluetooth_chooser_get_selected_device_type (selector); - target_ssp = !legacypairing; legacypairing is an int, not a boolean. The value is -1 when we don't know whether it's available.
Review of attachment 245262 [details] [review]: Did you bump the required glib version in another patch?
Review of attachment 245266 [details] [review]: > a consequence of this is the diffstat of > this patch is the removal of more than 500 lines of code. We don't really need this in the log message. In fact, we really don't need it... There's only a sideways explanation of where the services property went. ::: lib/bluetooth-client-private.h @@ +37,1 @@ +gboolean bluetooth_client_pair(BluetoothClient *client, The comments made previously still apply to this.
Review of attachment 245267 [details] [review]: See previous comments on the same patch.
Created attachment 245468 [details] [review] bluetooth-client: Port to BlueZ 5 Port the client part of gnome-bluetooth to BlueZ 5. The new API is a way more simple than the old one. One of the things that are gone with the BlueZ API is the Services information, now we only rely on UUID to know which profiles are supported. This doesn't affect us much since Services was already relying UUID information. The new API uses freedesktop DBus ObjectMananer and Properties, thus we don't need to watch on many different interfaces for signals, for example.
Created attachment 245469 [details] [review] 08d5126c4a7c34af7d60a5c69992fcfb0f25fb51 wizard: rename target_ssp to legacypairing legacypairing tells if a device uses LegacyPairing or Secure Simple Pairing, it has the opposite meaning of target_ssp.
Created attachment 245470 [details] [review] wizard: update to the new libgnome-bluetooth After this change the wizard has simpler logic and implementation. automatic_pincode is now gone, since user_pincode can tell the same thing. the char *pincode is gone, we store pincode only in user_pincode now. The PIN options dialog now only the radios that makes sense. for example, for an SSP device the only two option available are "Automatic PIN code" an "Do not Pair"
Created attachment 245471 [details] [review] wizard: rename target_ssp to legacypairing legacypairing tells if a device uses LegacyPairing or Secure Simple Pairing, it has the opposite meaning of target_ssp.
Created attachment 245585 [details] [review] bluetooth-client: Port to BlueZ 5 Port the client part of gnome-bluetooth to BlueZ 5. The new API is a way more simple than the old one. One of the things that are gone with the BlueZ API is the Services information, now we only rely on UUID to know which profiles are supported. This doesn't affect us much since Services was already relying UUID information. The new API uses freedesktop DBus ObjectMananer and Properties, thus we don't need to watch on many different interfaces for signals, for example.
Created attachment 245586 [details] [review] agent: Port agent code to BlueZ 5 The biggest difference of the new Agent API is that BlueZ 5 doesn't register one agent per adapter anymore. There is now only one agent, to handle all adapters at the same time and most of changes in this commit are related to that. The new API also introduces a new Agent method, RequestAuthorization(), to give power to the user allow/deny JustWorks pairing(when both sides doesn't require user interaction), so now bluetooth_agent_set_authorize_func is related to that method and there is new bluetooth_agent_set_authorize_service_func which is sets the callback to the function to authorize services to trying to connect
Created attachment 245587 [details] [review] wizard: update to the new libgnome-bluetooth After this change the wizard has simpler logic and implementation. automatic_pincode is now gone, since user_pincode can tell the same thing. the char *pincode is gone, we store pincode only in user_pincode now. The PIN options dialog now only the radios that makes sense. for example, for an SSP device the only two option available are "Automatic PIN code" an "Do not Pair"
Created attachment 245589 [details] [review] wizard: add callback to Agent's RequestAuthorization()
Created attachment 245590 [details] [review] applet: port to BlueZ 5
Created attachment 245594 [details] [review] applet: port to BlueZ 5
Comment on attachment 245262 [details] [review] lib: Remove deprecated g_type_init() in test-agent Attachment 245262 [details] pushed as 777d01d - lib: Remove deprecated g_type_init() in test-agent
Comment on attachment 245204 [details] [review] wizard: only enable "PIN options" button if device is selected Attachment 245204 [details] pushed as ccbc47e - wizard: only enable "PIN options" button if device is selected
Comment on attachment 245131 [details] [review] applet: fix method documentation Attachment 245131 [details] pushed as 0310b28 - applet: fix method documentation
Comment on attachment 245471 [details] [review] wizard: rename target_ssp to legacypairing Attachment 245471 [details] pushed as fe049dd - wizard: rename target_ssp to legacypairing
Attachment 244941 [details] pushed as 154705b - lib: Add introspection XML to the BlueZ API's Attachment 245586 [details] pushed as 1755d1b - agent: Port agent code to BlueZ 5
Comment on attachment 244945 [details] [review] wizard: add callback to Agent's RequestAuthorization() Updated patch was posted.
Attachment 244954 [details] pushed as 15ac497 - sendto: fix 'Retry' button Attachment 245145 [details] pushed as a2e0769 - sendto: split the code to update the From label Attachment 245146 [details] pushed as 3a7f91a - sendto: set the From label as soon as we create the window Attachment 245236 [details] pushed as d7228b9 - sendto: port to BlueZ 5
Comment on attachment 245594 [details] [review] applet: port to BlueZ 5 Attachment 245594 [details] pushed as 76c9733 - applet: port to BlueZ 5
The 2 accepted-commit_now patches don't apply cleanly, please rebase them. Would also be great if you could work on using templates in "Remove deprecated gtk_widget_pop/push_composite_child()".
(In reply to comment #123) > The 2 accepted-commit_now patches don't apply cleanly, please rebase them. They depended on "applet: port to BlueZ 5", which was applied last. Both pushed. Thanks!
I have opened bug 701277, bug 701278 and bug 701280 for the remaining issues. Let's close this one!