(In reply to comment #0)
> See http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/Bluetooth
> for details.
> 
> This is the current API being discussed:
> 
> Node: org.gnome.Bluetooth
>  - node /org/gnome/Bluetooth
>  - interface: org.gnome.Bluetooth
> 
> Methods:
>     GetDevices () -> a{s(su)} - returns a dictionary of devices, keys are
> opaque identifiers, values are tuple with user facing name and a bit mask of
> capabilities (0x1: send file, 0x3: browse files, possibly others)

You also need to have something mentioning whether the device is connected, connecting, or disconnecting, and show a menu item to connect or disconnect if it's relevant to that device (eg. it's relevant for mice and keyboards, but not for phones).

You can see a file list in applet/main.c:update_device_list(), look for the "if (action == NULL) {" condition. There are currently 5 additional menu items, not including the connection status, and "connect/disconnect" entries.

>     InvokeAction (s device, u action) - invoke the action specified (it is the
> single capability bit) on the device - if device is empty, ask the user for one

Add a mention that "device" is the Bluetooth address (bdaddr) of the device.

> Properties:
>     Active: b - whether bluetooth is active or not

This isn't good enough. See the various combinations possible in killswitch_state_changed() in the applet. You need to show when Bluetooth isn't available because of killswitches.

>     Visible: b - whether this computer is visible

Read-write property.

>     GlobalCapabilities: u - capabilities globally supported by the adapter
> (should be shown in main menu)

This would correspond to whether or not the "adapter-action-group" is shown, see applet/popup-menu.ui.

> Signals:
>     DeviceListChanged () - reread device list
>     PropertiesChanged (a{sv} property_new_values)

You're missing API to handle the pairing agent registration. This could potentially be implemented directly in gnome-shell, but I'd rather only the interaction be done in gnome-shell, to avoid leaking too much of Bluetooth implementation details into the shell itself.

There are 6 separate "pairing" requests that need to be handled (in UI) by the shell, before the answers are passed back to the applet.

From discussion on that wiki page, I thought that you wanted to avoid a DBus API, since the applet is just presentation code.
In fact, with the right amount of introspection, rewriting the indicator in JS is straightforward, and could be cleaner and faster than using DBus (for one, you have one daemon less in every session).

Actually, it may be that the solution is to rewrite the indicator in JS, but keep it in the gnome-bluetooth tree, and sort of "dinamically load" it at startup.
(Just need to install it under $(datadir)/gnome-shell/js/ui/status)

