(In reply to comment #0)
> * Separator is shown when there are no devices in the section.  This is because
> updateFullMenu() is called after updateDevices().

Ugh. Will fix.

> * Seems we relaunch bluetooth-sendto each time the menu item is clicked.  We
> should re-present the existing tool.

Bug in gnome-bluetooth, I suppose (should use GtkApplication / libunique-3)

> * In other menus we keep the Settings menu item separate at the bottom but
> since we have so many separators already I think it may be fine not to have
> another one.

So should devices be contiguous with the settings item?

> * Should we still offer Send Files to Device if I don't have any devices set
> up?  Maybe yes since it seems that the sendto tool lets me pick one.  When I do
> pick one is it saved in my devices list after it is used successfully?

Only if you pair with it. Devices shown on the menu are those that are trusted to automatically connect with you, and often you don't want this.
The behaviour is the same as bluetooth-applet.

> * It seems that the devices-changed signal is emitted while the sendto tool is
> scanning for other devices on the network.  I'm not sure it makes sense to
> recompute our menus while that is occurring.  Is it really the same signal for
> devices that I have paired with vs. devices I can send to?

bluetoothd adds temporarily the device to the list when it is interacting with it, but we cannot distinguish this "device-added" from other similar signals (neither at the dbus or at the applet layer).

(In reply to comment #1)
> > * It seems that the devices-changed signal is emitted while the sendto tool is
> > scanning for other devices on the network.  I'm not sure it makes sense to
> > recompute our menus while that is occurring.  Is it really the same signal for
> > devices that I have paired with vs. devices I can send to?
> 
> bluetoothd adds temporarily the device to the list when it is interacting with
> it, but we cannot distinguish this "device-added" from other similar signals
> (neither at the dbus or at the applet layer).

But rather than always destroying and rebuilding the menu, we could figure out the updated menu contents first, and then only actually destroy+rebuild if it's going to change.

Created attachment 176831 [details] [review]
BluetoothMenu: hide the device separator if no devices are shown

When BluetoothApplet::show-full-menu property is notified (when you
switch from a disabled adapter / no adapter to an active one), we
would show all the menu, including the device separator, without
checking if any devices actually existed.

Created attachment 176832 [details] [review]
BluetoothStatus: move the sendto item to the bottom

Kill one separator by merging all global actions items at the end
of the menu, which ends up divided in three sections: status,
devices and actions.

Created attachment 176833 [details] [review]
BluetoothStatus: update only devices that actually changed

When receiving a "devices-changed" signal from BluetoothApplet,
check if some device item corresponds to an existing one, destroy
the remaining and add the new ones.
With this patch, signal emission when no device actually changed
(which happen due to bluetoothd creating temporary devices) result
in a no-op.

Comment on attachment 176831 [details] [review]
BluetoothMenu: hide the device separator if no devices are shown

>+        if (!this._hasDevices)
>+            this._deviceSep.hide();
>+        // in the other case (show_full_menu false, hasDevices true),
>+        // the menu must remain hidden

I don't understand this comment. Particularly, that code is executed whether show_full_menu is true or false.

Comment on attachment 176833 [details] [review]
BluetoothStatus: update only devices that actually changed

looks right

(In reply to comment #6)
> (From update of attachment 176831 [details] [review])
> >+        if (!this._hasDevices)
> >+            this._deviceSep.hide();
> >+        // in the other case (show_full_menu false, hasDevices true),
> >+        // the menu must remain hidden
> 
> I don't understand this comment. Particularly, that code is executed whether
> show_full_menu is true or false.

I considered four cases:
show_full_menu true, hasDevices true => covered by showAll
show_full_menu false, hasDevices false => covered by hideAll
show_full_menu true, hasDevices false => specifically hiding deviceSep
so the only case not handled by code is show_full_menu false, hasDevices true (in which case deviceSep must remain hidden)
Anyway, I see that this comment just creates confusion, so I will remove it.

Created attachment 176957 [details] [review]
BluetoothStatus: hide the device separator if no devices are shown

When BluetoothApplet::show-full-menu property is notified (when you
switch from a disabled adapter / no adapter to an active one), we
would show all the menu, including the device separator, without
checking if any devices actually existed.

Comment on attachment 176957 [details] [review]
BluetoothStatus: hide the device separator if no devices are shown

ah, i get it now. how about:

        if (this._applet.show_full_menu) {
            this._showAll(this._fullMenuItems);
            if (this._hasDevices)
              this._showAll(this._deviceItems);
            else
              this._deviceSep.hide();
        } else {
            this._hideAll(this._fullMenuItems);
            this._hideAll(this._deviceItems);
        }

? I think that makes it clearer. OK to commit like that

Comment on attachment 176957 [details] [review]
BluetoothStatus: hide the device separator if no devices are shown

Pushed with your change.