GNOME Bugzilla – Bug 723848
Always show the bluetooth submenu when there is bluetooth hardware
Last modified: 2016-09-02 12:14:25 UTC
The design used to state that the bluetooth menu should only be displayed when bluetooth is on (I assume that this is what we are doing now, although I'm finding it hard to test). This is inconvenient if you have paired devices set up and you want to turn bluetooth on. It is also inconsistent with the logic we are using for the other system status items. I'm proposing that we should the bluetooth submenu when paired devices are set up. This way, the menu will not be visible to those who never use bluetooth, but will be within reach for those who do. This is consistent with how we handle mobile broadband interfaces. If Bluetooth is on but no devices are connected, the status string would read "On". Otherwise, it will read "Off" or indicate the number of connected devices (eg. "1 Connected Device"). https://wiki.gnome.org/action/info/Design/OS/SystemStatus?action=diff&rev2=23&rev1=22
(In reply to comment #0) > The design used to state that the bluetooth menu should only be displayed when > bluetooth is on (I assume that this is what we are doing now, although I'm > finding it hard to test). It's currently shown when it's *used* (something is connected). > This is inconvenient if you have paired devices set > up and you want to turn bluetooth on. It is also inconsistent with the logic we > are using for the other system status items. > > I'm proposing that we should the bluetooth submenu when paired devices are set > up. This way, the menu will not be visible to those who never use bluetooth, > but will be within reach for those who do. This is consistent with how we > handle mobile broadband interfaces. > > If Bluetooth is on but no devices are connected, the status string would read > "On". Otherwise, it will read "Off" or indicate the number of connected devices > (eg. "1 Connected Device"). > > https://wiki.gnome.org/action/info/Design/OS/SystemStatus?action=diff&rev2=23&rev1=22 Makes sense to me, and should be easily implementable as well. Except that we cannot know whether devices are setup if the adapter is turned off. So this would be: - Shown if devices are setup and adapter is turned on ("X connected devices" if so, "On" otherwise) - Hidden otherwise
Created attachment 268407 [details] [review] bluetooth: Update Bluetooth status menu for latest design XXX Completely untested, but this should be roughly it.
Thanks! This would be an improvement. At the same time, it doesn't quite address the issue described in the bug report: if you want to turn bluetooth on, you still have to dig into Settings (which is particularly frustrating if you have just turned off bluetooth from the menu, and it has disappeared in front of your eyes). If we can't tell if there are paired devices set up when bluetooth is off, maybe it would be better to always show the bluetooth menu? (We'd still only show the icon in the top bar if bluetooth is "in use".)
(In reply to comment #3) > Thanks! This would be an improvement. > > At the same time, it doesn't quite address the issue described in the bug > report: if you want to turn bluetooth on, you still have to dig into Settings > (which is particularly frustrating if you have just turned off bluetooth from > the menu, and it has disappeared in front of your eyes). I don't think that turning off the Bluetooth alone is something that people do often, especially if they do use Bluetooth devices. Either they'd use hardware or software killswitches, which would take care of that, or they'd leave it on. > If we can't tell if there are paired devices set up when bluetooth is off, > maybe it would be better to always show the bluetooth menu? We cannot know that there are Bluetooth devices setup when there's no Bluetooth adapter. The pairings/setup is per-adapter, so no adapter (when it's killswitched, it's as if it was unplugged), no devices. > (We'd still only show the icon in the top bar if bluetooth is "in use".) Right, my patch doesn't do that.
(In reply to comment #4) ... > > At the same time, it doesn't quite address the issue described in the bug > > report: if you want to turn bluetooth on, you still have to dig into Settings > > (which is particularly frustrating if you have just turned off bluetooth from > > the menu, and it has disappeared in front of your eyes). > > I don't think that turning off the Bluetooth alone is something that people do > often, especially if they do use Bluetooth devices. Either they'd use hardware > or software killswitches, which would take care of that, or they'd leave it on. We expose the "turn off" option in the UI, so I think we have a responsibility to make it work nicely. Also, I don't see why people wouldn't use this method to save power. > > If we can't tell if there are paired devices set up when bluetooth is off, > > maybe it would be better to always show the bluetooth menu? > > We cannot know that there are Bluetooth devices setup when there's no Bluetooth > adapter. The pairings/setup is per-adapter, so no adapter (when it's > killswitched, it's as if it was unplugged), no devices. That's unfortunate. Couldn't we keep our own record of whether there are pairings set up (at least while the adaptor is disabled)? Or what about simply always showing the bluetooth menu? > > (We'd still only show the icon in the top bar if bluetooth is "in use".) > > Right, my patch doesn't do that. Cool
*** Bug 724997 has been marked as a duplicate of this bug. ***
> I don't think that turning off the Bluetooth alone is something that people do > often, especially if they do use Bluetooth devices. Either they'd use hardware > or software killswitches, which would take care of that, or they'd leave it on. I agree with Allan here. Using Bluetooth for listening to music, I prefer to turn it off when not in use (especially when AC is not plugged in). So I turn it on/off quite often and it's a hassle to turn on (even in current 3.11.x) because Bluetooth is by default hidden, so I have to climb into settings.
Review of attachment 268407 [details] [review]: The patch needs rebasing after bug 709353 landed, so marking 'needs-work'. Just a couple of style nits otherwise: ::: js/ui/status/bluetooth.js @@ -75,3 @@ - let adapter = this._getDefaultAdapter(); - if (!adapter) - return 0; It's not quite obvious why you are moving the getAdapter() code out of this function rather than just changing the return value to [0, 0] here. @@ +77,3 @@ let [ret, iter] = this._model.iter_children(adapter); while (ret) { + let [ isConnected, isPaired, isTrusted ] = this._model.get_value(iter, Style nit: no space after opening bracket or before closing bracket. @@ +88,3 @@ ret = this._model.iter_next(iter); } + return [ nDevices, nConnectedDevices ]; Dto. @@ +97,3 @@ + nConnectedDevices = 0; + } else { + let [ nDevices, nConnectedDevices ] = this._getNConnectedDevices(adapter); Dto.
We had some feedback on Twitter yesterday from someone for whom this is an issue. In this case, the user has a bluetooth headset, and they are frustrated that it is impossible to reconnect from the system status menu.
Created attachment 290624 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup. But checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some.
The rest of the items in the bug seem to have been fixed (the visibility of the icon, and of the menu item).
Review of attachment 290624 [details] [review]: ::: js/ui/status/bluetooth.js @@ +33,3 @@ this._indicator = this._addIndicator(); this._indicator.icon_name = 'bluetooth-active-symbolic'; + this._had_setup_devices = false; Style nit: _hadSetupDevices @@ +107,3 @@ + } else { + this._item.actor.visible = this._proxy.BluetoothHasAirplaneMode && !this._proxy.BluetoothAirplaneMode; + this._had_setup_devices = this._item.actor.visible; I don't understand why we (potentially) reset hadSetupDevices here - should this be "true" (we've seen *some* devices) or "hadSetupDevices || this._item.actor.visible" (we showed the menu at one point)?
(In reply to comment #12) > Review of attachment 290624 [details] [review]: > > ::: js/ui/status/bluetooth.js > @@ +33,3 @@ > this._indicator = this._addIndicator(); > this._indicator.icon_name = 'bluetooth-active-symbolic'; > + this._had_setup_devices = false; > > Style nit: _hadSetupDevices Fixed. > @@ +107,3 @@ > + } else { > + this._item.actor.visible = this._proxy.BluetoothHasAirplaneMode && > !this._proxy.BluetoothAirplaneMode; > + this._had_setup_devices = this._item.actor.visible; > > I don't understand why we (potentially) reset hadSetupDevices here - should > this be "true" (we've seen *some* devices) or "hadSetupDevices || > this._item.actor.visible" (we showed the menu at one point)? The goal here is to reset the value when there are no setup devices. So if you remove the last setup device, the menu would get hidden. The code is however incorrect, because we count the number of connected devices instead of the number of setup devices.
Created attachment 290626 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup. But checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some.
Review of attachment 290626 [details] [review]: Needs-work for the typo in getNDevices(), not the questions later. ::: js/ui/status/bluetooth.js @@ +91,3 @@ nDevices++; + nConnectedDevices++; + nDevices++; This doesn't make any sense - nConnectedDevices++ should replace nDevices++ in the if(isConnected) block @@ +110,3 @@ + } else { + this._item.actor.visible = this._proxy.BluetoothHasAirplaneMode && !this._proxy.BluetoothAirplaneMode; + this._hadSetupDevices = this._item.actor.visible; "The goal here is to reset the value when there are no setup devices. So if you remove the last setup device, the menu would get hidden." Mmmh, there's at least a disconnect with the commit message here: "If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on." Now if I turn off Bluetooth in the menu, hadSetupDevices is unset and the menu is hidden, so it can only be turned back on in Settings.
(In reply to comment #15) > Review of attachment 290626 [details] [review]: > > Needs-work for the typo in getNDevices(), not the questions later. > > ::: js/ui/status/bluetooth.js > @@ +91,3 @@ > nDevices++; > + nConnectedDevices++; > + nDevices++; > > This doesn't make any sense - nConnectedDevices++ should replace nDevices++ in > the if(isConnected) block Yeah, typo. > @@ +110,3 @@ > + } else { > + this._item.actor.visible = this._proxy.BluetoothHasAirplaneMode && > !this._proxy.BluetoothAirplaneMode; > + this._hadSetupDevices = this._item.actor.visible; > > "The goal here is to reset the value when there are no setup devices. So if you > remove the last setup device, the menu would get hidden." > > Mmmh, there's at least a disconnect with the commit message here: > > "If we detected that Bluetooth devices were setup, show the Bluetooth > menu so that users can easily turn Bluetooth back on." > > Now if I turn off Bluetooth in the menu, hadSetupDevices is unset and the menu > is hidden, so it can only be turned back on in Settings. If you turn off Bluetooth, the adapter disappears, and _getNDevices() returns [-1, -1], and goes through the other other path in the conditional.
Created attachment 290640 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup. But checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some.
Review of attachment 290640 [details] [review]: Bastien clarified on IRC how it's supposed to work (as in the commit message), but it doesn't, so marking as needs-work
Created attachment 290658 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup. But checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some.
Latest version of the patch, still untested as gnome-shell from master fails for me right now.
(In reply to comment #20) > Latest version of the patch, still untested I'm afraid the menu still disappears when turning off bt
I've looked into BlueZ, and I can't find a way to do this cleanly. As far as any BlueZ D-Bus client is concerned, an adapter or a device disappearing is just an interface disappearing. When an adapter disappears, we see all the device interfaces disappearing one-by-one, as if we removed them by hand, before the adapter interface disappears. We seem to have no way of knowing whether any devices were still configured for that adapter before it disappeared.
I've cooked up a patch against gnome-bluetooth in bug 743693, to have it export the number of devices that were setup against the default adapter. I'll upload a kludge of gnome-shell patch here shortly (there's no code to handle the menu being visible without it being On, so that'll need fixing). The main problem I have with this approach is that if the user starts the system with Bluetooth disabled, they won't see any Bluetooth menus, but if they turn it off during run-time, they would. This goes against the primary goal of this bug, which would be to show the Bluetooth menu item when the user will have a use for it, even on startup. (We could try and remember it across sessions... Florian?) Allan, ideas?
Created attachment 295753 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup. But checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some.
Could we fix the BlueZ API?
(In reply to comment #25) > Could we fix the BlueZ API? It's not the BlueZ API per-se, it's the ObjectManager API that creates that problem. But even if that was fixed (or worked around, as in the bug), it wouldn't solve the problem of knowing whether there really are adapters available, which one would be the default, and whether it has any devices attached to it. This is something you could probably get away with if you could target a certain hardware platform.
Coming back to this issue, I think that it would be best to always show the Bluetooth menu if there's Bluetooth hardware (and if we can't reliably detect that, fall back to the current behaviour). There are some supporting arguments for doing this in bug 740229.
(In reply to Bastien Nocera from comment #26) > (In reply to comment #25) > > Could we fix the BlueZ API? > > It's not the BlueZ API per-se, it's the ObjectManager API that creates that > problem. But even if that was fixed (or worked around, as in the bug), it > wouldn't solve the problem of knowing whether there really are adapters > available, which one would be the default, and whether it has any devices > attached to it. This is something you could probably get away with if you > could target a certain hardware platform. I don't even agree that we need to add a new API to bluez for this. All of the information that we need is presently available and reported via ObjectManager. Set aside the killswitch case. That's clearly not going to work, since we can't see the adaptor in that case, nor its hw address. That's fine, since a "Turn On" option in software wouldn't do anything in any case when the device has been hardware killswitched, so it makes no sense to show it. Now consider the non-killswitch case where the adaptor has merely been disabled via software. In that case, we can absolutely make a decision to show the icon or not based on the existing information on the bluez API. The adaptor is present on the system bus as /org/bluez/hci0 (or similar) and even when it is disabled, we can see that it has pairings available because there are also paths such as /org/bluez/hci0/dev_F4_F1_EA_3F_8C_5B. If the adaptor gets pulled or killswitched then these paths disappear, which if I understand the arguments above correctly, is exactly what we want. So then the decision to show a icon or not is very simple: - does there exist a path on D-Bus under /org/bluez/*/* ? The existence of this node is directly reported via the org.freedesktop.DBus.ObjectManager interface on the root path and can easily be monitored there as well.
(In reply to Allison Ryan Lortie (desrt) from comment #28) > (In reply to Bastien Nocera from comment #26) > > (In reply to comment #25) > > > Could we fix the BlueZ API? > > > > It's not the BlueZ API per-se, it's the ObjectManager API that creates that > > problem. But even if that was fixed (or worked around, as in the bug), it > > wouldn't solve the problem of knowing whether there really are adapters > > available, which one would be the default, and whether it has any devices > > attached to it. This is something you could probably get away with if you > > could target a certain hardware platform. > > I don't even agree that we need to add a new API to bluez for this. All of > the information that we need is presently available and reported via > ObjectManager. > > Set aside the killswitch case. That's clearly not going to work, since we > can't see the adaptor in that case, nor its hw address. That's fine, since > a "Turn On" option in software wouldn't do anything in any case when the > device has been hardware killswitched, so it makes no sense to show it. > > Now consider the non-killswitch case where the adaptor has merely been > disabled via software. Right, this is where your case falls down. On a Thinkpad for example: $ rfkill list 0: tpacpi_bluetooth_sw: Bluetooth Soft blocked: no Hard blocked: no 2: phy0: Wireless LAN Soft blocked: no Hard blocked: no 4: hci0: Bluetooth Soft blocked: no Hard blocked: no $ rfkill block bluetooth $ rfkill list 0: tpacpi_bluetooth_sw: Bluetooth Soft blocked: yes Hard blocked: no 2: phy0: Wireless LAN Soft blocked: no Hard blocked: no $ hciconfig $ The Bluetooth adapter disappeared, and we could un-rfkill with just software. We cannot ask bluetoothd anything as it doesn't know where the Bluetooth adapter is. > In that case, we can absolutely make a decision to > show the icon or not based on the existing information on the bluez API. > The adaptor is present on the system bus as /org/bluez/hci0 (or similar) and > even when it is disabled, we can see that it has pairings available because > there are also paths such as /org/bluez/hci0/dev_F4_F1_EA_3F_8C_5B. If the > adaptor gets pulled or killswitched then these paths disappear, which if I > understand the arguments above correctly, is exactly what we want. > > So then the decision to show a icon or not is very simple: > > - does there exist a path on D-Bus under /org/bluez/*/* ? > > The existence of this node is directly reported via the > org.freedesktop.DBus.ObjectManager interface on the root path and can easily > be monitored there as well. All this is incorrect, as one of the premises is incorrect. So, as I said, we'd need new API to implement this, or we need to make concessions.
Okay. Fair enough. rfkill is pretty confusing. We can still improve the case of "adaptor is visible in hciconfig but disabled". This is the case that we get when we disable the adaptor via the existing UI (and is therefore the case that we want to be reversible). Once we're beyond that then we could start talking about adding an API to bluez to be aware of softkilled HCIs, But as far as I'm concerned, this is not really hugely important... Totally random aside: hitting my hardware killswitch on a XPS 13 entirely removes my HCI from 'rfkill list'. According to 'dmesg' it appears that the hardware killswitch unplugs the adaptor from USB. As a result, I never see "Hard blocked: yes" for my HCI.
One more potentially confusing thing to be aware of, however: desrt@humber:~$ rfkill list ... 12: hci0: Bluetooth Soft blocked: no desrt@humber:~$ rfkill block 12 desrt@humber:~$ rfkill list ... 12: hci0: Bluetooth Soft blocked: yes desrt@humber:~$ hciconfig hci0: Type: BR/EDR Bus: USB BD Address: 7C:7A:91:27:06:95 ACL MTU: 1021:5 SCO MTU: 96:5 DOWN ... desrt@humber:~$ sudo hciconfig hci0 up ... Can't init device hci0: Operation not possible due to RF-kill (132) ... but it seems that the switch in the control centre is "strong enough" to bring it back from this state, so I don't really think this is a serious problem.
Okay. I was missing one important piece of information: disabling bluetooth in the UI is actually using rfkill and not the equivalent of "hciconfig down", which is what I was assuming before. ... and that is what causes trouble on Thinkpads, which cannot see the adaptor, even when it is only softkilled. The above changes would still improve the situation for people who are using laptops that don't have this problem. Another possibility is to use 'hciconfig down' instead of rfkill for switching off bluetooth from the shell menu. That would prevent the adaptor from disappearing on Thinkpads (but I imagine this may come with a small-but-non-zero power consumption cost). Other than that, I see your point now that this will require changes in bluez.
As discussed on IRC, we can't really change the way that the Bluetooth adapters are enabled or disabled. What we would do is to show the Bluetooth menu if: - the adapter is there, powered, whether or not it has devices paired/setup, because we want to be able to turn it off easily - there is no adapter, but a Bluetooth rfkill exists, and the machine had, at one point, a default adapter with devices paired/setup
Created attachment 314153 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup, at some point. This state is saved in the gnome-shell GSettings in the had-bluetooth-devices-setup key. Checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some in the future.
This last patch is untested, and probably not very elegant code-wise, but I'm getting tunnel vision staring at it. This doesn't need any gnome-bluetooth changes anymore (yay!).
Created attachment 314208 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup, at some point. This state is saved in the gnome-shell GSettings in the had-bluetooth-devices-setup key. Checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some in the future.
Created attachment 314209 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup, at some point. This state is saved in the gnome-shell GSettings in the had-bluetooth-devices-setup key. Checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some in the future.
I think I nailed it. A couple of typos, a case of a silent failure, and a couple of logic errors, but I think we're good now. The last version of the patch made sure to always hide the menu when Bluetooth is hard-blocked. Allan, after updating your jhbuilt gnome-shell, you can test with: jhbuild run gnome-shell --replace --wayland
Review of attachment 314209 [details] [review]: Some concerns about the GSettings use here, but I think the logic is mostly right. The "number of devices, or 1, or -1" thing seems pretty confusing... particularly when there are two return values. ::: js/ui/status/bluetooth.js @@ +37,3 @@ this._indicator.icon_name = 'bluetooth-active-symbolic'; + this._settings = new Gio.Settings({ schema_id: this._SHELL_SCHEMA }); + this._hadSetupDevices = this._settings.get_boolean(this._HAD_BLUETOOTH_DEVICES_SETUP); There is no point in caching this value in this._hadSetupDevices. Just get() it when you need it. Doing so would also (at least slightly) improve response to the user manually flipping the setting in the editor -- but you'd still need to connect a signal handler if you really wanted that to work properly. @@ +115,3 @@ } + + this._settings.set_boolean(this._HAD_BLUETOOTH_DEVICES_SETUP, nDevices > 0); This is going to thrash GSettings like crazy. Please at least check that you're changing the value before you do the set(). One alternative to ponder that would make this slightly less evil: what if the 'had bluetooth devices setup' flag was flipped from the control centre when the change was actually made? Then the shell would never write this key; only read it.
Review of attachment 314209 [details] [review]: ::: js/ui/status/bluetooth.js @@ +29,3 @@ + _SHELL_SCHEMA: 'org.gnome.shell', + _HAD_BLUETOOTH_DEVICES_SETUP: 'had-bluetooth-devices-setup', Use a const at the top, not a property @@ +36,3 @@ this._indicator = this._addIndicator(); this._indicator.icon_name = 'bluetooth-active-symbolic'; + this._settings = new Gio.Settings({ schema_id: this._SHELL_SCHEMA }); You don't need to create a Settings object for the shell schema, it's available as global.settings @@ +130,3 @@ + // Remember if there were setup devices and show the menu + // if we've seen setup devices and we're not hard blocked + if (nDevices > 0) { Style nit: no braces where all blocks are one line
(In reply to Allison Ryan Lortie (desrt) from comment #39) > Review of attachment 314209 [details] [review] [review]: > > Some concerns about the GSettings use here, but I think the logic is mostly > right. > > The "number of devices, or 1, or -1" thing seems pretty confusing... > particularly when there are two return values. > > ::: js/ui/status/bluetooth.js > @@ +37,3 @@ > this._indicator.icon_name = 'bluetooth-active-symbolic'; > + this._settings = new Gio.Settings({ schema_id: this._SHELL_SCHEMA > }); > + this._hadSetupDevices = > this._settings.get_boolean(this._HAD_BLUETOOTH_DEVICES_SETUP); > > There is no point in caching this value in this._hadSetupDevices. Just > get() it when you need it. I'll keep it to avoid having to look it up later, to not set it when it hasn't changed. > Doing so would also (at least slightly) improve response to the user > manually flipping the setting in the editor -- but you'd still need to > connect a signal handler if you really wanted that to work properly. > > @@ +115,3 @@ > } > + > + this._settings.set_boolean(this._HAD_BLUETOOTH_DEVICES_SETUP, > nDevices > 0); > > This is going to thrash GSettings like crazy. Please at least check that > you're changing the value before you do the set(). I've changed this to check the new value against this._hadSetupDevices. > One alternative to ponder that would make this slightly less evil: what if > the 'had bluetooth devices setup' flag was flipped from the control centre > when the change was actually made? Then the shell would never write this > key; only read it. I would prefer not to have direct dependencies in gnome-control-center against the shell's schema. It's also created right where it's consumed, and we need to loop through the devices to check the number of connected devices in any case. (In reply to Florian Müllner from comment #40) > Review of attachment 314209 [details] [review] [review]: > > ::: js/ui/status/bluetooth.js > @@ +29,3 @@ > > + _SHELL_SCHEMA: 'org.gnome.shell', > + _HAD_BLUETOOTH_DEVICES_SETUP: 'had-bluetooth-devices-setup', > > Use a const at the top, not a property I was using the same style as in network.js. Fixed. > @@ +36,3 @@ > this._indicator = this._addIndicator(); > this._indicator.icon_name = 'bluetooth-active-symbolic'; > + this._settings = new Gio.Settings({ schema_id: this._SHELL_SCHEMA > }); > > You don't need to create a Settings object for the shell schema, it's > available as global.settings Fixed. > @@ +130,3 @@ > + // Remember if there were setup devices and show the menu > + // if we've seen setup devices and we're not hard blocked > + if (nDevices > 0) { > > Style nit: no braces where all blocks are one line Fixed.
Created attachment 314216 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup, at some point. This state is saved in the gnome-shell GSettings in the had-bluetooth-devices-setup key. Checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some in the future.
Review of attachment 314216 [details] [review]: ::: js/ui/status/bluetooth.js @@ +115,3 @@ + if (this._hadSetupDevices != (nDevices > 0)) { + this._hadSetupDevices = !this._hadSetupDevices; + global.set_boolean(HAD_BLUETOOTH_DEVICES_SETUP, this._hadSetupDevices); global.settings, not global
Created attachment 314298 [details] [review] bluetooth: Show the Bluetooth menu when there were setup devices If we detected that Bluetooth devices were setup, show the Bluetooth menu so that users can easily turn Bluetooth back on. This is a bit of a hack, as we cannot detect whether there is a Bluetooth adapter at all when it's disabled, so we cannot tell whether there were any Bluetooth devices setup, at some point. This state is saved in the gnome-shell GSettings in the had-bluetooth-devices-setup key. Checking whether we saw Bluetooth devices at one point is a good enough guess of whether there will be some in the future.
Review of attachment 314298 [details] [review]: LGTM
Attachment 314298 [details] pushed as 8ceae3b - bluetooth: Show the Bluetooth menu when there were setup devices
*** Bug 770591 has been marked as a duplicate of this bug. ***