GNOME Bugzilla – Bug 621707
Experimental network manager applet
Last modified: 2011-03-16 15:05:01 UTC
Created attachment 163749 [details] [review] Patch for experimental NMApplet I promised it in bug 621017, so here it goes, ready to be integrated in experimental branch system-status. Depends on proposed patch for bug 621311 and bug 621356, and heavily on bug 621705 for user interface.
Created attachment 163909 [details] [review] Second iteration of NMApplet More features added, like icons for wireless signal strength, VPN and mobile broadband, network settings, automatic creation of wireless connection for a new access point (sometimes). Also adapted to match the mockups (with switches from bug 621880) and fixed some bugs.
Created attachment 164358 [details] [review] Third iteration for NMApplet Some more bugfixes, the addition of symbolic icons from GNOME theme, support for bluetooth, mobile broadband (with signal indicator).
Comment on attachment 163749 [details] [review] Patch for experimental NMApplet as discussed in bug 623656, we are not going to do it this way (although parts of the UI code here will probably be reusable in the eventual implementation).
*** Bug 591704 has been marked as a duplicate of this bug. ***
Comment on attachment 164358 [details] [review] Third iteration for NMApplet It turns out that nm-applet will not be there in GNOME 3.0 (shell version), so this patch is not that bad. Of course, a version with NMGlib from introspection would be cleaner.
Giovanni: if you are interested in this, you should mail Dan Williams <dcbw@redhat.com> and figure out a time to meet on IRC and talk about what needs doing to get the shell acting as the menu for NetworkManager 0.9.
Created attachment 179325 [details] [review] PopupMenu: introduce PopupMenuSection Complex popup menus require the ability to manager sequences of items as "sections", to which you can add and and remove items, as well as hide and show. PopupMenuSection does exactly that, leveraging the existing machinery for submenus, but without being exposed as a submenu to the user.
Created attachment 179326 [details] [review] PopupMenu: make parameters overridable in items Make all subclasses of PopupMenuBase accept a params argument, which can be used to make the item non reactive, not responsive to hover and, as a new feature, with a different style class.
Created attachment 179327 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and requires the rm-userset unstable branch.
The indicator just attached is supposed to be mostly feature complete (missing are WiMax support, and wireless dialogs, including for connecting to secure networks) - enough for UI freeze, at least. It has a known bugs (related to GJS marshalling of ByteArray, and possibly other - it complains generically about a TypeError in marshalling), and little testing, so by attaching I hope that a more widespread audience will help with making this work. How to test: you need the rm-userset git branch of NetworkManager (not in master yet), and you must run it instead of the distro provided NM, which in turn requires a newer wpa_supplicant (I picked it from fedora-rawhide - seems to be very customized for distros so don't recommend building) and turning SELinux off or permissive. Also don't forget to add symlinks from ~/gnome-shell/install/etc/{NetworkManager,sysconfig} to /etc, or you won't get any connection. Note that GConf connections are not available, so make it global before testing. (Or try network-manager-applet from git, should have some form of migration). Differences from the designs: VPN is a section, rather than a menu item. I believe that for the few people using it, having an handy list of connections is better, and for the others, you should just hide everything, and point them to control center. It does not use a spinner for scanning devices - NM does not give us this information. Device status is not shown beside the section title (it would be overtly complex to do it like that), instead it is an inner menu item, like the previous mockup).
Comment on attachment 179326 [details] [review] PopupMenu: make parameters overridable in items >+.popup-inactive-menu-item { >+ font-style: italic; >+} this is not used in this patch... >+ // GJS bug: it complains when an optional property is undefined >+ if (styleClass) { >+ this.box = new St.BoxLayout({ style_class: styleClass, >+ vertical: true }); >+ } else { >+ this.box = new St.BoxLayout({ vertical: true }); >+ } I don't think this is a bug; undefined is not the same thing as null (even though they compare equal). "styleClass || null" will probably do what you want. Or you could use add_style_class_name() separately like in the menuitem case. > getMenuItems: function() { >- return this.box.get_children().map(function (actor) { return actor._delegate; }).filter(function(item) { return item instanceof PopupBaseMenuItem; }); >+ return this.box.get_children().map(function (actor) { return actor._delegate; }).filter(function(item) { return item instanceof PopupBaseMenuItem || item instanceof PopupMenuSection; }); This belongs in the sections patch, not here.
Comment on attachment 179325 [details] [review] PopupMenu: introduce PopupMenuSection >+ _connect_sub_menu_signals: function(object, menu) { _connectSubMenuSignals (and likewise _connectItemSignals) >+ if (menuItem instanceof PopupMenuSection) { >+ this._connect_sub_menu_signals(menuItem, menuItem); >+ menuItem._activateId = menuItem._subMenuActivateId; >+ menuItem._activeChangeId = menuItem._subMenuActiveChangeId; this seems messy... why do you need two copies of those properties? >+ this._connect_sub_menu_signals(menuItem, menuItem.menu); and this is also very weird... you call the method with different arguments in the two cases... and one of the classes also overrides the method definition. There's got to be a cleaner way to organize the three classes. >+ // Override to avoid closing when an item is activated it may be simpler to just have open()/close() be no-ops for MenuSection, and have hide()/show() methods on them to do the hiding/showing.
(In reply to comment #11) > (From update of attachment 179326 [details] [review]) > >+.popup-inactive-menu-item { > >+ font-style: italic; > >+} > > this is not used in this patch... But this patch is exactly to enable that class to have an effect, so it makes most sense here. > >+ // GJS bug: it complains when an optional property is undefined > >+ if (styleClass) { > >+ this.box = new St.BoxLayout({ style_class: styleClass, > >+ vertical: true }); > >+ } else { > >+ this.box = new St.BoxLayout({ vertical: true }); > >+ } > > I don't think this is a bug; undefined is not the same thing as null (even > though they compare equal). "styleClass || null" will probably do what you > want. Or you could use add_style_class_name() separately like in the menuitem > case. It is. It complains explicitly if styleClass is undefined (even if GObject would have no problem with that property unset), whereas it does not for null, if the property accepts it (objects and strings do). Probably I should check for exact equality to undefined though, so null and empty strings are passed to GObject. > > getMenuItems: function() { > >- return this.box.get_children().map(function (actor) { return actor._delegate; }).filter(function(item) { return item instanceof PopupBaseMenuItem; }); > >+ return this.box.get_children().map(function (actor) { return actor._delegate; }).filter(function(item) { return item instanceof PopupBaseMenuItem || item instanceof PopupMenuSection; }); > > This belongs in the sections patch, not here. Yep. Will move.
Created attachment 179894 [details] [review] PopupMenu: make parameters overridable in items Make all subclasses of PopupMenuBase accept a params argument, which can be used to make the item non reactive, not responsive to hover and, as a new feature, with a different style class.
Created attachment 179896 [details] [review] PopupMenu: introduce PopupMenuSection Complex popup menus require the ability to manager sequences of items as "sections", to which you can add and and remove items, as well as hide and show. PopupMenuSection does exactly that, leveraging the existing machinery for submenus, but without being exposed as a submenu to the user.
Review of attachment 179327 [details] [review]: Two trivial problems found during runtime testing are listed below. ::: js/ui/status/network.js @@ +319,3 @@ + if (this.connections.length > 0) { + for(let j = 0; j < this.connections.length; ++j) { + let obj = this.connections[i]; Should be "this.connections[j]" @@ +321,3 @@ + let obj = this.connections[i]; + if (obj.connection == activeConnection) + continue; Should be "this.activeConnection" if I'm not mistaken
Some UI comments about the applet: * There is a chunk of empty space at the top of the menu. Approx one line. * The first menu item is "Enable networking" even when the network is enabled. - Should probably be a ToggleItem? * Disabling wireless should probably hide other wireless options. - As an aside, the wireless options aren't implemented. * The text "Connect to hidden wireless network..." makes the menu too wide, it should probably be shortened. - "Connect manually..." ? + OSX uses "Join other network..." * Doesn't list any broadband networks (I have one bluetooth DUN network). * Flipping a wireless/broadband switch shouldn't close the menu. - This is applicable to the a11y applet too, though, so not specific to this.
(In reply to comment #17) > Some UI comments about the applet: > > * There is a chunk of empty space at the top of the menu. Approx one line. > * The first menu item is "Enable networking" even when the network is enabled. > - Should probably be a ToggleItem? > * Disabling wireless should probably hide other wireless options. These should be fixed in a following patch (which accounts for the change in PopupMenuSection after the review). > * The text "Connect to hidden wireless network..." makes the menu too wide, it > should probably be shortened. > - "Connect manually..." ? > + OSX uses "Join other network..." Waiting for designer input here. > * Doesn't list any broadband networks (I have one bluetooth DUN network). You need to configure the connection system wide. I don't know how to do it for bluetooth, which is not in nm-connection-editor. Otherwise run nm-applet from git (repo network-manager-applet, branch rm-userset), should migrate all connections to the system. > * Flipping a wireless/broadband switch shouldn't close the menu. > - This is applicable to the a11y applet too, though, so not specific to this. Again, waiting for designer input.
Created attachment 180226 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and requires the rm-userset unstable branch.
(In reply to comment #18) > > * The text "Connect to hidden wireless network..." makes the menu too wide, it > > should probably be shortened. > > - "Connect manually..." ? > > + OSX uses "Join other network..." > > Waiting for designer input here. This is in the "More..." submenu right? I think "Other network..." may be fine here because the join is implicit and the menu only serves one purpose. The "hidden" part isn't needed because it can be used to connect to networks that aren't hidden but just can't be fit into the menu or something. > > * Flipping a wireless/broadband switch shouldn't close the menu. > > - This is applicable to the a11y applet too, though, so not specific to this. > > Again, waiting for designer input. Good question. I wonder if it would be weird to have the row activation close the menu but direct manipulation of the switch not close it.
> Good question. I wonder if it would be weird to have the row activation close > the menu but direct manipulation of the switch not close it. The only issue I can see is that it presents a smaller target for people that aren't very accurate, and people using keynav would be unable to have the ability.
Created attachment 181051 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager. Updated to the latest API break from NetworkManager. Now featuring working wireless dialogs, if you include bug 642503.
Created attachment 181255 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs. Fixed and tested some more. Now I can disconnect and reconnect.
Created attachment 181414 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs.
Comment on attachment 179894 [details] [review] PopupMenu: make parameters overridable in items (In reply to comment #13) > > this is not used in this patch... > > But this patch is exactly to enable that class to have an effect, so it makes > most sense here. I think it makes more sense to add it in the patch that actually uses that style. > > >+ // GJS bug: it complains when an optional property is undefined > > I don't think this is a bug; undefined is not the same thing as null > It is. It complains explicitly if styleClass is undefined (even if GObject > would have no problem with that property unset), whereas it does not for null, > if the property accepts it (objects and strings do). gjs GObject constructors don't work that way. Each value you include in the object literal is passed to g_object_new(). When you say "style_class: null", that doesn't mean "don't set style_class", it means, "explicitly set style_class to NULL" (which has no effect in this case since that's the default value anyway, but if that *wasn't* the default value, then you'd see a difference). So passing undefined there should mean "set this property to undefined", but undefined doesn't correspond to any type known to glib, so gjs can't do that and you get an error. (The exact error message gjs gives in this case is sort of weird, but that's the underlying issue.)
Comment on attachment 179896 [details] [review] PopupMenu: introduce PopupMenuSection >+ _connectSubMenuSignals: function(object, menu) { This could really use a comment explaining what object and menu are. > getMenuItems: function() { Hm... ok. I feel like the two places that use this now (addMenuItem and removeAll) are doing completely unrelated things... addMenuItem wants "all things that count as a position for purposes of counting menu items", and removeAll wants "all things that have to be destroyed because they won't be destroyed by someone else". And it just so happens that those two different things match the same objects right now. But that's weird. I guess it would be weirder to have two identical-but-differently-named methods, so we can keep it like this for now, but let's make it a private method, so no one else ends up depending on any particular semantics.
Comment on attachment 181414 [details] [review] Status area: add NetworkManager indicator >+ <autotools id="NetworkManager" autogenargs="--enable-introspection"> >+ <branch repo="anongit.freedesktop.org" module="NetworkManager/NetworkManager.git" checkoutdir="NetworkManager" /> >+ <dependencies> >+ <dep package="glib"/> >+ </dependencies> >+ </autotools> I don't think we can add NetworkManager to our jhbuild config; it needs to run as root (and in fact, you can't even fully jhbuild it as non-root, since it installs some files in / rather than $(prefix)). I think we just have to say that to use the NM status icon, you need to have a new-enough system package of NetworkManager. network-manager-applet doesn't have the same problems, but if we're requiring a new-enough system NetworkManager, the system will also have a new network-manager-applet along with it.
Created attachment 181501 [details] [review] PopupMenu: make parameters overridable in items Make all subclasses of PopupMenuBase accept a params argument, which can be used to make the item non reactive, not responsive to hover and, as a new feature, with a different style class.
Created attachment 181502 [details] [review] PopupMenu: introduce PopupMenuSection Complex popup menus require the ability to manager sequences of items as "sections", to which you can add and and remove items, as well as hide and show. PopupMenuSection does exactly that, leveraging the existing machinery for submenus, but without being exposed as a submenu to the user. Also, make getMenuItems() private, since it is used for different things now and may change semantics in the future.
Created attachment 181504 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs. I did not remove NetworkManager from the moduleset, as people may find practical to jhbuild it, even if manually (and it is unlikely that current stable distros will ship NM 0.9)
Attachment 181501 [details] pushed as 6e23654 - PopupMenu: make parameters overridable in items Attachment 181502 [details] pushed as f34ce92 - PopupMenu: introduce PopupMenuSection
(In reply to comment #30) > (and it is unlikely that current stable distros will ship NM 0.9) right, I was suggesting that we should just not even bother trying to make the new applet work for people who would have to build NM themselves, because it's going to be too much of a hassle. Case in point, I have not yet managed to get it working on my machine. I've done: killall nm-applet sudo service NetworkManager stop sudo /opt/gnome-shell/sbin/NetworkManager /opt/gnome-shell/bin/nm-applet but then trying to join a wireless network immediately gives: ** (nm-applet:12798): WARNING **: <WARN> add_and_activate_cb(): Failed to add/activate connection: (32) Error checking authorization: GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: Action org.freedesktop.NetworkManager.settings.modify.system is not registered and trying to join a wired network almost succeeds, but then gives: ** (nm-applet:17720): WARNING **: fetch_connections_done: error fetching connections: (19) Method "ListConnections" with signature "" on interface "org.freedesktop.NetworkManager.Settings" doesn't exist
Comment on attachment 181504 [details] [review] Status area: add NetworkManager indicator Reviewing blind; I have not yet managed to test this >+ModemGsm.prototype = { >+ _init: function(path) { >+ this._proxy = new ModemGsmNetworkProxy(DBus.system, 'org.freedesktop.ModemManager', path); indentation >+ _registrationInfoChanged: function(status, code, name) { shouldn't that be function(proxy, status, code, name) ? I guess that means you haven't been able to test this code? Any other parts that are untested? >+ this._proxy.GetSignalQualityRemote(Lang.bind(this, this._qualityChanged)); >+ this._proxy.GetServingSystemRemote(Lang.bind(this, function(status, name, code) { bit odd that you use an inline function here, but not anywhere else >+function macToArray(string) { >+ const regexp = /([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2})/; Sort of heavyweight. You could just do string.split(':'). >+function makeClassKey(object) { >+ return Object.prototype.toString.call(object); I think "object.constructor.name" would work fine for this. >+function addChipersFromFlags(settings, flags) { typo ("Chipers") >+function signalToIcon(value) { >+ if(value > 80) space before "(" >+ this._title = title || String(accessPoint.get_ssid()); Presumably get_ssid() returns a bytearray rather than a string because it may be non-UTF8? In which case, trying to convert it to a String will throw an error. We'd need to sanitize it somehow. See what nm-applet does, since GtkLabel would have the same problem. >+ _get_icon: function() { _getIcon >+function NMDevice() { >+ throw TypeError('Instantanting abstract class NMDevice'); need "new" before TypeError. (same mistake also occurs elsewhere) >+NMDevice.prototype = { >+ _init: function(client, device, connections, activeConnectionPosition) { >+ /* <protected> */ "// protected". No need for the special gtk-doc syntax here. Also, I think the precedent elsewhere is to use _-prefixing for "protected" vars/methods. >+ this.titleItem = new PopupMenu.PopupMenuItem(this.device._description, { reactive: false, style_class: 'popup-subtitle-menu-item' }); newline before style_class >+ // record the connection >+ let obj = { >+ connection: connection, >+ name: connection._name, >+ uuid: connection._uuid, >+ }; >+ this.connections.push(obj); I think you need a "connection" class. This particular "constructor" appears in two different places, and then various other bits of code make various other assumptions about those objects. >+ if (!this.device || >+ (this.device.state == NetworkManager.DeviceState.DISCONNECTED || >+ this.device.state == NetworkManager.DeviceState.ACTIVATED)) { >+ // we need to show this connection totally unclear why we want to show it in that case, but not others. I think it would make more sense to have a comment before the if(), explaining what kinds of connections we want to show (and why that corresponds to this condition, if it's not obvious at that point). >+ log ('Cannot remove a connection without an UUID'); no space before "(". (Also, "a UUID".) >+ return _("connecting..."); lowercase seems out of place here, but I haven't seen how it actually looks in the menu. Maybe it works. >+ this.autoConnectionName = _("Auto Ethernet"); isn't that name available from NM somewhere? (and if not, shouldn't it be?) >+ if (connection._type != '802-3-ethernet') that constant ought to be available as NetworkManager.SETTING_WIRED_SETTING_NAME (likewise with some other constant strings) >+ _get_signal_icon: function() { _getSignalIcon >+ // XXX: breaking the layers with this, but cannot call >+ // this.connectionValid until I have a device >+ this.device = device; would moving the NMDevice.prototype._init() to here instead of at the end work? >+ obj = { name: name, >+ connections: [ ], >+ item: null, >+ accessPoints: [ ap ] >+ }; >+ this._accessPoints.push(obj); another case where we might want an actual class. Both the "constructor" and the sorting method are duplicated. Also, should this._accessPoints maybe be this._networks or something, since each element might consist of multiple access points. >+ if (fixedBand == 'a' && (freq < 4915 || freq > 5825)) mmm... magic >+ if (!this._overflowItem) { >+ this._overflowItem = new PopupMenu.PopupSubMenuMenuItem(_("More...")); >+ } >+ this._overflowItem.menu.addMenuItem(menuItem); We're going to need some design for the case where there are more networks available than can fit on your screen... :-/ >+ this._allSections = [ ]; a bit odd that allSections does not contain statusSection... >+ this._read_connections(); >+ this._read_devices(); >+ this._sync_nm_state(); camelCase (etc, etc) >+ //this._sync_nm_state(); Should that be there or not? >+ this._source = new MessageTray.Source(_("Network Manager")); >+ let icon = new St.Icon({ icon_name: 'network-transmit-receive', >+ icon_type: St.IconType.SYMBOLIC, >+ icon_size: this._source.ICON_SIZE >+ }); >+ this._source._setSummaryIcon(icon); That's a protected method... an easy fix would be to make MessageTray.SystemInformationSource() take optional title and iconName arguments, and then use that. >+ this._client.connect('notify::' + type + '-enabled', handler); >+ this._client.connect('notify::' + type + 'hardware-enabled', handler); missing "-" before "hardware" I think >+ // we don't connect to 'updated' because GJS currently cannot deliver it >+ // (it needs to type overrides from GIR) >+ //connection.connect('updated', Lang.bind(this, this._update_connection)); hm?
(In reply to comment #32) > (In reply to comment #30) > > (and it is unlikely that current stable distros will ship NM 0.9) > > right, I was suggesting that we should just not even bother trying to make the > new applet work for people who would have to build NM themselves, because it's > going to be too much of a hassle. > > Case in point, I have not yet managed to get it working on my machine. I've > done: > > killall nm-applet > sudo service NetworkManager stop > sudo /opt/gnome-shell/sbin/NetworkManager > /opt/gnome-shell/bin/nm-applet > > but then trying to join a wireless network immediately gives: > > ** (nm-applet:12798): WARNING **: <WARN> add_and_activate_cb(): Failed to > add/activate connection: (32) Error checking authorization: > GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: Action > org.freedesktop.NetworkManager.settings.modify.system is not registered As the error says, the PolicyKit action is not registered, because it is installed in /opt/gnome-shell/share/polkit-1/actions, instead of /usr/share/polkit-1/actions. Unfortunately, replacing NetworkManager 0.8 policy with the 0.9 will stop the system NM from working, that's why I did not test everything which requires authentication. > and trying to join a wired network almost succeeds, but then gives: > > ** (nm-applet:17720): WARNING **: fetch_connections_done: error fetching > connections: (19) Method "ListConnections" with signature "" on interface > "org.freedesktop.NetworkManager.Settings" doesn't exist Maybe your NM crashed and dbus-launch picked the system one? (In reply to comment #33) > (From update of attachment 181504 [details] [review]) > Reviewing blind; I have not yet managed to test this > > >+ _registrationInfoChanged: function(status, code, name) { > > shouldn't that be function(proxy, status, code, name) ? I guess that means you > haven't been able to test this code? Any other parts that are untested? Everything which requires PolicyKit, and everything for which system connections cannot be created under NM 0.8 (so wwan, dsl, bluetooth). > >+ this._proxy.GetSignalQualityRemote(Lang.bind(this, this._qualityChanged)); > >+ this._proxy.GetServingSystemRemote(Lang.bind(this, function(status, name, code) { > > bit odd that you use an inline function here, but not anywhere else Will fix. > >+function macToArray(string) { > >+ const regexp = /([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2}):([A-Fa-f0-9]{2})/; > > Sort of heavyweight. You could just do string.split(':'). Ugh. Didn't think of that. Thanks! > >+function makeClassKey(object) { > >+ return Object.prototype.toString.call(object); > > I think "object.constructor.name" would work fine for this. Even better, libnm-glib has NMDevice.get_device_type() now, so this hack can be removed. > >+ this._title = title || String(accessPoint.get_ssid()); > > Presumably get_ssid() returns a bytearray rather than a string because it may > be non-UTF8? In which case, trying to convert it to a String will throw an > error. We'd need to sanitize it somehow. See what nm-applet does, since > GtkLabel would have the same problem. Will check. > >+ // record the connection > >+ let obj = { > >+ connection: connection, > >+ name: connection._name, > >+ uuid: connection._uuid, > >+ }; > >+ this.connections.push(obj); > > I think you need a "connection" class. This particular "constructor" appears in > two different places, and then various other bits of code make various other > assumptions about those objects. This particular object is private to NMDevice and derived classes (instance private), I'd like to avoid the code overhead of a full-blown NMConnection class. > >+ if (!this.device || > >+ (this.device.state == NetworkManager.DeviceState.DISCONNECTED || > >+ this.device.state == NetworkManager.DeviceState.ACTIVATED)) { > >+ // we need to show this connection > > totally unclear why we want to show it in that case, but not others. I think it > would make more sense to have a comment before the if(), explaining what kinds > of connections we want to show (and why that corresponds to this condition, if > it's not obvious at that point). Connections are shown if the device is disconnected or activated. The other states show a message instead. (The !this.device hack is for NMDeviceVpn). > >+ return _("connecting..."); > > lowercase seems out of place here, but I haven't seen how it actually looks in > the menu. Maybe it works. The older mockup in the wiki has lower case. Given that the new one is not readily implementable, I'm tracking that. > >+ this.autoConnectionName = _("Auto Ethernet"); > > isn't that name available from NM somewhere? (and if not, shouldn't it be?) No, libnm-glib is not translated. nm-applet has this string as well. > >+ // XXX: breaking the layers with this, but cannot call > >+ // this.connectionValid until I have a device > >+ this.device = device; > > would moving the NMDevice.prototype._init() to here instead of at the end work? No. NMDevice.prototype._init calls createSection(), which requires _accessPoints to be filled with data. > >+ obj = { name: name, > >+ connections: [ ], > >+ item: null, > >+ accessPoints: [ ap ] > >+ }; > >+ this._accessPoints.push(obj); > > another case where we might want an actual class. Both the "constructor" and > the sorting method are duplicated. I may refactor the sorting part inside NMAccessPointMenuItem, but I'd like to avoid a class for the wrapper object. > Also, should this._accessPoints maybe be this._networks or something, since > each element might consist of multiple access points. Ok. > >+ if (fixedBand == 'a' && (freq < 4915 || freq > 5825)) > > mmm... magic It says it is copied from nm-applet. I am as clueless as you about those numbers, unfortunately. > >+ if (!this._overflowItem) { > >+ this._overflowItem = new PopupMenu.PopupSubMenuMenuItem(_("More...")); > >+ } > >+ this._overflowItem.menu.addMenuItem(menuItem); > > We're going to need some design for the case where there are more networks > available than can fit on your screen... :-/ There is not much we can do, beside making PopupMenu scrollable like the Gtk one, the "More..." submenu is supposed to show all available networks. > >+ this._allSections = [ ]; > > a bit odd that allSections does not contain statusSection... _allSections means "all sections that need to be closed when NetworkManager's state is not good for showing connections", it is just a convenience for _sync_nm_state. > > >+ //this._sync_nm_state(); > > Should that be there or not? No. Will kill. > >+ this._source = new MessageTray.Source(_("Network Manager")); > >+ let icon = new St.Icon({ icon_name: 'network-transmit-receive', > >+ icon_type: St.IconType.SYMBOLIC, > >+ icon_size: this._source.ICON_SIZE > >+ }); > >+ this._source._setSummaryIcon(icon); > > That's a protected method... an easy fix would be to make > MessageTray.SystemInformationSource() take optional title and iconName > arguments, and then use that. SystemInformationSource is a transient source. I want a persistent one (so you can check on which network you connected, if you missed it), but without the length of a custom subclass. > >+ // we don't connect to 'updated' because GJS currently cannot deliver it > >+ // (it needs to type overrides from GIR) > >+ //connection.connect('updated', Lang.bind(this, this._update_connection)); > > hm? 'updated' used to have a dbus-glib custom boxed type as the first argument, which was overridden from GI annotations. But since GJS does not use GI for signals (using only GType information), it had no effect. The signal has then been removed, so no problem.
Created attachment 181648 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs.
nm-applet doesn't show its menu like the others (accessibility, sound, chat). The others use a balloon like menu when clicked, and it can be seen while you are seeing "Activities" (you can do so with the others applets). I don't know if applet is the correct name for it, buy anyway.
Well, yes - this is what this bug is about ...
I'm getting an error while using your patch: JS ERROR: !!! Exception was: TypeError: devices is undefined JS ERROR: !!! lineNumber = '1557' JS ERROR: !!! fileName = '/usr/share/gnome-shell/js/ui/status/network.js' JS ERROR: !!! stack = '([object _private_NMClient_RemoteConnection])@/usr/share/gnome-shell/js/ui/status/network.js:1557 ()@/usr/share/gnome-shell/js/ui/status/network.js:1501 ([object _private_NMClient_RemoteSettings])@/usr/share/gnome-shell/js/ui/status/network.js:1186 ([object _private_NMClient_RemoteSettings])@/usr/share/gjs-1.0/lang.js:110 ' JS ERROR: !!! message = 'devices is undefined' I tried to debug this, but my js skills are really bad (and 'devices' seems to be defined the line before). I'm using the latest git versions of NetworkManager, nm-applet and gjs. NetworkManager and nm-applet work without the patch.
Created attachment 182113 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs. NetworkManager has broken API more recently - this should fix it.
The nm-applet dialog api has been committed.
Created attachment 182305 [details] [review] Updated patch with fixed dbus service name and paths for nm-applet Updated patch with the changes that I made to the patch I pushed to nm-applet.
Created attachment 182920 [details] [review] another iteration Here is another iteration of the patch, after some testing to weed out undefined variables and whatnot. The main problem for further testing at this point is: ValueError: Invalid object path '/org/gnome/network-manager-applet': contains invalid character '-' So, we need an nm-applet that uses a valid object path.
Created attachment 182967 [details] [review] another iteration This just adds a null check for wirelessSecuritySettings to avoid an exception out of wirelessSettings.ap_security_compatible
I hear the check in the last iteration is the wrong way around. Also noticed: if I manually disconnect wireless, the icon does not update
Restarting NM makes all devices show up twice
Disconnecting wired should make it so that one can actually reconnect. Currently, the wired connection continues to appear selected, and is not activatable in the menu. If I manually bring it up from the cmdline, 'System eth0' appears twice in the menu. It seems the duplicate filtering code just needs some more work.
Created attachment 183008 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs. Added support for NetworkManager restarting (by fixing the handlers for ConnectionRemoved and DeviceRemoved), and removed the display of notifications, which is done by nm-applet (but code is still there).
Created attachment 183009 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs. Fixed typo, thanks to Dan.
You need s/network-manager-applet/network_manager_applet/ in a few places to work with dcbws fixed object path and interface name
And CreateWirelessNetworkRemote must be changed to CreateWifiNetworkRemote
I've now used nm-connection-editor to create a system-wide vpn connection, and it actually shows up, hurray. But...clicking on it does nothing except show me a JS error: JS ERROR: !!! Exception in callback for signal: activate JS ERROR: !!! message = 'this._client is undefined' JS ERROR: !!! lineNumber = '391' JS ERROR: !!! fileName = '/usr/share/gnome-shell/js/ui/status/network.js' JS ERROR: !!! stack = '([object Object],[object _private_Clutter_Event])@/usr/share/gnome-shell/js/ui/status/network.js:391
I notice that vpn connections get not cleared from the menu when I restart nm. I also see duplicated vpn connections.
I have now succeeded in activating a vpn connection via nmcli. The network menu did notice and put up a different icon in the panel. It looks a bit wierd though, like a padlock on a stick... Restarting the shell with an active vpn connection did mark the vpn connection as active in the vpn section, but put the plain wireless icon in the menu, instead of the wierd padlock-on-a-stick one.
Trying 'Disconnect' in the vpn section again gives me: JS ERROR: !!! Exception in callback for signal: activate JS ERROR: !!! message = 'this.device is null' JS ERROR: !!! lineNumber = '408' JS ERROR: !!! fileName = '/usr/share/gnome-shell/js/ui/status/network.js' JS ERROR: !!! stack = '([object Object],[object _private_Clutter_Event])@/usr/share/gnome-shell/js/ui/status/network.js:408
Dropping the vpn connection with nmcli does not update the selected item in the vpn section.
Created attachment 183074 [details] [review] popupMenu: don't include hidden items in the width computation
(In reply to comment #46) > Disconnecting wired should make it so that one can actually reconnect. > Currently, the wired connection continues to appear selected, and is not > activatable in the menu. > > If I manually bring it up from the cmdline, 'System eth0' appears twice in the > menu. It seems the duplicate filtering code just needs some more work. Fixed (== instead of =). Weirdly enough, it did not appear in my system (perhaps because I have more than one wired connection). (In reply to comment #49) > You need s/network-manager-applet/network_manager_applet/ in a few places to > work with dcbws fixed object path and interface name (In reply to comment #50) > And CreateWirelessNetworkRemote must be changed to CreateWifiNetworkRemote (In reply to comment #51) Fixed. > I've now used nm-connection-editor to create a system-wide vpn connection, and > it actually shows up, hurray. But...clicking on it does nothing except show me > a JS error: > > JS ERROR: !!! Exception in callback for signal: activate > JS ERROR: !!! message = 'this._client is undefined' > JS ERROR: !!! lineNumber = '391' > JS ERROR: !!! fileName = > '/usr/share/gnome-shell/js/ui/status/network.js' > JS ERROR: !!! stack = '([object Object],[object > _private_Clutter_Event])@/usr/share/gnome-shell/js/ui/status/network.js:391 Fixed (client was not passed to NMDeviceVPN constructor). (In reply to comment #52) > I notice that vpn connections get not cleared from the menu when I restart nm. > I also see duplicated vpn connections. Fixed (disposing of connection cleared _uuid too early). (In reply to comment #53) > I have now succeeded in activating a vpn connection via nmcli. The network menu > did notice and put up a different icon in the panel. It looks a bit wierd > though, like a padlock on a stick... > > Restarting the shell with an active vpn connection did mark the vpn connection > as active in the vpn section, but put the plain wireless icon in the menu, > instead of the wierd padlock-on-a-stick one. The first time you saw the vpn icon because it was activating. It was not updated to the wireless icon because of a bug in _notifyActiveConnection. Also, now vpn connections are preferred to normal ones (but pending design on the possibility of showing two icons or using a GEmblemedIcon). The padlock-on-a-stick is network-vpn, from gnome-icon-theme-symbolic. (In reply to comment #54) > Trying 'Disconnect' in the vpn section again gives me: > > JS ERROR: !!! Exception in callback for signal: activate > JS ERROR: !!! message = 'this.device is null' > JS ERROR: !!! lineNumber = '408' > JS ERROR: !!! fileName = > '/usr/share/gnome-shell/js/ui/status/network.js' > JS ERROR: !!! stack = '([object Object],[object > _private_Clutter_Event])@/usr/share/gnome-shell/js/ui/status/network.js:408 Fixed (VPN needs nm_client_deactivate_connection, not nm_device_disconnect).
Created attachment 183081 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service and for wireless dialogs.
http://people.gnome.org/~danw/too-many-networks.png maybe the easiest fix for now would be to add another "More..." that went to the Network control panel after a certain point?
After sitting down with designers today, here is a list of short-term recommendations for the menu for 3.0 (also sent to the release-team list): - Make VPN connections work - Remove 'Disconnect' from the menu. It is a rarely useful feature, adds confusing states, and does not work correctly right now. - Change the Wired section to be one line per device (as in the design); ie move the 'unplugged' text up in the same line and add a on/off switch for the device like we have for wireless - Drop 'Other...' and 'Create...' items from the menu. Again, rarely used feature that we should make available from the network panel - Limit the length of the 'More...' section to avoid overflowing the screen (probably need to add some indicator that there's still more) - Figure out what is making the menu so wide, and fix it - Don't overlay the padlock icon in the panel, it doesn't work - Merge it Seems like we already have some of that covered in todays patches. Nice.
Created attachment 183092 [details] [review] mobile broadband fixes Notes beyond what's in the patch: - Not sure if this is intentional, but it shows 2 lines for each modem: the first gives the provider name and signal strength (and is insensitive), and the second shows the connection name and can be selected - When you enable a broadband connection, _updateIcon() ends up getting called before _syncActiveConnection() does, and so mainConnection._primaryDevice is not yet set, and _updateIcon() ends up throwing an exception - NM itself seems to not let you just turn on Mobile Broadband like you can with other types. You have to pick a specific connection to connect to, and then that enables Mobile Broadband. This is inconsistent and weird... (And it's especially a problem for the status icon, due to the bug mentioned in the patch, where when Mobile Broadband gets disabled, it hides the "Create New Connection" item - In CDMA (but not GSM), the menu items are slightly confused, and the connection labelled "New connection" actually activates the first configured CDMA connection...
(In reply to comment #60) > - Merge it One problem here is the lack of real NM packages. How many days til 0.9? Maybe we should make panel.js do: try { STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['network'] = imports.ui.status.network.NMApplet; } catch (e) {} if the user doesn't have NM 0.9ish, then the "imports.gi.NetworkManager" in network.js will throw, and then we'll just fall back to allowing the trayicon.
(In reply to comment #60) > After sitting down with designers today, here is a list of short-term > recommendations for the menu for 3.0 (also sent to the release-team list): > > - Make VPN connections work Should work now. Tell me if you find anything that doesn't, unfortunately I have no VPN to test. > - Remove 'Disconnect' from the menu. It is a rarely useful feature, > adds confusing states, and does not work correctly right now. Yes, it is rarely used, but this is the network menu. You expect to find a "disconnect" in there. And it is useful, for example if you want to turn off a link-local connection, a dsl, an adhoc, a bridge, a VPN, or things like that, that have explicit "connected" states (as opposed to ethernet, which is "connect-and-go"). > - Change the Wired section to be one line per device (as in the > design); ie move the 'unplugged' text up in the same line and add a > on/off switch for the device like we have for wireless Sadly, that's difficult to implement cleanly. At the moment, section titles are managed by NMApplet, while their contents are managed by each NMDevice (a JS object around a native NMClient.Device), to allow for more than one device in each section. It would not be easy to code so that it can seamlessly go from one device per section to more than one and back. > - Drop 'Other...' and 'Create...' items from the menu. Again, rarely > used feature that we should make available from the network panel "Other..." is used, all the time you need to connect to hidden APs. I oppose to its removal. Btw, it has been in nm-applet all this time and nobody complained about it. (In reply to comment #61) > Created an attachment (id=183092) [details] [review] > mobile broadband fixes > > Notes beyond what's in the patch: > > - Not sure if this is intentional, but it shows 2 lines for each > modem: the first gives the provider name and signal strength (and > is insensitive), and the second shows the connection name and > can be selected Yes, this is expected. It matches the behavior of current nm-applet, and makes sense in case you're not connected, but the modem is active (for example because you want to make phone calls or receive sms). > - When you enable a broadband connection, _updateIcon() ends up > getting called before _syncActiveConnection() does, and so > mainConnection._primaryDevice is not yet set, and _updateIcon() > ends up throwing an exception This is weird. _updateIcon itself calls _syncActiveConnection at the very beginning. mainConnection._primaryDevice not set means we're getting ActiveConnections for devices we haven't seen yet, or that connection-to-device matching is not working. > - NM itself seems to not let you just turn on Mobile Broadband like > you can with other types. You have to pick a specific connection > to connect to, and then that enables Mobile Broadband. This is > inconsistent and weird... (And it's especially a problem for the > status icon, due to the bug mentioned in the patch, where when > Mobile Broadband gets disabled, it hides the "Create New Connection" > item Ugh. So it needs specific code, and another layering breakage (NMDevices need to be aware of wwan-enabled flag and act accordingly). > - In CDMA (but not GSM), the menu items are slightly confused, and > the connection labelled "New connection" actually activates the > first configured CDMA connection... Is it possible you actually had a connection named "New connection", somehow configured by the mobile broadband or NetworkManager itself? The auto item is named "Create New Connection" for both GSM and CDMA.
> > - Make VPN connections work > > Should work now. Tell me if you find anything that doesn't, unfortunately I > have no VPN to test. I'll try again, thanks. > > - Remove 'Disconnect' from the menu. It is a rarely useful feature, > > adds confusing states, and does not work correctly right now. > > Yes, it is rarely used, but this is the network menu. You expect to find a > "disconnect" in there. And it is useful, for example if you want to turn off a > link-local connection, a dsl, an adhoc, a bridge, a VPN, or things like that, > that have explicit "connected" states (as opposed to ethernet, which is > "connect-and-go"). All fringe functionality that should be in the network panel, if at all. Look, there is no less than 3 (!) equally named 'Disconnect' items in my menu, and the are all easily confused with the surrounding connections. Lets just clean this up. > > - Drop 'Other...' and 'Create...' items from the menu. Again, rarely > > used feature that we should make available from the network panel > > "Other..." is used, all the time you need to connect to hidden APs. I oppose to > its removal. Btw, it has been in nm-applet all this time and nobody complained > about it. Well, this is not about recreating nm-applet... I have never used this in my life. I agree that 'Create' is even more fringe than 'Other'. > Sadly, that's difficult to implement cleanly. At the very minimum, 'System eth0' has to go. No eth anything in the menu... > > - Not sure if this is intentional, but it shows 2 lines for each > > modem: the first gives the provider name and signal strength (and > > is insensitive), and the second shows the connection name and > > can be selected > > Yes, this is expected. It matches the behavior of current nm-applet, and makes > sense in case you're not connected, but the modem is active (for example > because you want to make phone calls or receive sms). It contributes to the vertical space problem.
(In reply to comment #62) > (In reply to comment #60) > > - Merge it > > One problem here is the lack of real NM packages. How many days til 0.9? dcbw's promise was: March 16. But I don't know what to make of Dans promises... > Maybe we should make panel.js do: > > try { > STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['network'] = > imports.ui.status.network.NMApplet; > } catch (e) {} > > if the user doesn't have NM 0.9ish, then the "imports.gi.NetworkManager" in > network.js will throw, and then we'll just fall back to allowing the trayicon. This sounds like a very good idea. I like it, it will make our distributors a lot happier if we don't have to force them to jump headfirst into nm 0.9.
Few more things I had forgotten earlier: - There's too many 'networks' in this menu. The section headings should just be 'Wired', 'Wifi' (or 'Wireless', but thats a bit generic), 'VPN'. And, if we end up keeping 'Other networks...', it should just be 'Other...' - We need to do something about the connection duplication. I very easily end up in a situation where the menu shows clasen_home > Auto clasen_home Auto clasen_home Auto clasen_home and everytime I try to connect to another unavailable connection, another 'Auto clasen_home' is added to that list.
VPN does still not work here, btw. Trying to activate a vpn item gives me: JS ERROR: !!! Exception in callback for signal: activate JS ERROR: !!! message = 'Argument 'device' (type interface) may not be null' JS ERROR: !!! lineNumber = '0' JS ERROR: !!! fileName = 'gjs_throw' JS ERROR: !!! stack = '("Argument 'device' (type interface) may not be null")@gjs_throw:0
(In reply to comment #64) > > > - Make VPN connections work > > > > Should work now. Tell me if you find anything that doesn't, unfortunately I > > have no VPN to test. > > I'll try again, thanks. > > > > - Remove 'Disconnect' from the menu. It is a rarely useful feature, > > > adds confusing states, and does not work correctly right now. > > > > Yes, it is rarely used, but this is the network menu. You expect to find a > > "disconnect" in there. And it is useful, for example if you want to turn off a > > link-local connection, a dsl, an adhoc, a bridge, a VPN, or things like that, > > that have explicit "connected" states (as opposed to ethernet, which is > > "connect-and-go"). > > All fringe functionality that should be in the network panel, if at all. > Look, there is no less than 3 (!) equally named 'Disconnect' items in my menu, > and the are all easily confused with the surrounding connections. > Lets just clean this up. DSL and VPNs are not "fringe functionality". Also, how do you disconnect a wireless connection without disabling wireless entirely and thus disconnecting all the other wireless cards, powering them down and preventing further scans (which means the menu has to be hidden)? > > > > - Drop 'Other...' and 'Create...' items from the menu. Again, rarely > > > used feature that we should make available from the network panel > > > > "Other..." is used, all the time you need to connect to hidden APs. I oppose to > > its removal. Btw, it has been in nm-applet all this time and nobody complained > > about it. > > Well, this is not about recreating nm-applet... > I have never used this in my life. > I agree that 'Create' is even more fringe than 'Other'. > > > Sadly, that's difficult to implement cleanly. > > At the very minimum, 'System eth0' has to go. No eth anything in the menu... NetworkManager's problem. "System eth0" is just the default connection name when you have something configured by system (for example with /etc/sysconfig/network-scripts or /etc/network/interfaces). > > > > - Not sure if this is intentional, but it shows 2 lines for each > > > modem: the first gives the provider name and signal strength (and > > > is insensitive), and the second shows the connection name and > > > can be selected > > > > Yes, this is expected. It matches the behavior of current nm-applet, and makes > > sense in case you're not connected, but the modem is active (for example > > because you want to make phone calls or receive sms). > > It contributes to the vertical space problem. How do propose to handle that? I want to show the operator name even if the device is not connected to the network. (In reply to comment #66) > Few more things I had forgotten earlier: > > - There's too many 'networks' in this menu. The section headings should just be > 'Wired', 'Wifi' (or 'Wireless', but thats a bit generic), 'VPN'. And, if we end > up keeping 'Other networks...', it should just be 'Other...' Ok, will change that, and also try to compact device statuses in the normal case. > - We need to do something about the connection duplication. I very easily end > up in a situation where the menu shows > > clasen_home > > Auto clasen_home > Auto clasen_home > Auto clasen_home > > and everytime I try to connect to another unavailable connection, another 'Auto > clasen_home' is added to that list. Ugh. I'll check that.(In reply to comment #67) > VPN does still not work here, btw. Trying to activate a vpn item gives me: > > JS ERROR: !!! Exception in callback for signal: activate > JS ERROR: !!! message = 'Argument 'device' (type interface) may not be > null' > JS ERROR: !!! lineNumber = '0' > JS ERROR: !!! fileName = 'gjs_throw' > JS ERROR: !!! stack = '("Argument 'device' (type interface) may not be > null")@gjs_throw:0 Ok, that's a problem in the introspection for libnm-glib. Will file a bug.
(In reply to comment #68) > > > DSL and VPNs are not "fringe functionality". Also, how do you disconnect a > wireless connection without disabling wireless entirely and thus disconnecting > all the other wireless cards, powering them down and preventing further scans > (which means the menu has to be hidden)? Why do you want to do that ? I've never wanted it. It seems like a pretty made-up request. And if you really want to something like that, it is easy enough to bring up the network settings. > > At the very minimum, 'System eth0' has to go. No eth anything in the menu... > > NetworkManager's problem. "System eth0" is just the default connection name > when you have something configured by system (for example with > /etc/sysconfig/network-scripts or /etc/network/interfaces). If there is only one wired device, and only one connection there is no advantage at all to showing the connection name. I don't really care if we show more information in the presence of multiple wired network devices or manually set up connections, because frankly, thats the < 1% case. Most people just need the menu to show them their wireless access points in an easily clickable way. > How do propose to handle that? I want to show the operator name even if the > device is not connected to the network. https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/Network?action=AttachFile&do=get&target=network-menu.png has about the level of information that I think is needed directly in the menu.
Created attachment 183236 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service. A major UI rework. Now next to all titles there is a switch, which is replaced by status text when the device is connecting or in error state (matching the mockups). Toggling the switch disconnects the currently active connection, but does not allow reconnecting again (this is because NetworkManager thinks in terms of connections, not devices), except for wireless and wwan, where it changes the global setting and has the expected behavior. If there are more than one devices in a section, the global switch / status text is removed (again, except for wireless and wwan) and each devices gains a subtile and a switch. This allowed me to kill the "disconnect" menu item. Also, I removed the wireless dialogs items, and all required to support them (including dbus dependency on nm-applet). nm-applet is still required to be running, and must be autostarted (not bus activated), or NetworkManager won't have needed connection secrets (PIN codes, WPA keys, VPN passwords, etc.)
Review of attachment 183074 [details] [review]: Tested, looks good. (And becomes expecially required with last iteration of NMApplet)
Overall, great improvement, thanks a lot! There is a sizing issue left that I am going to demonstrate with two screenshots. Two other small observations first: - The icon switches to 'wired' immediately upon plugging in the cable, while the menu still shows ...connecting. Should we wait with switching the icon until the connection is actually established ? - The text 'network cable unplugged' is a bit too long (see screenshot). Just 'cable unplugged' should be obvious enough.
Created attachment 183239 [details] unplugged text a bit too long
Created attachment 183240 [details] size/expand problem when plugging in a cable
Apart from the VPN problem (going to talk to dcbw about that - did you already file a bug ?), and the two issues mentioned above, this looks almost ready to land, from my perspective.
(In reply to comment #72) > Overall, great improvement, thanks a lot! > > There is a sizing issue left that I am going to demonstrate with two > screenshots. > Two other small observations first: > > - The icon switches to 'wired' immediately upon plugging in the cable, while > the menu still shows ...connecting. Should we wait with switching the icon > until the connection is actually established ? You were connected to two connections, were you? Of course, NetworkManager says it's generally connected, but the main connection (the one we should show) is not a connected one. Will fix. > - The text 'network cable unplugged' is a bit too long (see screenshot). Just > 'cable unplugged' should be obvious enough. Ok. (In reply to comment #74) > Created an attachment (id=183240) [details] > size/expand problem when plugging in a cable Did this happen with Dan's patch applied? I experienced something similar, related to status texts that are not shown, but that fixed it.
(In reply to comment #76) > (In reply to comment #72) > > Overall, great improvement, thanks a lot! > > > > There is a sizing issue left that I am going to demonstrate with two > > screenshots. > > Two other small observations first: > > > > - The icon switches to 'wired' immediately upon plugging in the cable, while > > the menu still shows ...connecting. Should we wait with switching the icon > > until the connection is actually established ? > > You were connected to two connections, were you? Of course, NetworkManager says > it's generally connected, but the main connection (the one we should show) is > not a connected one. Will fix. Yes, I had a wireless connection (and the correct icon for that was in the menu), and then I plugged in the cable. > > Did this happen with Dan's patch applied? > I experienced something similar, related to status texts that are not shown, > but that fixed it. This is with Dan's patch, yes
Created attachment 183243 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service.
Created attachment 183246 [details] [review] StTextureCache: generate icon names in the right order GThemedIcon expects the first name to be the most specific, and will thus prefer it to later ones. We thus need to order the names from the longer to the shorter.
Review of attachment 183246 [details] [review]: ::: src/st/st-texture-cache.c @@ +1420,3 @@ g_strfreev (parts); + + numnames = g_strv_length (names); Could do an in-place swap: for (i = 0; i < numnames / 2; i++) { char *tmp = names[numnames - i - 1]; names[numnames - i - 1] = names[i]; names[i] = tmp; } But up to you which you prefer. OK to commit like this, but you need a brief comment here like: /* longest (most-specific) name has to come first */
Comment on attachment 183246 [details] [review] StTextureCache: generate icon names in the right order Attachment 183246 [details] pushed as 0573487 - StTextureCache: generate icon names in the right order
Created attachment 183277 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service. Features a renewed interface, with each device controllable through a switch, which if toggled off disconnects, and if toggled on connects to the most recently used valid connection. More esoteric features like creation of ad-hoc networks have been moved to the control center panel.
Comment on attachment 183074 [details] [review] popupMenu: don't include hidden items in the width computation Attachment 183074 [details] pushed as e375e17 - popupMenu: don't include hidden items in the width computation
Comment on attachment 183277 [details] [review] Status area: add NetworkManager indicator (In reply to comment #63) > > - Not sure if this is intentional, but it shows 2 lines for each > > modem: the first gives the provider name and signal strength (and > > is insensitive), and the second shows the connection name and > > can be selected > > Yes, this is expected. It matches the behavior of current nm-applet, and makes > sense in case you're not connected, but the modem is active (for example > because you want to make phone calls or receive sms). Except that the signal-strength call fails if you're not active. And for CDMA, the get-name call fails too. In nm-applet, it seems that it doesn't try to make an initial call to get the name/strength, but instead it just waits until it gets a signal, and then creates the signal-strength menuitem at that point. > > - In CDMA (but not GSM), the menu items are slightly confused, and > > the connection labelled "New connection" actually activates the > > first configured CDMA connection... > > Is it possible you actually had a connection named "New connection", somehow > configured by the mobile broadband or NetworkManager itself? The auto item is > named "Create New Connection" for both GSM and CDMA. No it's not: >+ if (is_wwan) { >+ this.category = NMConnectionCategory.WWAN; >+ this._autoConnectionName = _("New Mobile Broadband connection..."); I guess the issue is that this means "connect to default broadband connection", while it looks like it means "open up a dialog to configure a broadband connection". If it's not going to open a dialog, it shouldn't have "..." at the end. New bugs: - The width of the rightmost column of the menu gets messed up when you plug in an ethernet cable. (The switches in the rest of the menu un-right-align.) - Initially I have "Wired /cable unplugged/". If I plug in and then unplug an ethernet cable, I get "Wired /unavailable/". - The menu width possibly increases when you open the More... item. This is probably a bug in my patch... >+ // FIXME: the applet has a mapping of ids to names >+ this.operator_name = 'Mystery CDMA Provider ' + id; We need to do something better there. At a minimum, make it _("Unknown CDMA Provider") or something, but ideally we need a new API to get that info. (You should get dcbw to look through all the FIXMEs and XXXs.) >+ 'network': imports.ui.status.network.NMApplet We should do the conditional approach from comment 62. Most people aren't going to have a usable NM for a little while yet. >+ get state() { >+ return this._switch.state; >+ }, >+ >+ setToggleState: function(newval) { >+ this._switch.setToggleState(newval); >+ } weird non-parallel interface there. Should either be both methods or both property access. >+ log ('Section title activated when there is more than one device, should be non reactive'); extra space there before the '(' >+ } else >+ this.setStatus(null); you use setStatus('') in NMWiredSectionTitleMenuItem but setStatus(null) in NMWirelessSectionTitleMenuItem. >+ log ('setting enabled property, is ' + this._switch.state); leftover debugging? >+ if (activeConnection) { >+ if (activeConnection._connection) { >+ // remove the connection if it was already seen >+ let pos = this._findConnection(activeConnection._connection._uuid); >+ if (pos != -1) { >+ let obj = this._connections[pos]; >+ if (obj.item) >+ obj.item.destroy(); >+ obj.item = null; >+ } can't you just do this.removeConnection(activeConnection._connection, true)? >+ setEnabled: function(enabled) { >+ // do nothing by default, we want to keep the conneciton list visible typo "conneciton" >+ _updateStatusItem: function() { >+ // ignore device state here, when there's only one device >+ // it will be taken care by NMApplet >+ if (this._onlyOfCategory) nothing ever sets this true >+ this._operatorItem = new PopupMenu.PopupImageMenuItem(this.mobileDevice.operator_name || '', we need something better than '' >+function NMDeviceBluetooth() { has anyone tested this yet? >+ // Use undefined instead of null, so we're sure not to match anything >+ // that comes from NetworkManager >+ let activeApSsid = activeAp ? activeAp.get_ssid() : null; that comment does not appear to match the code? >+ for(let j = 0; j < this._networks.length; j++) { a few lines around here are missing spaces before '(' >+ // notify immediately >+ Mainloop.idle_add(Lang.bind(this, function() { >+ this._notifyActiveConnection(a); >+ })); It probably doesn't matter here, but "idle_add" isn't "immediately". Eg, if something is animating, the idle probably won't trigger until after the animation finishes. Use some form of Meta.later_add() if you really need "immediately". >+ case NMConnectionCategory.VPN: >+ // XXX: there is no network-vpn-acquiring >+ this.setIcon('network-vpn'); is there a bug filed against the icon theme? >diff --git a/tools/build/gnome-shell.modules b/tools/build/gnome-shell.modules Remove this for now.
For the CDMA stuff, the applet uses a small C file to parse the mobile broadband providers database: http://git.gnome.org/browse/network-manager-applet/tree/src/utils/nmn-mobile-providers.c http://git.gnome.org/browse/network-manager-applet/tree/src/utils/nmn-mobile-providers.h you won't always get a name out of ModemManager because the modem doesn't always give us one, and it never will for CDMA. So: CDMA: 1) Call org.freedesktop.ModemManager.Modem.Cdma.GetServingSystem() to get the "(usu)" reply of (band class, band, and SID). Ignore the band class and the band, all you care about is the SID. 2) Using the returned SID (if !0) you search through the mobile broadband providers database and find the provider that owns that SID. Owen suggested importing the C files above into the shell for the time being. Once you've got the provider, show the provider's name. GSM: 1) Call org.freedesktop.ModemManager.Modem.Gsm.Network.GetRegistrationInfo() to get the "(uss)" network registration information. The "u" indicates whether the network is home, roaming, forbidden, etc. The first "s" is the MCC/MNC specific to that GSM network, and the second "s" is the provider's name if known. 2) if the provider's name is not returned, using the same process as the CDMA stuff, you look up the MCC/MNC in the mobile broadband provider database and find the provider name from it.
Quick ModemManager introspection XML links: CDMA: http://cgit.freedesktop.org/ModemManager/ModemManager/tree/introspection/org.freedesktop.ModemManager.Modem.Cdma.xml GSM: http://cgit.freedesktop.org/ModemManager/ModemManager/tree/introspection/org.freedesktop.ModemManager.Modem.Gsm.Network.xml Obviously when you see the device initially, you'll want to grab the registration state, and then you should be able to register for state changes after that. The applet has some polling code in it just to be compatible with earlier ModemManager versions that didn't automatically poll internally. You shouldn't need to poll ModemManager; if you do, then that's a bug in MM that I'll fix. MM should handle polling internally and emit state change signals when things like registration, provider, or signal strength change.
+ // FIXME: listen for this property changing and recreate modem stuff Don't worry about this bit for now; neither NM nor ModemManager support the device changing capabilities on-the-fly yet so we'll fix stuff up in the shell indicator when that happens later. + // FIXME: we need to summon the mobile wizard here I suppose we could add some bits to nm-applet's D-Bus interface to start the mobile wizard and complete connection setup. Then at the end we have a choice: we can either have nm-applet call AddConnection and return the new object path to the shell indicator, which the shell can use to activate the connection itself, or we can have nm-applet call AddAndActivateConnection and the shell indicator can just listen for state changes. However, in the future, we want the shell indicator to do the activation itself, but since time is short we can do the activation from nm-applet if you like. + // FIXME: WiMax support (if enabled) WiMAX is pretty easy to support, as it's quite simple, but it's clearly a lower priority than Wired + WiFi + WWAN. + // XXX: is this enough? or do we need other stuff from bluetoothd? I can't quite follow the auto connection path here, but almost all of the time the indicator should be doing the following when the user selects something from the menu: 1) Is there any existing connection for that item? 2) You can use the combination of nm_device_filter_connections(), nm_access_point_filter_connections(), and nm_wimax_nsp_filter_connections() from libnm-glib to get a list of connections applicable to any given NMDevice and AP/NSP. Just call nm_client_activate_connection() on the first one in the list for now. 3) If not, then you can use the nm_client_add_and_activate_connection() function to build up the connection for you automatically in NM and activate it. In general you can set as little information in the connection as you want; even just the name is usually sufficient, because given the NMDevice object path and the AP/NSP object path (as the "specific_object" parameter) NM can figure out what you want. You'll also want to combine compatible APs if you're not doing that already. We want to show *networks* in the menu, not specific wifi access points. In a business you'll typically have 10 or 20 different APs you can see, all with the same security settings and SSID, so that you can easily roam. We want to combine all those into the same "network" that we show in the menu. The combination rules are in the applet's src/utils/utils.c utils_hash_ap() function. An AP gets a hash based on: - SSID - mode (infrastructure or adhoc) - security category (open, WEP, or WPA) nm-applet tags each AP with a hash generated from that data when it gets a "new ap found" signal from the NMDeviceWifi, and uses that hash to combine APs that it shows in the menu into WiFi networks. + // among all visible networks, pick the last recently used connection + ... + // XXX: what else to do? + // for now, just pick a random network + // (this function is called in a corner case anyway, that is, only when + // the user toggles the switch and has more than one wireless device) I'm not quite sure what this all means, but it seems wrong. The applet should only ever call AddAndActivateConnection() or ActivateConnection() in response to a user clicking on a device or an AP/NSP in the menu. Otherwise, let NM pick which network to start. We shouldn't be letting the user click on a wifi *device*, but only on wifi networks, to start a connection. If they have toggled the wifi switch to "on" then NM will notice, and NM will pick which AP to connect to internally. + // XXX: check what nm-applet does here + device._description = device.get_product(); nm-applet uses the utils_get_device_description() function from it's internal utility libs to clean up strings reported by the Linux PCI database. That includes things like removing specific words ("Corporation", "Company", "Co.", "Ltd.", etc), and removing entire phrases ("54 Mbps Wireless PC Card", "PC Card with XJACK(r) Antenna", etc). The strings here come almost entirely from the Linux PCI database (/usr/share/hwdata/pci.ids) via udev, and require some cleanup before they can be displayed in the UI. + if (a._type != NetworkManager.SETTING_VPN_SETTING_NAME) { + // find a good device to be considered primary + // XXX: check what nm-applet does here Not really sure what this code is trying to do, can you describe what's happening a bit more? If I understand correctly, you're just trying to cache the primary device of the active connection? If so the code here is probably fine; NM wont' (yet) ever send more than one device for an ActiveConnection. + // XXX: NetworkManager does not give us signal for bluetooth Nope; it's not really useful to do so, since you have to be within about 2 - 3 meters of the device anyway. Here's what you do for the icon: 1) if it's a PAN/NAP connection, it should show a bluetooth icon, but you wont' get signal strength for the WWAN connection, because NM doesn't know that, because the connection is really just an ethernet connection and the phone handles all the WWAN stuff. 2) if it's a DUN connection, then you want to treat it exactly the same as ModemManager handled WWAN connection, because it really is. So if its DUN do the same thing you do for non-Bluetooth WWAN GSM and CDMA, like asking ModemManager what the signal strength is, what the provider name and registration state is, etc.
(In reply to comment #84) > (From update of attachment 183277 [details] [review]) > (In reply to comment #63) > > > - Not sure if this is intentional, but it shows 2 lines for each > > > modem: the first gives the provider name and signal strength (and > > > is insensitive), and the second shows the connection name and > > > can be selected > > > > Yes, this is expected. It matches the behavior of current nm-applet, and makes > > sense in case you're not connected, but the modem is active (for example > > because you want to make phone calls or receive sms). > > Except that the signal-strength call fails if you're not active. And for CDMA, > the get-name call fails too. In nm-applet, it seems that it doesn't try to make > an initial call to get the name/strength, but instead it just waits until it > gets a signal, and then creates the signal-strength menuitem at that point. Ok, will try to fix. > > > - In CDMA (but not GSM), the menu items are slightly confused, and > > > the connection labelled "New connection" actually activates the > > > first configured CDMA connection... > > > > Is it possible you actually had a connection named "New connection", somehow > > configured by the mobile broadband or NetworkManager itself? The auto item is > > named "Create New Connection" for both GSM and CDMA. > > No it's not: > > >+ if (is_wwan) { > >+ this.category = NMConnectionCategory.WWAN; > >+ this._autoConnectionName = _("New Mobile Broadband connection..."); > > I guess the issue is that this means "connect to default broadband connection", > while it looks like it means "open up a dialog to configure a broadband > connection". If it's not going to open a dialog, it shouldn't have "..." at the > end. That was taken from nm-applet, which has an assistant for configuring mobile connections. We don't support that (we could with some help from nm-applet), so it just creates a mostly empty connection and hopes that NetworkManager can fill the gaps. > New bugs: > - The width of the rightmost column of the menu gets messed up when you plug > in an ethernet cable. (The switches in the rest of the menu un-right-align.) I have no idea where that comes from, but an { x_align: St.Align.END } should fix it. > - Initially I have "Wired /cable unplugged/". If I plug in and then unplug > an ethernet cable, I get "Wired /unavailable/". You're device suddenly lost the Carrier capabilitity? Definitely not expected, but I see no other code that could do that (unless it somehow messes with device-to-section assignments) > > >+ // FIXME: the applet has a mapping of ids to names > >+ this.operator_name = 'Mystery CDMA Provider ' + id; > > We need to do something better there. At a minimum, make it _("Unknown CDMA > Provider") or something, but ideally we need a new API to get that info. > > (You should get dcbw to look through all the FIXMEs and XXXs.) Will do. > > >+ 'network': imports.ui.status.network.NMApplet > > We should do the conditional approach from comment 62. Most people aren't going > to have a usable NM for a little while yet. Ok. > >+ get state() { > >+ return this._switch.state; > >+ }, > >+ > >+ setToggleState: function(newval) { > >+ this._switch.setToggleState(newval); > >+ } > > weird non-parallel interface there. Should either be both methods or both > property access. It is the same interface as used by PopupSwitchMenuItem and Switch itself. state is a getter because it is readonly, and setters with side effects are discouraged by style guide. > >+ log ('Section title activated when there is more than one device, should be non reactive'); > > extra space there before the '(' > > >+ } else > >+ this.setStatus(null); > > you use setStatus('') in NMWiredSectionTitleMenuItem but setStatus(null) in > NMWirelessSectionTitleMenuItem. setStatus('') means 'set a status that is the empty string', while setStatus(null) means 'don't show a status, show a switch instead'. NMWiredSectionTitleMenuItem should show nothing when you have more than one device (as the global switch has no effect), while NMWirelessSectionTitleMenuItem should keep showing the switch to toggle airplane mode. > >+ log ('setting enabled property, is ' + this._switch.state); > > leftover debugging? Oops. Yes. > >+ if (activeConnection) { > >+ if (activeConnection._connection) { > >+ // remove the connection if it was already seen > >+ let pos = this._findConnection(activeConnection._connection._uuid); > >+ if (pos != -1) { > >+ let obj = this._connections[pos]; > >+ if (obj.item) > >+ obj.item.destroy(); > >+ obj.item = null; > >+ } > > can't you just do this.removeConnection(activeConnection._connection, true)? removeConnection removes the connection from _connections, which I don't want to do, I just want to remove the menu item. (removing the connection altogheter would require even more special code in NMWirelessDevice, which expects to have a list of all valid connections at all times) > >+ setEnabled: function(enabled) { > >+ // do nothing by default, we want to keep the conneciton list visible > > typo "conneciton" > > >+ _updateStatusItem: function() { > >+ // ignore device state here, when there's only one device > >+ // it will be taken care by NMApplet > >+ if (this._onlyOfCategory) > > nothing ever sets this true Worse, nothing ever calls (or should call) that method. Leftover from a previous iteration that never made into something working. > >+ this._operatorItem = new PopupMenu.PopupImageMenuItem(this.mobileDevice.operator_name || '', > > we need something better than '' Or we need to hide the operatorItem until we have something sensible. > >+function NMDeviceBluetooth() { > > has anyone tested this yet? Not yet. > >+ // Use undefined instead of null, so we're sure not to match anything > >+ // that comes from NetworkManager > >+ let activeApSsid = activeAp ? activeAp.get_ssid() : null; > > that comment does not appear to match the code? Uhm... > >+ for(let j = 0; j < this._networks.length; j++) { > > a few lines around here are missing spaces before '(' > > >+ // notify immediately > >+ Mainloop.idle_add(Lang.bind(this, function() { > >+ this._notifyActiveConnection(a); > >+ })); > > It probably doesn't matter here, but "idle_add" isn't "immediately". Eg, if > something is animating, the idle probably won't trigger until after the > animation finishes. Use some form of Meta.later_add() if you really need > "immediately". The code is right, I don't want to starve animations or anything like that, just eventually show a notification (generally, this code path is hit at the start of gnome-shell, when it notices connections that are already active). > > >+ case NMConnectionCategory.VPN: > >+ // XXX: there is no network-vpn-acquiring > >+ this.setIcon('network-vpn'); > > is there a bug filed against the icon theme? Not yet. > >diff --git a/tools/build/gnome-shell.modules b/tools/build/gnome-shell.modules > > Remove this for now. Ok.
(In reply to comment #87) > + // FIXME: listen for this property changing and recreate modem stuff > > Don't worry about this bit for now; neither NM nor ModemManager support the > device changing capabilities on-the-fly yet so we'll fix stuff up in the shell > indicator when that happens later. Ok. > + // FIXME: we need to summon the mobile wizard here > > I suppose we could add some bits to nm-applet's D-Bus interface to start the > mobile wizard and complete connection setup. Then at the end we have a choice: > we can either have nm-applet call AddConnection and return the new object path > to the shell indicator, which the shell can use to activate the connection > itself, or we can have nm-applet call AddAndActivateConnection and the shell > indicator can just listen for state changes. However, in the future, we want > the shell indicator to do the activation itself, but since time is short we can > do the activation from nm-applet if you like. As long as I have something to call into network-manager-applet, it can return nothing, a connection path or event a combination of username, password, additional details needed, and can get away with it. > + // FIXME: WiMax support (if enabled) > > WiMAX is pretty easy to support, as it's quite simple, but it's clearly a lower > priority than Wired + WiFi + WWAN. I'm not sure it is quite simple. It is conceptually the same as wifi, and is likely to require to same amount of code to track separately connections and access points (nsp). > + // XXX: is this enough? or do we need other stuff from bluetoothd? > > I can't quite follow the auto connection path here, but almost all of the time > the indicator should be doing the following when the user selects something > from the menu: > > 1) Is there any existing connection for that item? > 2) You can use the combination of nm_device_filter_connections(), > nm_access_point_filter_connections(), and nm_wimax_nsp_filter_connections() > from libnm-glib to get a list of connections applicable to any given NMDevice > and AP/NSP. Just call nm_client_activate_connection() on the first one in the > list for now. > 3) If not, then you can use the nm_client_add_and_activate_connection() > function to build up the connection for you automatically in NM and activate > it. In general you can set as little information in the connection as you > want; even just the name is usually sufficient, because given the NMDevice > object path and the AP/NSP object path (as the "specific_object" parameter) NM > can figure out what you want. Each NMDevice will invoke _createAutomaticConnection to build an appropriate NMConnection (that until add_and_activate needed to have everything sorted, now it is shallower, but still has some code for each device type). I'm glad I don't need to worry and that NetworkManager is clever enough. > > You'll also want to combine compatible APs if you're not doing that already. > We want to show *networks* in the menu, not specific wifi access points. In a > business you'll typically have 10 or 20 different APs you can see, all with the > same security settings and SSID, so that you can easily roam. We want to > combine all those into the same "network" that we show in the menu. The > combination rules are in the applet's src/utils/utils.c utils_hash_ap() > function. An AP gets a hash based on: > > - SSID > - mode (infrastructure or adhoc) > - security category (open, WEP, or WPA) > > nm-applet tags each AP with a hash generated from that data when it gets a "new > ap found" signal from the NMDeviceWifi, and uses that hash to combine APs that > it shows in the menu into WiFi networks. It is already doing that, but based only on SSID. I'll add mode and security to the hashing function. > + // among all visible networks, pick the last recently used connection > + ... > + // XXX: what else to do? > + // for now, just pick a random network > + // (this function is called in a corner case anyway, that is, only when > + // the user toggles the switch and has more than one wireless device) > > I'm not quite sure what this all means, but it seems wrong. The applet should > only ever call AddAndActivateConnection() or ActivateConnection() in response > to a user clicking on a device or an AP/NSP in the menu. Otherwise, let NM > pick which network to start. We shouldn't be letting the user click on a wifi > *device*, but only on wifi networks, to start a connection. If they have > toggled the wifi switch to "on" then NM will notice, and NM will pick which AP > to connect to internally. After Matthias's comments, now there is a switch for each device, including wired and vpn (which is not really a device but it is implemented as one), plus the global wireless and wwan ones. Toggling a switch off is obvious, just nm_device_deactivate, while toggling on is not. Pending some more API in NetworkManager, it just connects to the most recently used connection, or creates a new one if none exists. (As the comment says, that function is only called in a corner case, that is only when the device specific toggle is shown. Normally is it replaced with the global one. The problem still exists for wired, though). > + // XXX: check what nm-applet does here > + device._description = device.get_product(); > > nm-applet uses the utils_get_device_description() function from it's internal > utility libs to clean up strings reported by the Linux PCI database. That > includes things like removing specific words ("Corporation", "Company", "Co.", > "Ltd.", etc), and removing entire phrases ("54 Mbps Wireless PC Card", "PC Card > with XJACK(r) Antenna", etc). The strings here come almost entirely from the > Linux PCI database (/usr/share/hwdata/pci.ids) via udev, and require some > cleanup before they can be displayed in the UI. Ok. Will port that to JS. > + if (a._type != NetworkManager.SETTING_VPN_SETTING_NAME) { > + // find a good device to be considered primary > + // XXX: check what nm-applet does here > > Not really sure what this code is trying to do, can you describe what's > happening a bit more? If I understand correctly, you're just trying to cache > the primary device of the active connection? If so the code here is probably > fine; NM wont' (yet) ever send more than one device for an ActiveConnection. ActiveConnection::Devices is of type ao, so I assumed it returned more than device in some cases, and that I needed to filter out secondary devices to attach the connection. If nothing ever creates active connections with more than one device, then no problem! > > + // XXX: NetworkManager does not give us signal for bluetooth > > Nope; it's not really useful to do so, since you have to be within about 2 - 3 > meters of the device anyway. Here's what you do for the icon: > > 1) if it's a PAN/NAP connection, it should show a bluetooth icon, but you wont' > get signal strength for the WWAN connection, because NM doesn't know that, > because the connection is really just an ethernet connection and the phone > handles all the WWAN stuff. We don't have a bluetooth icon (besides the normal bluetooth one, but it would look weird if you had two bluetooth icons, connected to two different menus), and there is no generic network-wwan-connected either. Should I fallback to network-wired (or network-transmit-receive) here? > 2) if it's a DUN connection, then you want to treat it exactly the same as > ModemManager handled WWAN connection, because it really is. So if its DUN do > the same thing you do for non-Bluetooth WWAN GSM and CDMA, like asking > ModemManager what the signal strength is, what the provider name and > registration state is, etc. Ok
(In reply to comment #87) > 2) if it's a DUN connection, then you want to treat it exactly the same as > ModemManager handled WWAN connection, because it really is. So if its DUN do > the same thing you do for non-Bluetooth WWAN GSM and CDMA, like asking > ModemManager what the signal strength is, what the provider name and > registration state is, etc. In this case, how do I know if it is GSM, CDMA or LTE, to find the right ModemManager interface? Shall I Introspect the object?
Created attachment 183388 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service. Features a renewed interface, with each device controllable through a switch, which if toggled off disconnects, and if toggled on connects to the most recently used valid connection. More esoteric features like creation of ad-hoc networks have been moved to the control center panel.
Comment on attachment 183388 [details] [review] Status area: add NetworkManager indicator "git bz apply" complained about trailing whitespace. You can use "git diff --check" to see where. Random thing I've thought of far too late: we're (well) beyond string freeze, so we can't add any new translatable strings. Urk. The fix here is probably to make sure we use only strings that NM is using, and define _() in network.js to pull from NM's translation domain rather than our own. (And make sure to not add network.js to POTFILES.in.) >+ for (let iter in table) { >+ let providers = table[value]; s/value/iter/. >+ /* Search through each country's providers */ >+ for (let i = 0; i < providers.length; i++) { Looks like you copy+pasted the C code and then translated to JS? Parts of that code ended up with tabs instead of spaces, and you should also convert the comments to // style as well. (Likewise with the other newly-added code.) >+ _findOperatorName: function(name, opCode) { >+ if (name.length != 0 && (name.length > 6 || name.length < 5)) { >+ // this looks like a valid name, i.e. not an MCCMNC (that some >+ // devices return when not yet connected >+ return name; >+ } Is this logic copied from nm-applet? It seems like there would be false negatives. "Sprint", "Orange", etc. >+function fixupPCIDescription(desc) { >+ desc.replace('[_,]', ' '); That will replace the literal string "[_,]" with spaces. You need //, not ''. >+ if (_IGNORED_WORDS.indexOf(item) == -1) { >+ out += ' ' + item; that will result in out always starting with a space. One fix would be to make out an array, use "out.push(item);", and then "return out.join(' ');" >+try { >+ STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['network'] = imports.ui.status.network.NMApplet; >+} catch(e) { >+ STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['network'] = undefined; That shouldn't be necessary; the try section will throw while evaluating the right-hand side of the assignment, so nothing will have gotten assigned yet. >+// small optimization, to avoid using [] all the time >+const NM80211Mode = NetworkManager['80211Mode']; >+const NM80211ApFlags = NetworkManager['80211ApFlags']; need NM80211ApSecurityFlags too >+function getDeviceDescription(device) { make this an NMDevice method? >+function getApSecurityType(accessPoint) { can you make a (JS) enum for the return values here? >+ this._statusBin = new St.Bin({ x_align: St.Align.END }); I'd thought this was all that was needed too, but it doesn't fix the bug. (The menu should be shrinking, but it's not... might be boxpointer's fault.) >+ this._onlyOfCategory = false; You removed _updateStatusItem(), but NMDevice._init still initializes the unused property. >+ obj = { ssid: ssid, ssid: ap.get_ssid() (in NMDeviceWireless._init and _accessPointAdded). >+ if (any && this._shouldShowConnectionList() && !skipCreateMenu) { skipCreateMenu doesn't exist any more >+#ifndef MOBILE_BROADBAND_PROVIDER_INFO >+#define MOBILE_BROADBAND_PROVIDER_INFO DATADIR "/mobile-broadband-provider-info/serviceproviders.xml" >+#endif >+ >+#define ISO_3166_COUNTRY_CODES DATADIR "/zoneinfo/iso3166.tab" I think you should assume they're in /usr, not in our prefix. >+ /* Hack for rh#556292; iso3166.tab is just wrong */ The real fix for this is that NM (and gnome-shell) should be using iso-codes's iso_3166.xml, which (a) has the right string there, (b) has a .pc file so you can find the files correctly, (c) has its own translations so we don't need to duplicate them. >+ pieces[1] = g_strdup (_("United Kingdom")); >+ } >+ >+ g_hash_table_insert (table, pieces[0], pieces[1]); You're translating in the UK case but not the normal case. (assuming most of the rest of the copy+pasted code is correct...) >diff --git a/tools/build/gnome-shell.modules b/tools/build/gnome-shell.modules still need to remove this part
Every time I disable Wired, I get another "System eth0" entry added under it. (And similarly with the Mobile Broadband section.) It looks like you changed how CDMA connection names work, but I still get "New Mobile Broadband Connection...". >+ if (!name2 && needlemnc.substring(0, 2) == mcc.mnc.substring(0, 2)) mccmnc.mnc.substring... >+ if (cmda_sid[j] == sid) cdma_sid >+ this._proxy.connect('SignalQuality', Lang.bind(this, function(quality) { function(proxy, quality) GSM seems to forget the provider name while it's connecting... (it goes blank). And then after connecting it no longer shows the signal strength.
Dan, we got blanket exception for landing the network ui and strings.
(In reply to comment #92) > (From update of attachment 183388 [details] [review]) > >+#ifndef MOBILE_BROADBAND_PROVIDER_INFO > >+#define MOBILE_BROADBAND_PROVIDER_INFO DATADIR "/mobile-broadband-provider-info/serviceproviders.xml" > >+#endif > >+ > >+#define ISO_3166_COUNTRY_CODES DATADIR "/zoneinfo/iso3166.tab" > > I think you should assume they're in /usr, not in our prefix. > > >+ /* Hack for rh#556292; iso3166.tab is just wrong */ > > The real fix for this is that NM (and gnome-shell) should be using iso-codes's > iso_3166.xml, which (a) has the right string there, (b) has a .pc file so you > can find the files correctly, (c) has its own translations so we don't need to > duplicate them. > > >+ pieces[1] = g_strdup (_("United Kingdom")); > >+ } > >+ > >+ g_hash_table_insert (table, pieces[0], pieces[1]); > > You're translating in the UK case but not the normal case. > > (assuming most of the rest of the copy+pasted code is correct...) I'd prefer to avoid modifying this code, in this cycle. We can rethink about this later, when the shell will become independent of nm-applet. Or if you want file a bug directly at NetworkManager.
(In reply to comment #95) > I'd prefer to avoid modifying this code, in this cycle. We can rethink about > this later, when the shell will become independent of nm-applet. > Or if you want file a bug directly at NetworkManager. You need to at least fix the DATADIR thing. No one is going to have the provider database installed in their gnome-shell jhbuild prefix.
Latest patches from here against gnome-shell master blow up: ** Message: applet now removed from the notification area JS LOG: GNOME Shell started at Tue Mar 15 2011 11:56:19 GMT-0500 (CDT) (nm-applet:3119): libnotify-WARNING **: Failed to connect to proxy ** Message: applet now embedded in the notification area JS ERROR: !!! Exception was: ReferenceError: ssid is not defined JS ERROR: !!! lineNumber = '944' JS ERROR: !!! fileName = '/opt/gnome-shell/install/share/gnome-shell/js/ui/status/network.js' JS ERROR: !!! stack = '([object _private_NMClient_Client],[object _private_NMClient_DeviceWifi],[object Array])@/opt/gnome-shell/install/share/gnome-shell/js/ui/status/network.js:944 NMDeviceWireless([object _private_NMClient_Client],[object _private_NMClient_DeviceWifi],[object Array])@/opt/gnome-shell/install/share/gnome-shell/js/ui/status/network.js:915 ([object _private_NMClient_Client],[object _private_NMClient_DeviceWifi])@/opt/gnome-shell/install/share/gnome-shell/js/ui/status/network.js:1589 ()@/opt/gnome-shell/install/share/gnome-shell/js/ui/status/network.js:1576 ([object _private_NMClient_RemoteSettings])@/opt/gnome-shell/install/share/gnome-shell/js/ui/status/network.js:1509 ([object _private_NMClient_RemoteSettings])@/opt/gnome-shell/install/share/gjs-1.0/lang.js:110 ' JS ERROR: !!! message = 'ssid is not defined' nm-tool shows that NetworkManager is working fine: [jclinton@jclinton-laptop ~]$ nm-tool NetworkManager Tool State: connected (global) - Device: eth0 [System eth0] -------------------------------------------------- Type: Wired Driver: e1000e State: connected Default: yes HW Address: 00:1C:25:A0:81:4F Capabilities: Carrier Detect: yes Speed: 100 Mb/s Wired Properties Carrier: on IPv4 Settings: Address: 172.16.5.104 Prefix: 16 (255.255.0.0) Gateway: 172.16.0.1 DNS: 172.16.0.1 - Device: wlan0 ---------------------------------------------------------------- Type: 802.11 WiFi Driver: iwlagn State: disconnected Default: no HW Address: 00:16:EA:69:A9:0A Capabilities: Wireless Properties WEP Encryption: yes WPA Encryption: yes WPA2 Encryption: yes Wireless Access Points Popeyes-4G: Infra, 00:30:44:07:92:BA, Freq 2427 MHz, Rate 54 Mb/s, Strength 1 WPA WPA2 Land3: Infra, 00:1C:10:F4:EF:E1, Freq 2437 MHz, Rate 54 Mb/s, Strength 1 WPA hpsetup: Ad-Hoc, 2E:25:B3:BC:A6:FA, Freq 2437 MHz, Rate 11 Mb/s, Strength 1 advclust-5g: Infra, C0:3F:0E:7C:57:69, Freq 5765 MHz, Rate 54 Mb/s, Strength 2 WPA WPA2 Enterprise advclust-pub: Infra, 00:1C:F0:57:E5:FF, Freq 2462 MHz, Rate 54 Mb/s, Strength 2 WPA WPA2 advclust-2g: Infra, C0:3F:0E:7C:57:67, Freq 2412 MHz, Rate 54 Mb/s, Strength 2 WPA WPA2 Enterprise
Created attachment 183457 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service. Features a renewed interface, with each device controllable through a switch, which if toggled off disconnects, and if toggled on connects to the most recently used valid connection. More esoteric features like creation of ad-hoc networks have been moved to the control center panel.
Created attachment 183458 [details] [review] Status area: add NetworkManager indicator Adds an implementation of nm-applet in javascript. Uses the new introspection from NetworkManager, and temporarily requires nm-applet to be running for the secret service. Features a renewed interface, with each device controllable through a switch, which if toggled off disconnects, and if toggled on connects to the most recently used valid connection. More esoteric features like creation of ad-hoc networks have been moved to the control center panel.
(In reply to comment #96) > (In reply to comment #95) > > I'd prefer to avoid modifying this code, in this cycle. We can rethink about > > this later, when the shell will become independent of nm-applet. > > Or if you want file a bug directly at NetworkManager. > > You need to at least fix the DATADIR thing. No one is going to have the > provider database installed in their gnome-shell jhbuild prefix. No one is going to have NetworkManager in jhbuild either, so the applet will be skipped entirely. If one wants to test, it can add a symlink (like I did). I don't want to break deliberate installation in a different prefix.
(In reply to comment #100) > (In reply to comment #96) > > (In reply to comment #95) > > > I'd prefer to avoid modifying this code, in this cycle. We can rethink about > > > this later, when the shell will become independent of nm-applet. > > > Or if you want file a bug directly at NetworkManager. > > > > You need to at least fix the DATADIR thing. No one is going to have the > > provider database installed in their gnome-shell jhbuild prefix. > > No one is going to have NetworkManager in jhbuild either, so the applet will be > skipped entirely. If one wants to test, it can add a symlink (like I did). I > don't want to break deliberate installation in a different prefix. I'd expect system nm 0.9 + jhbuild gnome-shell to be very common way of working on gnome-shell from this point version. (But then again, if that works a bit badly with mobile broadband, certainly not a blocker for getting this stuff committed as long as it doesn't entirely blow up.)
Created attachment 183537 [details] [review] gnome-shell-jhbuild: Adjust GI_TYPELIB_PATH for NM inclusion When jhbuilding, we use a jhbuilt gobject-introspection, so the default typelib path is the jhbuild prefix, not /usr. So if we are using NetworkManager from packages, we need to adjust GI_TYPELIB_PATH to include it.
Comment on attachment 183458 [details] [review] Status area: add NetworkManager indicator still some tabs: perl -pi -e 's/\t/ /;' js/misc/modemManager.js js/misc/util.js js/ui/status/network.js will fix it (that's 8 spaces). And then commit it with that. Further fixes can happen as new bugs. Also, I just attached a patch to fix up GI_TYPELIB_PATH; I'd been setting it by hand when I was testing.
Review of attachment 183537 [details] [review]: Looks good.
Comment on attachment 183458 [details] [review] Status area: add NetworkManager indicator Attachment 183458 [details] pushed as c8ac3fd - Status area: add NetworkManager indicator Since the gettext domain was not changed, I also added everything to POTFILES.in
Attachment 183537 [details] pushed as cab9a58 - gnome-shell-jhbuild: Adjust GI_TYPELIB_PATH for NM inclusion