After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 698918 - A large assortment of fixes for the networking code
A large assortment of fixes for the networking code
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-04-26 06:03 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-06-06 21:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
network: Require NMGtk (3.89 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove some unused helpers (1.02 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Properly disconnect from the state-changed signal (1.55 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove UNKNOWN security type (1.18 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove some dead code (1.55 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Deduplicate some similar code (2.06 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Fix some splice mishaps (1.41 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove some genericism in the wireless code (3.63 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Don't create submenus for multiple-connection items (2.80 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove explicit autoconnection code (9.24 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove some more dead code (1.68 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Remove _createAPItem (2.72 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Use activeApChanged to get the initial active network (1.32 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
network: Rename "apObj" to "network" (12.90 KB, patch)
2013-04-26 06:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:07 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!
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:10 UTC
Created attachment 242505 [details] [review]
network: Require NMGtk

Enough time has passed that we can safely depend on NMGtk in the new
network implementation.
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:13 UTC
Created attachment 242506 [details] [review]
network: Remove some unused helpers
Comment 3 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:17 UTC
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.
Comment 4 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:20 UTC
Created attachment 242508 [details] [review]
network: Remove UNKNOWN security type

It's unused.
Comment 5 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:24 UTC
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.
Comment 6 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:27 UTC
Created attachment 242510 [details] [review]
network: Deduplicate some similar code
Comment 7 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:30 UTC
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.
Comment 8 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:35 UTC
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.
Comment 9 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:38 UTC
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.
Comment 10 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:42 UTC
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.
Comment 11 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:45 UTC
Created attachment 242515 [details] [review]
network: Remove some more dead code

The title param isn't used anymore.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:49 UTC
Created attachment 242516 [details] [review]
network: Remove _createAPItem

The code flows better if this is inlined like this.
Comment 13 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:52 UTC
Created attachment 242517 [details] [review]
network: Use activeApChanged to get the initial active network
Comment 14 Jasper St. Pierre (not reading bugmail) 2013-04-26 06:03:56 UTC
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.
Comment 15 Giovanni Campagna 2013-04-26 15:25:43 UTC
Review of attachment 242505 [details] [review]:

I agree, NMGtk is a fine requirement.
Comment 16 Giovanni Campagna 2013-04-26 15:26:00 UTC
Review of attachment 242506 [details] [review]:

Yes.
Comment 17 Giovanni Campagna 2013-04-26 15:27:16 UTC
Review of attachment 242507 [details] [review]:

Ok
Comment 18 Giovanni Campagna 2013-04-26 15:27:39 UTC
Review of attachment 242508 [details] [review]:

Yes
Comment 19 Giovanni Campagna 2013-04-26 15:29:40 UTC
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.
Comment 20 Giovanni Campagna 2013-04-26 15:30:40 UTC
Review of attachment 242510 [details] [review]:

Yes
Comment 21 Giovanni Campagna 2013-04-26 15:30:57 UTC
Review of attachment 242511 [details] [review]:

Ops, yes
Comment 22 Giovanni Campagna 2013-04-26 15:31:38 UTC
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.
Comment 23 Giovanni Campagna 2013-04-26 15:33:30 UTC
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.
Comment 24 Jasper St. Pierre (not reading bugmail) 2013-04-26 15:34:23 UTC
Review of attachment 242509 [details] [review]:

No, I mean, look at what calls createActiveConnectionItem. :)
Comment 25 Giovanni Campagna 2013-04-26 15:35:31 UTC
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.
Comment 26 Jasper St. Pierre (not reading bugmail) 2013-04-26 15:36:06 UTC
(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.
Comment 27 Giovanni Campagna 2013-04-26 15:36:11 UTC
Review of attachment 242515 [details] [review]:

This becomes wrong if the _activeNetwork change is blocked.
Comment 28 Giovanni Campagna 2013-04-26 15:38:05 UTC
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.
Comment 29 Giovanni Campagna 2013-04-26 15:39:08 UTC
Review of attachment 242517 [details] [review]:

Ok
Comment 30 Giovanni Campagna 2013-04-26 15:39:57 UTC
Review of attachment 242518 [details] [review]:

I used to be consistent on apObj, but I agree that network reads better.
Comment 31 Giovanni Campagna 2013-04-26 15:47:29 UTC
Review of attachment 242514 [details] [review]:

After IRC discussion, my review was wrong, and this patch is fine.
Comment 32 Giovanni Campagna 2013-04-26 16:10:19 UTC
(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.
Comment 33 Jasper St. Pierre (not reading bugmail) 2013-04-26 16:25:01 UTC
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.
Comment 34 Jasper St. Pierre (not reading bugmail) 2013-06-06 21:17:36 UTC
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.