(In reply to comment #2)
> From discussion on that wiki page, I thought that you wanted to avoid a DBus
> API, since the applet is just presentation code.

The applet isn't just presentation code, which is the problem. Most of it is presentation, the rest of it is tricky, and somewhat fragile working around the kernel and bluetoothd behaviours.

FWIW, we had the discussion with Owen on IRC yesterday.

> In fact, with the right amount of introspection, rewriting the indicator in JS
> is straightforward, and could be cleaner and faster than using DBus (for one,
> you have one daemon less in every session).

Except that you'd want me to make public things that I don't want public.

> Actually, it may be that the solution is to rewrite the indicator in JS, but
> keep it in the gnome-bluetooth tree, and sort of "dinamically load" it at
> startup.

If you want to rewrite it, don't expect me to help, writing it once, and then maintaining it is more than enough.

> (Just need to install it under $(datadir)/gnome-shell/js/ui/status)

Turns out I changed my mind. The whole point of the exercise is to avoid reimplementing all the work-arounds, and tricks that are in the applet. The D-Bus API would be minimal impact, and probably more straight forward to test without needing the shell (which means I'd be able to test it on more platforms as well).

If you're still interested in working on this, could you please make corrections to the D-Bus API?

I see your points, and partly agree (it maybe that I haven't read the code enough)... so the DBus API needs to be:

Methods:

GetDevices() -> a{s(suu)}
returns dictionary from bdaddr to tuple with name, connected status (enumeration: 0 = disconnected, 1 = connected, 2 = connecting) and capability bit mask, whose values are
0x1: connect/disconnect
0x2: send to
0x4: browse file
0x8: open sound prefs
0x16: open keyboard prefs
0x32: open mouse prefs
(as you see, the API is designed around avoiding to expose the type of device)

InvokeAction(s bdaddr, u cap_bit)
as above

AuthResponse(s bdaddr, b success)
ConfirmResponse(s bdaddr, b success)
invoked after signal AuthRequest (resp. ConfirmRequest), to report the user selection

PinResponse(s bdaddr, s pin)
invoked after signal PinRequest, to give the PIN

Properties:

Visible: b - read/write
KillswitchStatus: u (enum with 0 = hard blocked, 1 = soft blocked, 2 = unblocked) - read/write (if not hard blocked)
GlobalCapabilities: u
I see that in current bluetooth applet it is all-or-nothing (and only obex), but it may be more future-proof to leave this as a bitmask

Signals:

DeviceListChanged()
PropertiesChanged(a{sv} new_values)

AuthRequest(s bdaddr, s name, s long_name)
ConfirmRequest(s bdaddr, s name, s long_name, s pin)
PinRequest(s bdaddr, s name, s long_name)
these expose the corresponding agent UI

AgentRequestCancelled(s bdaddr)

Actually, I thought on this later... if gnome-bluetooth is running as a session daemon, it could implement the agents directly (they would use Gtk dialogs anyway), leaving only the status area indicator in the shell, and making the API cleaner and smaller.

(In reply to comment #5)
> Actually, I thought on this later... if gnome-bluetooth is running as a session
> daemon, it could implement the agents directly (they would use Gtk dialogs
> anyway), leaving only the status area indicator in the shell, and making the
> API cleaner and smaller.

That'd be the way until those are actually implemented in gnome-shell, as we want a different UI for system (non-application) dialogues (like the logout dialogue). See:
http://live.gnome.org/GnomeShell/Design/Whiteboards/SystemDialogs

Good. In that case, it is better to implement the agents in the Shell from the start (and use stolen GtkBuilder files if we have nothing better)

(In reply to comment #7)
> Good. In that case, it is better to implement the agents in the Shell from the
> start (and use stolen GtkBuilder files if we have nothing better)

I think it would be better if the applet kept the smarts about that. There are a bunch of requests that we should handle internally without nagging the user. Just ignore that whole part for now, and I'll try to come up with something better in the future.

Just an example of the things we want to do in the agent:
- Handle keyboards and mice in a special way
- Provide better integration with PulseAudio for devices asking to play music back on the computer
- Disallow any access to a few services that we think should require pairing (eg. try accessing the computer's address book without being paired).

Something I was thinking recently: another DBus server introduces a lot of overhead, placing itself between the shell and the BlueZ stack. And most of time, it would just propagate calls to the GnomeBluetooth library.
So, what about a GnomeBluetoothApplet private introspected library, exposing an API even more high-level than the proposed DBus?
Then both legacy bluetooth-applet and gnome-shell indicator could use the library to expose the bluetooth UI.

(In reply to comment #9)
> Something I was thinking recently: another DBus server introduces a lot of
> overhead, placing itself between the shell and the BlueZ stack. And most of
> time, it would just propagate calls to the GnomeBluetooth library.
> So, what about a GnomeBluetoothApplet private introspected library, exposing an
> API even more high-level than the proposed DBus?
> Then both legacy bluetooth-applet and gnome-shell indicator could use the
> library to expose the bluetooth UI.

I really don't see how you could implement that cleanly enough, but feel free to write the code and try it out for yourself. I still don't like the idea of exposing internals to the world, but I can I can make do with it.

What world? The library would be private and privately introspected (like libmutterprivate or libshell, for example).
I'll try reorganizing bluetooth-applet, to see what I end up with.

While reimplementing the agent for libgnome-bluetooth BluetoothApplet, I discovered that org.bluez.Agent.CancelRequest is inusable, since the method does not specify which device cancelled its request, thus the agent can't tell which request to cancel.

Also, the implementation is buggy, as it relies on the adapter path to be the key inside the input list, but adapter used is always the default one at method invocation time, not the default when making the request.
Consider the following:

1) device asks for pairing to Adapter One, which is the default
2) org.bluez.Agent.AuthRequest is called
3) input_data->path is set to "/org/bluez/<pid>/hci0", dialog is shown
4) mainloop in bluetooth-applet runs
5) default adapter changes, now is Adapter Two
6) default_adapter_changed() is called, changes BluetoothAgent->cancel_data to be the DBusGProxy for the new adapter
7) pairing request is cancelled
8) cancel_request() is called, with the DBusGProxy for Adapter Two
9) path is /org/bluez/<pid>/hci1, cannot be found in input_list
10) correct reply to org.bluez.Agent.AuthRequest is not given, memory is leaked

I forgot to add, that for now I will just cancel any pending request upon org.bluez.Agent.CancelRequest(), and so should do the front end using the library.

Created attachment 169962 [details] [review]
Refactor bluetooth-applet as a library

Turn all non UI components into an introspected BluetoothApplet class
in libgnome-bluetooth.la so that GNOME Shell can use it for the native
indicator.
Also, includes some modifications to make it work with latest gobject-
introspection (which is stricter wrt annotations and will (skip)
anything it is unsure about).

Created attachment 170506 [details] [review]
Refactor bluetooth-applet as a library

Some bugs fixed, as I have now tested it with a the corresponding Shell part.

Review of attachment 170506 [details] [review]:

