After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 685717 - Add support for BlueZ 5.0
Add support for BlueZ 5.0
Status: RESOLVED FIXED
Product: gnome-bluetooth
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-bluetooth-general-maint@gnome.bugs
gnome-bluetooth-general-maint@gnome.bugs
Depends on:
Blocks: 682106 689530 694347 700891
 
 
Reported: 2012-10-08 12:45 UTC by Bastien Nocera
Modified: 2013-05-30 14:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch 1 (942 bytes, patch)
2013-05-21 13:26 UTC, Gustavo Padovan
committed Details | Review
Patch 2 (925 bytes, patch)
2013-05-21 13:26 UTC, Gustavo Padovan
committed Details | Review
Patch 3 (824 bytes, patch)
2013-05-21 13:27 UTC, Gustavo Padovan
committed Details | Review
Patch 4 (948 bytes, patch)
2013-05-21 13:27 UTC, Gustavo Padovan
committed Details | Review
lib: Add introspection XML to the BlueZ API's (8.55 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
committed Details | Review
bluetooth-client: Port to BlueZ 5 (59.01 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
agent: Port agent code to BlueZ 5 (18.26 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
wizard: update to the new libgnome-bluetooth (19.27 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
wizard: add callback to Agent's RequestAuthorization() (1.17 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
wizard: hide device filter from the chooser (918 bytes, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
rejected Details | Review
sendto: hide device and category filters from the chooser (863 bytes, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
rejected Details | Review
sendto: filter devices that support Object Push Profile (766 bytes, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
Port bluetooth-sendto to BlueZ 5 (13.44 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
Remove obsolete sources (15.92 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
committed Details | Review
sendto: set the From label as soon as we create the window (2.50 KB, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
Clear the progress bar text when there's an error (823 bytes, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
committed Details | Review
sendto: fix 'Retry' button (874 bytes, patch)
2013-05-21 16:01 UTC, Emilio Pozuelo Monfort
committed Details | Review
sendto: check if we could create the proxy (1.37 KB, patch)
2013-05-21 16:02 UTC, Emilio Pozuelo Monfort
committed Details | Review
WIP: update of bluetooth applet (4.83 KB, patch)
2013-05-21 16:02 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
applet: fix method documentation (856 bytes, patch)
2013-05-23 14:03 UTC, Emilio Pozuelo Monfort
committed Details | Review
applet: port to BlueZ 5 (9.69 KB, patch)
2013-05-23 14:03 UTC, Emilio Pozuelo Monfort
needs-work Details | Review
Bump the SONAME (852 bytes, patch)
2013-05-23 14:10 UTC, Emilio Pozuelo Monfort
committed Details | Review
sendto: port to BlueZ 5 (13.30 KB, patch)
2013-05-23 15:06 UTC, Emilio Pozuelo Monfort
reviewed Details | Review
sendto: split the code to update the From label (1.96 KB, patch)
2013-05-23 15:07 UTC, Emilio Pozuelo Monfort
committed Details | Review
sendto: set the From label as soon as we create the window (1.29 KB, patch)
2013-05-23 15:07 UTC, Emilio Pozuelo Monfort
committed Details | Review
applet: port to BlueZ 5 (9.69 KB, patch)
2013-05-23 16:17 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
sendto: port to BlueZ 5 (13.35 KB, patch)
2013-05-23 17:03 UTC, Emilio Pozuelo Monfort
none Details | Review
sendto: port to BlueZ 5 (14.64 KB, patch)
2013-05-23 18:03 UTC, Emilio Pozuelo Monfort
reviewed Details | Review
bluetooth-client: Port to BlueZ 5 (61.15 KB, patch)
2013-05-23 21:05 UTC, Gustavo Padovan
needs-work Details | Review
agent: Port agent code to BlueZ 5 (18.25 KB, patch)
2013-05-23 21:06 UTC, Gustavo Padovan
needs-work Details | Review
wizard: rename target_ssp to legacypairing (2.80 KB, patch)
2013-05-23 21:07 UTC, Gustavo Padovan
reviewed Details | Review
wizard: update to the new libgnome-bluetooth (15.38 KB, patch)
2013-05-23 21:09 UTC, Gustavo Padovan
reviewed Details | Review
wizard: add callback to Agent's RequestAuthorization() (1.17 KB, patch)
2013-05-23 21:10 UTC, Gustavo Padovan
needs-work Details | Review
wizard: Remove matches/does not matches buttons (4.05 KB, patch)
2013-05-23 22:30 UTC, Gustavo Padovan
none Details | Review
Remove deprecated g_type_init() (635 bytes, patch)
2013-05-23 22:31 UTC, Gustavo Padovan
needs-work Details | Review
Remove deprecated gtk_widget_pop/push_composite_child() (1.73 KB, patch)
2013-05-23 22:31 UTC, Gustavo Padovan
needs-work Details | Review
client: add bluetooth_client_remove_device() (2.65 KB, patch)
2013-05-23 22:32 UTC, Gustavo Padovan
rejected Details | Review
wizard: Remove matches/does not matches buttons (6.81 KB, patch)
2013-05-23 22:43 UTC, Gustavo Padovan
rejected Details | Review
wizard: only enable "PIN options" button if device is selected (1.55 KB, patch)
2013-05-24 00:40 UTC, Gustavo Padovan
committed Details | Review
wizard: remove 0000, 1111 and 1234 default PINs (6.16 KB, patch)
2013-05-24 00:41 UTC, Gustavo Padovan
rejected Details | Review
wizard: remove the "Fixed PIN" label (3.02 KB, patch)
2013-05-24 00:41 UTC, Gustavo Padovan
rejected Details | Review
wizard: Hide dialog if the selected device changes (801 bytes, patch)
2013-05-24 00:42 UTC, Gustavo Padovan
reviewed Details | Review
sendto: port to BlueZ 5 (14.68 KB, patch)
2013-05-24 11:46 UTC, Emilio Pozuelo Monfort
committed Details | Review
wizard: rename target_ssp to legacypairing (2.81 KB, patch)
2013-05-24 19:09 UTC, Gustavo Padovan
needs-work Details | Review
lib: Remove deprecated g_type_init() in test-agent (656 bytes, patch)
2013-05-24 19:10 UTC, Gustavo Padovan
committed Details | Review
bluetooth-client: Port to BlueZ 5 (61.35 KB, patch)
2013-05-24 19:24 UTC, Gustavo Padovan
needs-work Details | Review
wizard: update to the new libgnome-bluetooth (15.23 KB, patch)
2013-05-24 19:28 UTC, Gustavo Padovan
needs-work Details | Review
bluetooth-client: Port to BlueZ 5 (61.50 KB, patch)
2013-05-28 16:58 UTC, Gustavo Padovan
none Details | Review
08d5126c4a7c34af7d60a5c69992fcfb0f25fb51 (3.56 KB, patch)
2013-05-28 16:59 UTC, Gustavo Padovan
none Details | Review
wizard: update to the new libgnome-bluetooth (14.66 KB, patch)
2013-05-28 17:00 UTC, Gustavo Padovan
none Details | Review
wizard: rename target_ssp to legacypairing (3.56 KB, patch)
2013-05-28 17:01 UTC, Gustavo Padovan
committed Details | Review
bluetooth-client: Port to BlueZ 5 (61.50 KB, patch)
2013-05-29 20:20 UTC, Gustavo Padovan
committed Details | Review
agent: Port agent code to BlueZ 5 (18.63 KB, patch)
2013-05-29 20:21 UTC, Gustavo Padovan
committed Details | Review
wizard: update to the new libgnome-bluetooth (14.18 KB, patch)
2013-05-29 20:22 UTC, Gustavo Padovan
committed Details | Review
wizard: add callback to Agent's RequestAuthorization() (1.16 KB, patch)
2013-05-29 20:22 UTC, Gustavo Padovan
committed Details | Review
applet: port to BlueZ 5 (9.86 KB, patch)
2013-05-29 20:23 UTC, Gustavo Padovan
none Details | Review
applet: port to BlueZ 5 (7.39 KB, patch)
2013-05-29 21:13 UTC, Gustavo Padovan
committed Details | Review

Description Bastien Nocera 2012-10-08 12:45:06 UTC
The GetProperties() calls are now on the Properties interface instead.
Comment 1 Robert Ancell 2012-11-04 22:14:36 UTC
Any idea when BlueZ 5.0 is being released?
Comment 2 Johan Hedberg 2012-11-05 06:49:54 UTC
The target is before the end of this year.
Comment 3 Bastien Nocera 2013-01-02 13:10:35 UTC
Porting guide:
http://www.bluez.org/bluez-5-api-introduction-and-porting-guide/
Comment 4 Gustavo Padovan 2013-05-21 13:26:04 UTC
Created attachment 244917 [details] [review]
Patch 1
Comment 5 Gustavo Padovan 2013-05-21 13:26:34 UTC
Created attachment 244918 [details] [review]
Patch 2
Comment 6 Gustavo Padovan 2013-05-21 13:27:04 UTC
Created attachment 244919 [details] [review]
Patch 3
Comment 7 Gustavo Padovan 2013-05-21 13:27:30 UTC
Created attachment 244920 [details] [review]
Patch 4
Comment 8 Emilio Pozuelo Monfort 2013-05-21 16:01:07 UTC
Created attachment 244941 [details] [review]
lib: Add introspection XML to the BlueZ API's
Comment 9 Emilio Pozuelo Monfort 2013-05-21 16:01:11 UTC
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.
Comment 10 Emilio Pozuelo Monfort 2013-05-21 16:01:15 UTC
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.
Comment 11 Emilio Pozuelo Monfort 2013-05-21 16:01:20 UTC
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)
Comment 12 Emilio Pozuelo Monfort 2013-05-21 16:01:24 UTC
Created attachment 244945 [details] [review]
wizard: add callback to Agent's RequestAuthorization()
Comment 13 Emilio Pozuelo Monfort 2013-05-21 16:01:28 UTC
Created attachment 244946 [details] [review]
wizard: hide device filter from the chooser
Comment 14 Emilio Pozuelo Monfort 2013-05-21 16:01:31 UTC
Created attachment 244947 [details] [review]
sendto: hide device and category filters from the chooser
Comment 15 Emilio Pozuelo Monfort 2013-05-21 16:01:35 UTC
Created attachment 244948 [details] [review]
sendto: filter devices that support Object Push Profile
Comment 16 Emilio Pozuelo Monfort 2013-05-21 16:01:39 UTC
Created attachment 244949 [details] [review]
Port bluetooth-sendto to BlueZ 5
Comment 17 Emilio Pozuelo Monfort 2013-05-21 16:01:44 UTC
Created attachment 244950 [details] [review]
Remove obsolete sources
Comment 18 Emilio Pozuelo Monfort 2013-05-21 16:01:48 UTC
Created attachment 244951 [details] [review]
sendto: set the From label as soon as we create the window
Comment 19 Emilio Pozuelo Monfort 2013-05-21 16:01:52 UTC
Created attachment 244953 [details] [review]
Clear the progress bar text when there's an error
Comment 20 Emilio Pozuelo Monfort 2013-05-21 16:01:57 UTC
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.
Comment 21 Emilio Pozuelo Monfort 2013-05-21 16:02:01 UTC
Created attachment 244955 [details] [review]
sendto: check if we could create the proxy
Comment 22 Emilio Pozuelo Monfort 2013-05-21 16:02:06 UTC
Created attachment 244956 [details] [review]
WIP: update of bluetooth applet
Comment 23 Emilio Pozuelo Monfort 2013-05-21 16:09:44 UTC
Comment on attachment 244956 [details] [review]
WIP: update of bluetooth applet

This obviously needs work :-)
Comment 24 Bastien Nocera 2013-05-23 12:45:08 UTC
Review of attachment 244941 [details] [review]:

Looks fine.
Comment 25 Bastien Nocera 2013-05-23 12:54:04 UTC
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?
Comment 26 Bastien Nocera 2013-05-23 13:01:00 UTC
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?
Comment 27 Bastien Nocera 2013-05-23 13:07:56 UTC
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?
Comment 28 Bastien Nocera 2013-05-23 13:08:52 UTC
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.
Comment 29 Bastien Nocera 2013-05-23 13:28:48 UTC
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).
Comment 30 Bastien Nocera 2013-05-23 13:29:22 UTC
Review of attachment 244947 [details] [review]:

Ditto the previous wizard change.
Comment 31 Bastien Nocera 2013-05-23 13:30:41 UTC
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.
Comment 32 Bastien Nocera 2013-05-23 13:35:03 UTC
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.
Comment 33 Bastien Nocera 2013-05-23 13:35:23 UTC
Review of attachment 244950 [details] [review]:

Fine.
Comment 34 Bastien Nocera 2013-05-23 13:37:16 UTC
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.
Comment 35 Bastien Nocera 2013-05-23 13:37:43 UTC
Review of attachment 244953 [details] [review]:

Looks fine.
Comment 36 Bastien Nocera 2013-05-23 13:38:23 UTC
Review of attachment 244954 [details] [review]:

Looks fine.
Comment 37 Bastien Nocera 2013-05-23 13:39:12 UTC
Review of attachment 244955 [details] [review]:

Fine.
Comment 38 Bastien Nocera 2013-05-23 13:40:09 UTC
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.
Comment 39 Bastien Nocera 2013-05-23 13:41:40 UTC
Please also make sure that you correctly prefix the commit messages' subject lines (sendto:, applet:, etc.).
Comment 40 Emilio Pozuelo Monfort 2013-05-23 14:03:54 UTC
Created attachment 245131 [details] [review]
applet: fix method documentation
Comment 41 Emilio Pozuelo Monfort 2013-05-23 14:03:59 UTC
Created attachment 245132 [details] [review]
applet: port to BlueZ 5

Based on an earlier patch by Gustavo Padovan.
Comment 42 Emilio Pozuelo Monfort 2013-05-23 14:06:40 UTC
Comment on attachment 244956 [details] [review]
WIP: update of bluetooth applet

obsoleted by: applet: port to BlueZ 5
Comment 43 Emilio Pozuelo Monfort 2013-05-23 14:10:30 UTC
Created attachment 245134 [details] [review]
Bump the SONAME

The some agent and applet APIs have been changed,
so bump the SONAME accordingly.
Comment 44 Bastien Nocera 2013-05-23 14:24:53 UTC
Review of attachment 245131 [details] [review]:

Fine.
Comment 45 Bastien Nocera 2013-05-23 14:29:32 UTC
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.
Comment 46 Bastien Nocera 2013-05-23 14:29:53 UTC
Review of attachment 245134 [details] [review]:

Yep.
Comment 47 Emilio Pozuelo Monfort 2013-05-23 15:06:19 UTC
Created attachment 245144 [details] [review]
sendto: port to BlueZ 5
Comment 48 Emilio Pozuelo Monfort 2013-05-23 15:07:04 UTC
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.
Comment 49 Emilio Pozuelo Monfort 2013-05-23 15:07:08 UTC
Created attachment 245146 [details] [review]
sendto: set the From label as soon as we create the window
Comment 50 Emilio Pozuelo Monfort 2013-05-23 15:08:11 UTC
(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.
Comment 51 Emilio Pozuelo Monfort 2013-05-23 15:10:56 UTC
(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.
Comment 52 Bastien Nocera 2013-05-23 16:08:01 UTC
(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.
Comment 53 Bastien Nocera 2013-05-23 16:09:58 UTC
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.
Comment 54 Bastien Nocera 2013-05-23 16:11:42 UTC
Review of attachment 245145 [details] [review]:

Looks good.
Comment 55 Bastien Nocera 2013-05-23 16:12:02 UTC
Review of attachment 245146 [details] [review]:

Looks good.
Comment 56 Emilio Pozuelo Monfort 2013-05-23 16:17:13 UTC
Created attachment 245155 [details] [review]
applet: port to BlueZ 5

Based on an earlier patch by Gustavo Padovan.
Comment 57 Bastien Nocera 2013-05-23 16:19:36 UTC
Review of attachment 245155 [details] [review]:

Looks fine.
Comment 58 Emilio Pozuelo Monfort 2013-05-23 17:03:42 UTC
Created attachment 245156 [details] [review]
sendto: port to BlueZ 5
Comment 59 Emilio Pozuelo Monfort 2013-05-23 17:10:55 UTC
Comment on attachment 245156 [details] [review]
sendto: port to BlueZ 5

I'll look at the cancel issue and resubmit.
Comment 60 Emilio Pozuelo Monfort 2013-05-23 18:03:05 UTC
Created attachment 245160 [details] [review]
sendto: port to BlueZ 5
Comment 61 Gustavo Padovan 2013-05-23 18:20:12 UTC
(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.
Comment 62 Gustavo Padovan 2013-05-23 19:55:47 UTC
(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.
Comment 63 Gustavo Padovan 2013-05-23 21:05:55 UTC
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.
Comment 64 Gustavo Padovan 2013-05-23 21:06:31 UTC
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.
Comment 65 Gustavo Padovan 2013-05-23 21:07:53 UTC
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.
Comment 66 Gustavo Padovan 2013-05-23 21:09:18 UTC
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"
Comment 67 Gustavo Padovan 2013-05-23 21:10:24 UTC
Created attachment 245183 [details] [review]
wizard: add callback to Agent's RequestAuthorization()
Comment 68 Gustavo Padovan 2013-05-23 21:31:18 UTC
(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
Comment 69 Gustavo Padovan 2013-05-23 22:30:44 UTC
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.
Comment 70 Gustavo Padovan 2013-05-23 22:31:05 UTC
Created attachment 245198 [details] [review]
Remove deprecated g_type_init()
Comment 71 Gustavo Padovan 2013-05-23 22:31:32 UTC
Created attachment 245199 [details] [review]
Remove deprecated gtk_widget_pop/push_composite_child()
Comment 72 Gustavo Padovan 2013-05-23 22:32:00 UTC
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.
Comment 73 Gustavo Padovan 2013-05-23 22:43:46 UTC
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.
Comment 74 Gustavo Padovan 2013-05-24 00:40:58 UTC
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
Comment 75 Gustavo Padovan 2013-05-24 00:41:28 UTC
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.
Comment 76 Gustavo Padovan 2013-05-24 00:41:56 UTC
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.
Comment 77 Gustavo Padovan 2013-05-24 00:42:39 UTC
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.
Comment 78 Bastien Nocera 2013-05-24 10:11:07 UTC
Review of attachment 245202 [details] [review]:

Security be damned.
Comment 79 Bastien Nocera 2013-05-24 10:13:23 UTC
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.
Comment 80 Bastien Nocera 2013-05-24 10:14:51 UTC
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.
Comment 81 Bastien Nocera 2013-05-24 10:15:55 UTC
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/
Comment 82 Bastien Nocera 2013-05-24 10:43:33 UTC
Review of attachment 245160 [details] [review]:

Looks fine otherwise.

::: sendto/main.c
@@ +895,3 @@
 	gtk_main();
 
+	g_clear_object (&cancellable);

g_cancellable_cancel() first.
Comment 83 Bastien Nocera 2013-05-24 10:44:37 UTC
Review of attachment 245179 [details] [review]:

Still needs-work as per previous review.
Comment 84 Bastien Nocera 2013-05-24 10:46:10 UTC
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.
Comment 85 Bastien Nocera 2013-05-24 10:47:00 UTC
Review of attachment 245180 [details] [review]:

As per previous review.
Comment 86 Bastien Nocera 2013-05-24 10:47:37 UTC
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.
Comment 87 Bastien Nocera 2013-05-24 10:48:20 UTC
Review of attachment 245198 [details] [review]:

Add "lib: " to the commit subject and mention that it's in test-agent.
Comment 88 Bastien Nocera 2013-05-24 10:49:28 UTC
Review of attachment 245204 [details] [review]:

Looks good.
Comment 89 Bastien Nocera 2013-05-24 10:51:41 UTC
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.
Comment 90 Bastien Nocera 2013-05-24 10:52:16 UTC
Review of attachment 245206 [details] [review]:

This is dependent on the previous patch that was rejected.
Comment 91 Bastien Nocera 2013-05-24 10:52:44 UTC
Review of attachment 245207 [details] [review]:

Why is that a problem?
Comment 92 Emilio Pozuelo Monfort 2013-05-24 11:42:13 UTC
(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.
Comment 93 Emilio Pozuelo Monfort 2013-05-24 11:46:41 UTC
Created attachment 245236 [details] [review]
sendto: port to BlueZ 5
Comment 94 Emilio Pozuelo Monfort 2013-05-24 11:49:26 UTC
(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!
Comment 95 Bastien Nocera 2013-05-24 12:10:36 UTC
Review of attachment 245236 [details] [review]:

Looks good.
Comment 96 Gustavo Padovan 2013-05-24 19:09:56 UTC
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.
Comment 97 Gustavo Padovan 2013-05-24 19:10:32 UTC
Created attachment 245262 [details] [review]
lib: Remove deprecated g_type_init() in test-agent
Comment 98 Gustavo Padovan 2013-05-24 19:24:14 UTC
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.
Comment 99 Gustavo Padovan 2013-05-24 19:28:49 UTC
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 100 Emilio Pozuelo Monfort 2013-05-25 10:19:45 UTC
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
Comment 101 Bastien Nocera 2013-05-28 10:52:11 UTC
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.
Comment 102 Bastien Nocera 2013-05-28 10:54:04 UTC
Review of attachment 245262 [details] [review]:

Did you bump the required glib version in another patch?
Comment 103 Bastien Nocera 2013-05-28 11:37:59 UTC
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.
Comment 104 Bastien Nocera 2013-05-28 11:39:00 UTC
Review of attachment 245267 [details] [review]:

See previous comments on the same patch.
Comment 105 Gustavo Padovan 2013-05-28 16:58:57 UTC
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.
Comment 106 Gustavo Padovan 2013-05-28 16:59:33 UTC
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.
Comment 107 Gustavo Padovan 2013-05-28 17:00:24 UTC
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"
Comment 108 Gustavo Padovan 2013-05-28 17:01:31 UTC
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.
Comment 109 Gustavo Padovan 2013-05-29 20:20:34 UTC
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.
Comment 110 Gustavo Padovan 2013-05-29 20:21:27 UTC
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
Comment 111 Gustavo Padovan 2013-05-29 20:22:15 UTC
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"
Comment 112 Gustavo Padovan 2013-05-29 20:22:59 UTC
Created attachment 245589 [details] [review]
wizard: add callback to Agent's RequestAuthorization()
Comment 113 Gustavo Padovan 2013-05-29 20:23:31 UTC
Created attachment 245590 [details] [review]
applet: port to BlueZ 5
Comment 114 Gustavo Padovan 2013-05-29 21:13:35 UTC
Created attachment 245594 [details] [review]
applet: port to BlueZ 5
Comment 115 Bastien Nocera 2013-05-30 11:26:47 UTC
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 116 Bastien Nocera 2013-05-30 11:29:38 UTC
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 117 Bastien Nocera 2013-05-30 11:30:34 UTC
Comment on attachment 245131 [details] [review]
applet: fix method documentation

Attachment 245131 [details] pushed as 0310b28 - applet: fix method documentation
Comment 118 Bastien Nocera 2013-05-30 11:32:08 UTC
Comment on attachment 245471 [details] [review]
wizard: rename target_ssp to legacypairing

Attachment 245471 [details] pushed as fe049dd - wizard: rename target_ssp to legacypairing
Comment 119 Bastien Nocera 2013-05-30 12:26:17 UTC
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 120 Bastien Nocera 2013-05-30 12:28:41 UTC
Comment on attachment 244945 [details] [review]
wizard: add callback to Agent's RequestAuthorization()

Updated patch was posted.
Comment 121 Bastien Nocera 2013-05-30 12:37:32 UTC
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 122 Bastien Nocera 2013-05-30 12:43:01 UTC
Comment on attachment 245594 [details] [review]
applet: port to BlueZ 5

Attachment 245594 [details] pushed as 76c9733 - applet: port to BlueZ 5
Comment 123 Bastien Nocera 2013-05-30 12:46:43 UTC
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()".
Comment 124 Emilio Pozuelo Monfort 2013-05-30 13:12:23 UTC
(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!
Comment 125 Emilio Pozuelo Monfort 2013-05-30 14:38:30 UTC
I have opened bug 701277, bug 701278 and bug 701280 for the remaining issues. Let's close this one!