GNOME Bugzilla – Bug 698918
A large assortment of fixes for the networking code
Last modified: 2013-06-06 21:17:47 UTC
While working on the new aggregate menu and the subsequent modal dialog, I accumulated a large amount of fixes that are (should be!) independent from the redesign. I can't imagine there being any controversial changes in here, but note that I'm not up to speed with libnm or the network status applet code yet, so some of these might be wrong. Comments appreciated!
Created attachment 242505 [details] [review] network: Require NMGtk Enough time has passed that we can safely depend on NMGtk in the new network implementation.
Created attachment 242506 [details] [review] network: Remove some unused helpers
Created attachment 242507 [details] [review] network: Properly disconnect from the state-changed signal The destroy signal never gets emitted, so we need to properly disconnect manually when we destroy the wrapper.
Created attachment 242508 [details] [review] network: Remove UNKNOWN security type It's unused.
Created attachment 242509 [details] [review] network: Remove some dead code We cannot possibly reach createActiveConnectionItem unless we have an active network, so the plain unreative menu item cannot ever be created.
Created attachment 242510 [details] [review] network: Deduplicate some similar code
Created attachment 242511 [details] [review] network: Fix some splice mishaps Calling splice() without a second argument removes all of the elements after the provided index, not just one.
Created attachment 242512 [details] [review] network: Remove some genericism in the wireless code This was presumably originally used in multiple places, but it's not anymore, so un-generify the code here.
Created attachment 242513 [details] [review] network: Don't create submenus for multiple-connection items As multiple-connections for a Wi-Fi AP won't fit in the new design, remove submenus right now. Simply make a simple item that connects to the first known connection for the AP, which should be the common case.
Created attachment 242514 [details] [review] network: Remove explicit autoconnection code NM is now a lot smarter about dealing with automatic connections, so just create an empty connection and pass it to it. The only places where NM requires connection settings is where we require explicit setup: Bluetooth DUN, WPA-Enterprise and WWAN/VPN. These cases are already handled by gnome-control-center, where complex configuration is handled, so remove the automatic connection management for now and just let NM handle it.
Created attachment 242515 [details] [review] network: Remove some more dead code The title param isn't used anymore.
Created attachment 242516 [details] [review] network: Remove _createAPItem The code flows better if this is inlined like this.
Created attachment 242517 [details] [review] network: Use activeApChanged to get the initial active network
Created attachment 242518 [details] [review] network: Rename "apObj" to "network" We put these "access point objects" in "this._networks" and "this._activeNetwork", so let's rename it. This also makes the fact that each "access point object" can contain multiple access points a tiny bit less confusing.
Review of attachment 242505 [details] [review]: I agree, NMGtk is a fine requirement.
Review of attachment 242506 [details] [review]: Yes.
Review of attachment 242507 [details] [review]: Ok
Review of attachment 242508 [details] [review]: Yes
Review of attachment 242509 [details] [review]: This is not dead code: activeNetwork/active_access_point can sometimes be null even if we have an active connection, because of bugs in the underlying device drivers, in NetworkManager, or because of an ad-hoc connection.
Review of attachment 242510 [details] [review]: Yes
Review of attachment 242511 [details] [review]: Ops, yes
Review of attachment 242512 [details] [review]: It was used for wwan originally, but it would be used for wimax too, if the patch was ever completed.
Review of attachment 242513 [details] [review]: Having one connection is a common case, connecting to the first one when you have two is not. Please don't remove this code, unless you need to.
Review of attachment 242509 [details] [review]: No, I mean, look at what calls createActiveConnectionItem. :)
Review of attachment 242514 [details] [review]: NM is smart, but not this smart. AFAIK, it needs a connection type, name and uuid. ::: js/ui/status/network.js @@ -375,2 @@ _activateAutomaticConnection: function() { - let connection = this._createAutomaticConnection(); This hook is used to spawn gnome-control-center in the wwan case, you can't just remove it.
(In reply to comment #25) > Review of attachment 242514 [details] [review]: > > NM is smart, but not this smart. AFAIK, it needs a connection type, name and > uuid. Not according to dcbw. Apparently this is a recent addition.
Review of attachment 242515 [details] [review]: This becomes wrong if the _activeNetwork change is blocked.
Review of attachment 242516 [details] [review]: If we remove submenus, you can probably unify the two 'activate' paths. If we don't, this is a no.
Review of attachment 242517 [details] [review]: Ok
Review of attachment 242518 [details] [review]: I used to be consistent on apObj, but I agree that network reads better.
Review of attachment 242514 [details] [review]: After IRC discussion, my review was wrong, and this patch is fine.
(In reply to comment #24) > Review of attachment 242509 [details] [review]: > > No, I mean, look at what calls createActiveConnectionItem. :) Oh, this is so nice... Let's say ok, it won't be more broken than now.
Attachment 242505 [details] pushed as 871c28a - network: Require NMGtk Attachment 242506 [details] pushed as e1de363 - network: Remove some unused helpers Attachment 242507 [details] pushed as dbb39d3 - network: Properly disconnect from the state-changed signal Attachment 242508 [details] pushed as e0b8ad7 - network: Remove UNKNOWN security type Attachment 242509 [details] pushed as a67b82e - network: Remove some dead code Attachment 242510 [details] pushed as 37dce7d - network: Deduplicate some similar code Attachment 242511 [details] pushed as 20619ad - network: Fix some splice mishaps Attachment 242512 [details] pushed as 7a8b392 - network: Remove some genericism in the wireless code Attachment 242514 [details] pushed as b4b00a4 - network: Remove explicit autoconnection code Attachment 242517 [details] pushed as e5f2266 - network: Use activeApChanged to get the initial active network Attachment 242518 [details] pushed as bfdf069 - network: Rename "apObj" to "network" Pushing these for now. I'll leave the other patches in my local branch and figure out what to do with them later, and maybe reattach updated versions when I land the modal dialog.
Attachment 242513 [details] pushed as a55288b - network: Don't create submenus for multiple-connection items Attachment 242515 [details] pushed as 2249da7 - network: Remove some more dead code Attachment 242516 [details] pushed as 9c222c7 - network: Remove _createAPItem On IRC we decided that pushing these three were the best way to move forward. It's unlikely that people are using multi-connection on purpose.