GNOME Bugzilla – Bug 646141
network: ad-hoc indicator not shown
Last modified: 2011-04-11 14:46:21 UTC
I don't know if this is intentional or not, but we're never showing ad-hoc icons in the shell network menu (the CC panel shows it).
No, it's not intentional. It's a side effect of building the menu on access points (which don't exist in the ad-hoc case). It should be easy to fix, it just needs to check for connection mode in addConnection and setActiveConnection, and then make the appropriate menu items in _createSection.
Created attachment 184792 [details] [review] NetworkMenu: add adhoc connections to the list Currently, only connections which are associated to an access point are added to the list (so only connections in infrastructure mode). Add another pass in _createSection that collects adhoc connections and shows them (as well as code in add/removeConnection that doesn't try to pair the connection)
Comment on attachment 184792 [details] [review] NetworkMenu: add adhoc connections to the list Mostly seems reasonable. Have you been able to test it? >+ // FIXME: network panel has network-workgroup for adhoc, but the symbolic version is missing filed bug 646359 about that; I don't think we want to show ad hoc networks with the regular icon. >- let any = false, forceupdate = false; >+ let any = false; forceupdate = false; mistake? >+ // only pair infrstructure connections with access points typo > _createAutomaticConnection: function(apObj) { I don't think we ever want an ad-hoc network to be an automatic connection >+ // HACK: obj.adhoc is not properly set for the initial connection list hm... why?
(In reply to comment #3) > (From update of attachment 184792 [details] [review]) > Mostly seems reasonable. Have you been able to test it? > > >+ // FIXME: network panel has network-workgroup for adhoc, but the symbolic version is missing > > filed bug 646359 about that; I don't think we want to show ad hoc networks with > the regular icon. We're already doing that, in _updateIcon. Given we now have a specific icon, I'll update that part as well. > >- let any = false, forceupdate = false; > >+ let any = false; forceupdate = false; > > mistake? Yep. > > _createAutomaticConnection: function(apObj) { > > I don't think we ever want an ad-hoc network to be an automatic connection Ok. Nothing was calling that anyway... > >+ // HACK: obj.adhoc is not properly set for the initial connection list > > hm... why? Because _connections is set by NMDevice.prototype._init without going through NMDeviceWireless.prototype.addConnection
Created attachment 184805 [details] [review] NetworkMenu: add adhoc connections to the list Currently, only connections which are associated to an access point are added to the list (so only connections in infrastructure mode). Add another pass in _createSection that collects adhoc connections and shows them (as well as code in add/removeConnection that doesn't try to pair the connection)
Comment on attachment 184805 [details] [review] NetworkMenu: add adhoc connections to the list (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 184792 [details] [review] [details]) > > Mostly seems reasonable. Have you been able to test it? You didn't answer that part. Has this code been tested against actual ad hoc networks yet, or is it just "based on what the applet does"? > > filed bug 646359 about that; I don't think we want to show ad hoc networks with > > the regular icon. > > We're already doing that, in _updateIcon. Given we now have a specific icon, > I'll update that part as well. You forgot that part. > > >+ // HACK: obj.adhoc is not properly set for the initial connection list > > > > hm... why? > > Because _connections is set by NMDevice.prototype._init without going through > NMDeviceWireless.prototype.addConnection Ah. I think it would be better to have NMDeviceWireless._init do something like: this._connections.forEach(Lang.bind(this, function (obj) { obj.adhoc = this._connectionIsAdHoc(obj.connection); })); so you know the properties are all set correctly after that point.
(In reply to comment #6) > (From update of attachment 184805 [details] [review]) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 184792 [details] [review] [details] [details]) > > > Mostly seems reasonable. Have you been able to test it? > > You didn't answer that part. Has this code been tested against actual ad hoc > networks yet, or is it just "based on what the applet does"? No, not yet. I'm going to test it soon, just a bit overloaded now... > > > filed bug 646359 about that; I don't think we want to show ad hoc networks with > > > the regular icon. > > > > We're already doing that, in _updateIcon. Given we now have a specific icon, > > I'll update that part as well. > > You forgot that part. Fixed. > > > >+ // HACK: obj.adhoc is not properly set for the initial connection list > > > > > > hm... why? > > > > Because _connections is set by NMDevice.prototype._init without going through > > NMDeviceWireless.prototype.addConnection > > Ah. I think it would be better to have NMDeviceWireless._init do something > like: > > this._connections.forEach(Lang.bind(this, function (obj) { obj.adhoc = > this._connectionIsAdHoc(obj.connection); })); > > so you know the properties are all set correctly after that point. It won't work: this._connections is created by NMDevice.prototype._init, which calls _createSection before returning, so _createSection will run at least once without having .adhoc set.
Created attachment 184951 [details] [review] NetworkMenu: add adhoc connections to the list Currently, only connections which are associated to an access point are added to the list (so only connections in infrastructure mode). Add another pass in _createSection that collects adhoc connections and shows them (as well as code in add/removeConnection that doesn't try to pair the connection)
Comment on attachment 184951 [details] [review] NetworkMenu: add adhoc connections to the list >+ // adhoc connections are never added to the more... submenu Hm... why is that? It looks weird. There is something quite wrong with this patch... when I turn on connection sharing on my mac, I see it appear as a regular (infrastructure) network in the network menu in the shell (nmcli sees it correctly as adhoc), but then at the bottom of the menu, I also get an adhoc item, with the name "Auto muelli", which IIRC was an adhoc network someone set up in the hotel at GUADEC last year, but which is certainly not currently visible from my house. Maybe a [j] that should have been an [i] or something?
Comment on attachment 184951 [details] [review] NetworkMenu: add adhoc connections to the list (In reply to comment #1) > No, it's not intentional. It's a side effect of building the menu on access > points (which don't exist in the ad-hoc case). This appears to be untrue, hence the fact that the ad-hoc network shows up in the normal network list. And the fact that the old GUADEC network shows up at all is because the new code is adding all configured ad hoc connections, regardless of whether or not they are associated with visible APs. I think all we need to do is just change _getIcon() to return a different icon in the ad-hoc case, and we're done. (I can't connect to the ad-hoc network I've set up here, but I can't connect from nm-applet either, so it's either an NM bug, or a misconfiguration on the mac side.)
Created attachment 185479 [details] [review] network: show ad-hoc icon for ad-hoc networks
Review of attachment 185479 [details] [review]: Apparently, my understanding of NetworkManager was wrong, and ad-hoc connections are associated with an AP. Besides, the idea to show all ad-hoc connections was to become able to create them like nm-applet does, but I think that delegating this functionality to the network panel makes sense. Good to commit with maintainer approval. ::: js/ui/status/network.js @@ +142,2 @@ _getIcon: function() { + if (this.bestAP.mode == NetworkManager['80211Mode'].ADHOC) You can use NM80211Mode here.
I forgot to add: I tested this with unpatched 3.0.0 at the italian Release Party: the ad-hoc access point was correctly reported (with the zero-strength icon from signalToIcon fallback) and the connection was created (it is still visible in nm-connection-editor) Connecting didn't work, but that's a different story...
Attachment 185479 [details] pushed as 8232684 - network: show ad-hoc icon for ad-hoc networks