GNOME Bugzilla – Bug 618312
No native gnome-shell Bluetooth applet
Last modified: 2010-12-18 18:13:16 UTC
In Gnome 2 on Ubuntu, there is a panel applet that allows easy control of bluetooth. When I use Gnome Shell, there is no easy way to perform these usual functions. I can't conveniently browse files any more. That's the main function that I use, but others probably use more.
Created attachment 173355 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library.
I'd rather the calls for bluetooth-sendto and mounting/browsing obex:///[] URIs were moved into the private library in gnome-bluetooth, and implemented there. Same thing for the calls to "gnome-shell-bluetooth-chooser" (which contains your home install path anyway).
(In reply to comment #2) > I'd rather the calls for bluetooth-sendto and mounting/browsing obex:///[] URIs > were moved into the private library in gnome-bluetooth, and implemented there. Ok, I'll prepare a patch for that then. > Same thing for the calls to "gnome-shell-bluetooth-chooser" (which contains > your home install path anyway). Ugh... only gnome-shell-bluetooth-chooser.in had to go in. Obsoleting for the time being...
Created attachment 173438 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library.
Review of attachment 173438 [details] [review]: Looking better, just minor niggles now. ::: js/helpers/bluetoothChooser.js @@ +1,1 @@ +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ This file will obviously need to go... ::: js/ui/panel.js @@ +34,3 @@ 'a11y': imports.ui.status.accessibility.ATIndicator, 'volume': imports.ui.status.volume.Indicator, + 'bluetooth': imports.ui.status.bluetooth.Indicator, Would it just fail to build, or run if gnome-bluetooth wasn't available? gnome-bluetooth only works on Linux systems... ::: js/ui/status/bluetooth.js @@ +26,3 @@ +} + +const BluetoothType = { This is already exported by libgnome-bluetooth, why re-declare it? @@ +155,3 @@ + let d = devices[i]; + let item = this._createDeviceItem(d); + this.menu.addMenuItem(item, 6 + this._deviceItems.length); We don't create menu items for devices which wouldn't have a sub-menu, so you need to filter out all the "!device.can_connect" devices. ::: src/gnome-shell-bluetooth-chooser.in @@ +17,3 @@ +export GSETTINGS_SCHEMA_DIR="$schemaDir" + +@GJS_CONSOLE@ -c "const BluetoothChooserHelper = imports.helpers.bluetoothChooser; Still don't see why that file is needed.
(In reply to comment #5) > Review of attachment 173438 [details] [review]: > > Looking better, just minor niggles now. > > ::: js/ui/panel.js > @@ +34,3 @@ > 'a11y': imports.ui.status.accessibility.ATIndicator, > 'volume': imports.ui.status.volume.Indicator, > + 'bluetooth': imports.ui.status.bluetooth.Indicator, > > Would it just fail to build, or run if gnome-bluetooth wasn't available? > gnome-bluetooth only works on Linux systems... Well, since libgnome-bluetooth-applet is a private library, libgnome-shell is linked explicitly against it, so it would fail to link if not installed. I should add a configure check and possibly make it optional. > > ::: js/ui/status/bluetooth.js > @@ +26,3 @@ > +} > + > +const BluetoothType = { > > This is already exported by libgnome-bluetooth, why re-declare it? Because I don't want to load GnomeBluetooth, that would be another typelib to load and a lot more types to resolve. Having it inline in JS is a lot faster. > > @@ +155,3 @@ > + let d = devices[i]; > + let item = this._createDeviceItem(d); > + this.menu.addMenuItem(item, 6 + this._deviceItems.length); > > We don't create menu items for devices which wouldn't have a sub-menu, so you > need to filter out all the "!device.can_connect" devices. Ok. (assuming that a device with .capabilities != None but .can_connect == false is still shown) > ::: src/gnome-shell-bluetooth-chooser.in > @@ +17,3 @@ > +export GSETTINGS_SCHEMA_DIR="$schemaDir" > + > +@GJS_CONSOLE@ -c "const BluetoothChooserHelper = > imports.helpers.bluetoothChooser; > > Still don't see why that file is needed. Like in bug 633361, because you cannot have dialogs in the Shell process. The only alternative is to have a bluetooth-chooser or bluetooth-browse in C in gnome-bluetooth, like we have bluetooth-sendto. (You cannot use system dialogs, as BluetoothChooser is Gtk based and would be horrible in a St dialog)
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 173438 [details] [review] [details]: > > > > Looking better, just minor niggles now. > > > > ::: js/ui/panel.js > > @@ +34,3 @@ > > 'a11y': imports.ui.status.accessibility.ATIndicator, > > 'volume': imports.ui.status.volume.Indicator, > > + 'bluetooth': imports.ui.status.bluetooth.Indicator, > > > > Would it just fail to build, or run if gnome-bluetooth wasn't available? > > gnome-bluetooth only works on Linux systems... > > Well, since libgnome-bluetooth-applet is a private library, libgnome-shell is > linked explicitly against it, so it would fail to link if not installed. > I should add a configure check and possibly make it optional. Agreed. Would be nicer as a run-time detection (so gnome-bluetooth can be installed after gnome-shell). > > ::: js/ui/status/bluetooth.js > > @@ +26,3 @@ > > +} > > + > > +const BluetoothType = { > > > > This is already exported by libgnome-bluetooth, why re-declare it? > > Because I don't want to load GnomeBluetooth, that would be another typelib to > load and a lot more types to resolve. Having it inline in JS is a lot faster. Right. But the applet library already knows internall about those. We could add bluetooth-enums.h to the introspection data (gnome-bluetooth-enum-types.[ch] in lib/ already do that). > > @@ +155,3 @@ > > + let d = devices[i]; > > + let item = this._createDeviceItem(d); > > + this.menu.addMenuItem(item, 6 + this._deviceItems.length); > > > > We don't create menu items for devices which wouldn't have a sub-menu, so you > > need to filter out all the "!device.can_connect" devices. > > Ok. > (assuming that a device with .capabilities != None but .can_connect == false is > still shown) This is the code in the legacy applet: static gboolean device_has_submenu (BluetoothSimpleDevice *dev) { if (dev->can_connect != FALSE) return TRUE; if (dev->capabilities != BLUETOOTH_CAPABILITIES_NONE) return TRUE; return FALSE; } > > ::: src/gnome-shell-bluetooth-chooser.in > > @@ +17,3 @@ > > +export GSETTINGS_SCHEMA_DIR="$schemaDir" > > + > > +@GJS_CONSOLE@ -c "const BluetoothChooserHelper = > > imports.helpers.bluetoothChooser; > > > > Still don't see why that file is needed. > > Like in bug 633361, because you cannot have dialogs in the Shell process. > The only alternative is to have a bluetooth-chooser or bluetooth-browse in C in > gnome-bluetooth, like we have bluetooth-sendto. > (You cannot use system dialogs, as BluetoothChooser is Gtk based and would be > horrible in a St dialog) Then it looks like we might need to reimplement a chooser in St. Does St have a treeview widget that would use GtkTreeModel? If so, reimplementing one should be pretty straight forward.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > Review of attachment 173438 [details] [review] [details] [details]: > > > > > > Looking better, just minor niggles now. > > > > > > ::: js/ui/panel.js > > > @@ +34,3 @@ > > > 'a11y': imports.ui.status.accessibility.ATIndicator, > > > 'volume': imports.ui.status.volume.Indicator, > > > + 'bluetooth': imports.ui.status.bluetooth.Indicator, > > > > > > Would it just fail to build, or run if gnome-bluetooth wasn't available? > > > gnome-bluetooth only works on Linux systems... > > > > Well, since libgnome-bluetooth-applet is a private library, libgnome-shell is > > linked explicitly against it, so it would fail to link if not installed. > > I should add a configure check and possibly make it optional. > > Agreed. Would be nicer as a run-time detection (so gnome-bluetooth can be > installed after gnome-shell). Can't do. We need to have the module loaded when opening the typelib, and dlopen fails (not in system search directory), so we need to set RPATH with libtool. > > > > ::: js/ui/status/bluetooth.js > > > @@ +26,3 @@ > > > +} > > > + > > > +const BluetoothType = { > > > > > > This is already exported by libgnome-bluetooth, why re-declare it? > > > > Because I don't want to load GnomeBluetooth, that would be another typelib to > > load and a lot more types to resolve. Having it inline in JS is a lot faster. > > Right. But the applet library already knows internall about those. We could add > bluetooth-enums.h to the introspection data (gnome-bluetooth-enum-types.[ch] in > lib/ already do that). Ok. > > > > @@ +155,3 @@ > > > + let d = devices[i]; > > > + let item = this._createDeviceItem(d); > > > + this.menu.addMenuItem(item, 6 + this._deviceItems.length); > > > > > > We don't create menu items for devices which wouldn't have a sub-menu, so you > > > need to filter out all the "!device.can_connect" devices. > > > > Ok. > > (assuming that a device with .capabilities != None but .can_connect == false is > > still shown) > > This is the code in the legacy applet: > static gboolean > device_has_submenu (BluetoothSimpleDevice *dev) > { > if (dev->can_connect != FALSE) > return TRUE; > if (dev->capabilities != BLUETOOTH_CAPABILITIES_NONE) > return TRUE; > return FALSE; > } > > > > ::: src/gnome-shell-bluetooth-chooser.in > > > @@ +17,3 @@ > > > +export GSETTINGS_SCHEMA_DIR="$schemaDir" > > > + > > > +@GJS_CONSOLE@ -c "const BluetoothChooserHelper = > > > imports.helpers.bluetoothChooser; > > > > > > Still don't see why that file is needed. > > > > Like in bug 633361, because you cannot have dialogs in the Shell process. > > The only alternative is to have a bluetooth-chooser or bluetooth-browse in C in > > gnome-bluetooth, like we have bluetooth-sendto. > > (You cannot use system dialogs, as BluetoothChooser is Gtk based and would be > > horrible in a St dialog) > > Then it looks like we might need to reimplement a chooser in St. Does St have a > treeview widget that would use GtkTreeModel? If so, reimplementing one should > be pretty straight forward. There is no TreeView in St, and implementing one will be a lot of work. I'd stick with the helper hack for now.
(In reply to comment #8) > There is no TreeView in St, and implementing one will be a lot of work. I'd > stick with the helper hack for now. Given that we only show paired devices with the obex ftp service in this list, it might make sense to remove the menu entry altogether. As long as we keep the device sub-menus. The same functionality would be available in the device specific sub-menus.
Created attachment 173601 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library. This mostly works, except: - Last rework of PopupMenus regressed submenus, and now they're always on the right. - Using clutter_actor_destroy for removing device submenus on update makes shell fail with SIGABRT (related to memory management, comes from gjs)
Created attachment 174209 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library. Updating labels for bug 634328
*** Bug 634328 has been marked as a duplicate of this bug. ***
Comment on attachment 174209 [details] [review] Bluetooth status indicator >+ [if test "x$with_bluetooth" != xauto; then >+ AC_MSG_FAILURE( >+ [--with-bluetooth was given, but gnome-bluetooth is not installed]) >+ fi The issue here is that gnome-bluetooth only builds on Linux, correct? If so, I think we should make it explicitly mandatory on Linux, and "optional" only on other platforms; we don't allow people to configure out any of the other indicators so there's no reason to make this one optional either. Also, I don't foresee many other uses for js/misc/config, so I think it might be better to just make this a ShellGlobal property. >- addMenuItem: function(menuItem) { >- this._box.add(menuItem.actor); >+ addMenuItem: function(menuItem, position) { >+ if (position != undefined) >+ this._box.insert_actor(menuItem.actor, position); >+ else >+ this._box.add(menuItem.actor); if you want to split this patch out and just commit it now, go ahead. >+ _init: function() { >+ PanelMenu.SystemStatusButton.prototype._init.call(this, 'bluetooth-disabled-symbolic', null); don't need to specify "-symbolic" >+ this._killswitch.connect('toggled', Lang.bind(this, function() { >+ let current_state = this._applet.killswitch_state; The logic in this handler is sort of convoluted: you're responding to the user toggling the menu item, but you never actually look at what state the menu item is in, you only look at the killswitch state, and assume that it must match. I'd check if current_state is HARD_BLOCKED, and force the toggle false in that case, but if it's not HARD_BLOCKED, then set the new killswitch_state based on the toggle state, not based on current_state. >+ this._discoverable = new PopupMenu.PopupSwitchMenuItem(_("Visibility"), this._applet.discoverable); this item seems to always get initialized to "off" for me >+ this._fullMenuItems = [new PopupMenu.PopupMenuItem(_("Send file to device...")), >+ new PopupMenu.PopupSeparatorMenuItem(), >+ new PopupMenu.PopupSeparatorMenuItem(), >+ new PopupMenu.PopupMenuItem(_("Configure new device..."))]; need to make sure the two separators get squished into one if there's nothing between them. (Actually, we could just do that at the PopupMenu level maybe, rather than needing to worry about it both here and in the power manager patch...) The menu item ordering and text doesn't correspond to the mockups, even ignoring the new-style-submenus thing. (Among other things, the mockups Capitalize Each Word in menu items. This applies throughout bluetooth.js, not just here.) >+ Mainloop.idle_add(Lang.bind(this, function() { >+ Main.messageTray.add(this._source); >+ return false; >+ })); this is because Main.messageTray doesn't exist yet I suppose? It would be better to fix that. Probably start() should make a separate panel call to initialize the status items after the big block of constructor calls. >+ this._applet.connect('notify::show-full-menu', Lang.bind(this, this._updateFullMenu)); The daemon is not supposed to be deciding what we do or don't show. >+ this._source = new Source(); The message tray will destroy unused sources; you want to create a source on demand, and keep reusing it until the tray destroys it, and then create a new source on demand the next time you need one. >+ if (on) { >+ this._discoverable.actor.show(); >+ this.setIcon('bluetooth-symbolic'); >+ } else { >+ this._discoverable.actor.hide(); >+ this.setIcon('bluetooth-disabled-symbolic'); again, don't need "-symbolic". Also, there doesn't seem to be a symbolic version of "bluetooth", so it falls back to the fullcolor version. You want "bluetooth-active" I guess. (Or maybe the symbolic icon theme should be installing a symlink.) >+ _updateDevices: function() { I can't get my laptop to pair successfully with my phone (with gnome-shell / gnome-bluetooth master) so I haven't been able to test this part... >+ notify: function(notification) { >+ this._notifications.push(notification); >+ >+ MessageTray.Source.prototype.notify.call(this, notification); >+ }, you need to remove it from this._notifications when it is destroyed
(In reply to comment #13) > (From update of attachment 174209 [details] [review]) > >+ [if test "x$with_bluetooth" != xauto; then > >+ AC_MSG_FAILURE( > >+ [--with-bluetooth was given, but gnome-bluetooth is not installed]) > >+ fi > > The issue here is that gnome-bluetooth only builds on Linux, correct? If so, I > think we should make it explicitly mandatory on Linux, and "optional" only on > other platforms; we don't allow people to configure out any of the other > indicators so there's no reason to make this one optional either. > > Also, I don't foresee many other uses for js/misc/config, so I think it might > be better to just make this a ShellGlobal property. Unfortunately, ShellGlobal is not there when importing the panel module (which imports ui.status.bluetooth, which itself imports the non existing gi.GnomeBluetooth). > > >- addMenuItem: function(menuItem) { > >- this._box.add(menuItem.actor); > >+ addMenuItem: function(menuItem, position) { > >+ if (position != undefined) > >+ this._box.insert_actor(menuItem.actor, position); > >+ else > >+ this._box.add(menuItem.actor); > > if you want to split this patch out and just commit it now, go ahead. Pushed.
Created attachment 174632 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library. I tested the agent and had to make some fixes. Unfortunately, I cannot complete pairing (the UI appears for a moment, just to be immediately destroyed by CancelRequest).
Created attachment 174710 [details] [review] popupMenu: add expand/align flags to menuitem layout Mixing submenu menuitems and toggle menuitems results in poor layout. The fix is to right-align the submenu arrows. Since we already need to right-align the battery percentages as well, add alignment support to PopupBaseMenuItem.addActor(), and update stuff for that.
Created attachment 174711 [details] [review] popupMenu: minor submenu improvements Use a Unicode triangle character rather than a ">" for the submenu indicator, and have the submenu appear to the left of the parent rather than to the right, since for the status are menus, putting it on the right would put it off screen. (This will become irrelevant when the submenu style is redone.) Also, fix boxpointer to not ever draw the arrow overlapping the rounded box corners (or even immediately adjacent to them, since that also looks bad).
Review of attachment 174710 [details] [review]: ::: js/ui/popupMenu.js @@ +156,3 @@ + } + + this._children.push(params); The code uses child, but stores params.actor, which means it will break if I happen to pass a different actor. This should not be allowed: don't make "actor" a parameter, instead add it to the params object later, just before pushing it to _children.
So, when I try to pair one of my computers to the other, I seem to be getting a libnotify-based notification about it, not the direct-from-javascript notification
That's because you didn't kill bluetooth-applet. I don't know if the icon should kill it, in GNOME 3.0 it won't be autostarted so no need to kill it.
Created attachment 174971 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library. Finally tested, I can confirm that it works with my phone. It requires some designer work on agent notification, though.
Comment on attachment 174971 [details] [review] Bluetooth status indicator >+ js/misc/config.js need to add this to .gitignore too >+ this._killswitch = new PopupMenu.PopupSwitchMenuItem(_("Bluetooth"), false); If I turn this off, I get some warnings from inside the applet code. If I turn it back on, I get a crash. >+ this.addBody(_("Please enter the PIN mentioned on the device.")); >+ >+ this._entry = new St.Entry(); >+ this.addActor(this._entry); need to CSS-style this; it comes out with no border, so you can't even see that the entry is there. also, typing Return in the entry should be equivalent to pressing OK, and typing Escape should be equivalent to Cancel. >+ if (this._numeric) >+ this._applet.agent_reply_pincode(this._devicePath, parseInt(this._entry.text)); >+ else >+ this._applet.agent_reply_passkey(this._devicePath, this._entry.text); I'm not sure if it's a bug here or in the applet code, but when I try to pair one of my computers to the other, it gives me a numeric PIN to type, and then when I type it, it crashes inside the applet code, in some code path where it has a number but some part of gnome-bluetooth is apparently expecting a string instead. >+ <autotools id="gnome-control-center"> when this does get committed, make sure there are no now-redundant module additions
Created attachment 176603 [details] [review] Bluetooth status indicator Introduce the new Bluetooth indicator in the System Status area. It is written in JS with St and uses the new GnomeBluetoothApplet library.
(In reply to comment #22) > (From update of attachment 174971 [details] [review]) > >+ this._killswitch = new PopupMenu.PopupSwitchMenuItem(_("Bluetooth"), false); > > If I turn this off, I get some warnings from inside the applet code. If I turn > it back on, I get a crash. bug 637476 > also, typing Return in the entry should be equivalent to pressing OK, and > typing Escape should be equivalent to Cancel. I'm not sure about Escape (you should be able to ignore the notification and handle it later), but done. > >+ if (this._numeric) > >+ this._applet.agent_reply_pincode(this._devicePath, parseInt(this._entry.text)); > >+ else > >+ this._applet.agent_reply_passkey(this._devicePath, this._entry.text); > > I'm not sure if it's a bug here or in the applet code, but when I try to pair > one of my computers to the other, it gives me a numeric PIN to type, and then > when I type it, it crashes inside the applet code, in some code path where it > has a number but some part of gnome-bluetooth is apparently expecting a string > instead. There was a bug there (pincode swapped by passkey), but the expected outcome (with master gnome-bluetooth), should have been gjs complaining about wrong parameters (or automatically coercing them), not a segfault. Anyway, fixed and tested.
Created attachment 176666 [details] [review] fixes fix a function name, remove some debugging
Comment on attachment 176603 [details] [review] Bluetooth status indicator ok to commit after squashing the attached fixes
Comment on attachment 176603 [details] [review] Bluetooth status indicator Pushed with the attached fixes (plus removing the duplicate gnome-control-center module)