I don't want to see the applet's code exported in the main library for 3rd-party apps developers.

I'll review the applet code separately, but starting with using C-style comments, and not C++ style would be good.

The generic introspection fixes should also be separated from the main patch.

::: configure.ac
@@ +190,3 @@
 AC_SUBST(GLIB_GENMARSHAL)
 
+GOBJECT_INTROSPECTION_CHECK([0.9.5])

Is 0.9.3 not enough?

::: lib/Makefile.am
@@ +64,3 @@
 	bluetooth-enums.h				\
+	bluetooth-plugin.h				\
+	applet/bluetooth-applet.h

I'd rather the applet code was either separate. It would statically link against libgnome-bluetooth.la, as is currently done, so that private functions are only accessed from within the applet library's code.

::: lib/bluetooth-chooser-button.h
@@ +32,3 @@
 typedef struct _BluetoothChooserButton BluetoothChooserButton;
 
+typedef struct _BluetoothChooserButtonClass BluetoothChooserButtonClass;

Could you please make those changes separately?

::: lib/bluetooth-client.h
@@ +81,3 @@
+					      const char *device,
+					      BluetoothClientConnectFunc func,
+					      gpointer data);

Why is all of this necessary? We try hard not to leak implementation details to application writers.

::: lib/marshal.list
@@ +6,3 @@
 VOID:STRING,STRING,STRING
+
+#for the applet

As I mentioned, I'd rather the applet library be separate.

Created attachment 171357 [details] [review]
Fix introspection

Fix generation of .gir files to work with latest gobject-introspection,
including annotations where necessary.

Created attachment 171358 [details] [review]
Refactor bluetooth-applet as a library

Take out non UI parts of bluetooth-applet and put them in a private
library, which will be used by gnome-shell (and therefore is installed
in gnome-shell private prefix).

(In reply to comment #17)
> Created an attachment (id=171357) [details] [review]
> Fix introspection

Some of those fixes were already in bug 630024, which I just merged.

Fixed up and committed, let me know if I made any mistakes in the merge.

commit fd89195dc3173b37e35299b07e673b3ec0253647
Author: Giovanni Campagna <scampa.giovanni@gmail.com>
Date:   Sun Sep 26 17:47:13 2010 +0200

    Fix introspection
    
    Fix generation of .gir files to work with latest gobject-introspection,
    including annotations where necessary.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=626759

Could you make another patch to avoid trying to create bindings for the plugin system. It only supports C language plugins.

bluetooth-plugin.h:16: Warning: GnomeBluetooth: symbol='GbtPluginInfo': Skipping foreign identifier 'GbtPluginInfo' from namespace GObject
bluetooth-plugin.h:17: Warning: GnomeBluetooth: symbol='GbtPlugin': Skipping foreign identifier 'GbtPlugin' from namespace GObject
<unknown>:: Warning: GnomeBluetooth: Skipping foreign identifier 'GbtPluginInfo' from namespace GObject
<unknown>:: Warning: GnomeBluetooth: Skipping foreign identifier 'GbtPlugin' from namespace GObject

Review of attachment 171358 [details] [review]:

I'd rather the library lived in applet/ along with the applet.

Any reasons the applet itself isn't using the new applet library code?

::: .gitignore
@@ +51,1 @@
 applet/test-agentdialog

Why remove applet/bluetooth-applet.desktop.in from the .gitignore?

::: lib/Makefile.am
@@ -40,3 @@
 	gnome-bluetooth-enum-types.h	\
 	bluetooth-chooser-private.h
-EXTRA_DIST += $(libgnome_bluetooth_private_headers)

Should this be made as a separate patch, along with the rest of the introspection fixes?

Created attachment 171774 [details] [review]
More fixes to introspection

Comment on attachment 171774 [details] [review]
More fixes to introspection

Committed after fixing the white space problems.

Created attachment 171783 [details] [review]
Refactor bluetooth-applet as a library

Minor fixes

Created attachment 171784 [details] [review]
Port bluetooth-applet to the new applet library

Refactor the existing standalone bluetooth-applet to use the applet
library just introduced, to reduce duplicated code and make it easier
to test and fix the library itself.

The patches are unreviewable as-is. You pretty much rewrote the applet, but the flow of original code isn't so visible.

I'll take me a little while to unpick the changes you made, and see whether they work as expected.

I found quite a few bugs on top of the existing code. Take a look at the commits in gnome-bluetooth master.

I expect to find similar niggles elsewhere, and I'll probably change a few APIs as I get more time to review the code (especially the agents dialogue, as discussed on IRC).

Thanks for working on this though, much appreciated.

Attachment 171783 [details] pushed as 1838f07 - Refactor bluetooth-applet as a library
Attachment 171784 [details] pushed as c7b48a3 - Port bluetooth-applet to the new applet library