GNOME Bugzilla – Bug 706262
Two fixes for the network section
Last modified: 2013-08-31 16:54:27 UTC
With these and bug 706259 (which the cause of the dreaded GLib.Error at startup no one could figure out), I get back a fully operational network menu section again. Also, the empty space in the top bar is again filled with icons.
Created attachment 252147 [details] [review] network: fix iterating connections in NMConnectionSection Must compare the length of the array, not the array itself.
Created attachment 252148 [details] [review] network: don't crash because a device doesn't have a description yet Descriptions are only added after all devices are read (thanks to the disambiguation in libnm-gtk), but we use them immediately when we call _sync() in various points (such as checkConnection())
Review of attachment 252147 [details] [review]: OK.
Review of attachment 252148 [details] [review]: Hm, so we call checkConnection immediately before all devices are read, meaning that we're somehow iterating through all the devices asking them if they want to take a connection, before we know all the devices? That seems broken.
(In reply to comment #4) > Review of attachment 252148 [details] [review]: > > Hm, so we call checkConnection immediately before all devices are read, meaning > that we're somehow iterating through all the devices asking them if they want > to take a connection, before we know all the devices? That seems broken. We've been doing that since 3.0, never saw a problem in that path...
Comment on attachment 252147 [details] [review] network: fix iterating connections in NMConnectionSection Attachment 252147 [details] pushed as 1b03e55 - network: fix iterating connections in NMConnectionSection
Created attachment 252293 [details] [review] Don't ignore VPN connections over ethernet We should not ignore vpn active connections just because we don't show the underlying device in the UI.
Created attachment 252294 [details] [review] NMConnectionItem: fix typo
Created attachment 252295 [details] [review] network: fix double VPN indicator in the panel If we elect VPN as our main connection (either because it took over the default route, or because the other connections are filtered because unknown), we shouldn't show also the other vpn indicator.
Review of attachment 252294 [details] [review]: OK.
Created attachment 252296 [details] [review] network: don't return null from NMConnectionDevice._getStatus() StLabel complains set you set the text to NULL
Review of attachment 252296 [details] [review]: OK.
Created attachment 252299 [details] [review] network: use the VPN state to compute the icon for VPN items We watch changes in the VPN state, not the active connection state, so if we use the active connection state, we might miss an update (because the VPN property is notified before the other one)
Review of attachment 252299 [details] [review]: OK.
Created attachment 252302 [details] [review] network: fix signal name There is no state-changed signal on NMActiveConnection
Created attachment 252303 [details] [review] NMConnectionDevice: don't rely on syncActiveConnection running before notify::active-connection We can't use the _connection property added to the active connection by syncActiveConnection, because that code might not have run yet, depending on the order of the signals emitted by NM.
Created attachment 252304 [details] [review] NMConnectionDevice: don't unconditionally access device.active_connection When called after _syncActiveConnections(), we might not yet have an active connection object.
Created attachment 252305 [details] [review] network: allow disconnecting while activation is in progress While connecting, the item should read "Turn Off", not "Connect". To do so, change the meaning of isActive() to be really "not isOff()"
Review of attachment 252304 [details] [review]: This doesn't make sense. It's guaranteed that if an ActiveConnection has a device listed in get_devices(), the backpointer is also around. This is a NM bug.
Review of attachment 252305 [details] [review]: ::: js/ui/status/network.js @@ +105,3 @@ return false; + return this._activeConnection.state <= NetworkManager.ActiveConnectionState.ACTIVATED; Do the states guarantee any ordering like this?
Review of attachment 252302 [details] [review]: OK, so state-changed is on the device? And NMVPNConnection has vpn-state-changed and notify::vpn-state? What a mess...
(In reply to comment #20) > Review of attachment 252304 [details] [review]: > > This doesn't make sense. It's guaranteed that if an ActiveConnection has a > device listed in get_devices(), the backpointer is also around. > > This is a NM bug. Guaranteed by what? First you get PropertiesChanged on the /o/fd/NetworkManager, then you get it on /o/fd/NM/Devices/X. Signal handling is obviously asynchronous, so there is a window in which the two might be out of sync.
(In reply to comment #21) > Review of attachment 252305 [details] [review]: > > ::: js/ui/status/network.js > @@ +105,3 @@ > return false; > > + return this._activeConnection.state <= > NetworkManager.ActiveConnectionState.ACTIVATED; > > Do the states guarantee any ordering like this? The values in the enumerations are part of the ABI, they won't change without notice.
Review of attachment 252293 [details] [review]: Patch looks good, and it fixes the new system status menu not showing my vpn status as connected.
Review of attachment 252295 [details] [review]: Patch looks good, I've added it to my local gnome-shell build and it causes no regressions.
Review of attachment 252303 [details] [review]: Note: I started looking into the patches attached to bug 706262 because after upgrading to Fedora 20 (pre-alpha), I've had various issues with networking in the new system status menu. Since I was spending time on them anyways I thought I might as well review them, but I must say js is not one of my strongest languages. With that said, lets move on to the actual review. This patch looks wrong to me, it changes the constructor for the abstract NMConnectionDevice class to take 3 parameters. But it does not change any of the implementations of NMConnectionDevice to actually pass a 3th parameter when calling their parent constructor.
All three of those fixes will be obsolete soon.
Attachment 252148 [details] pushed as f7284ca - network: don't crash because a device doesn't have a description yet Attachment 252294 [details] pushed as 0c12c07 - NMConnectionItem: fix typo Attachment 252296 [details] pushed as 5a9f0c2 - network: don't return null from NMConnectionDevice._getStatus() Attachment 252299 [details] pushed as fc5aadd - network: use the VPN state to compute the icon for VPN items Attachment 252302 [details] pushed as 22b2ccd - network: fix signal name Attachment 252305 [details] pushed as 6fbe765 - network: allow disconnecting while activation is in progress The other patches are now obsolete.