GNOME Bugzilla – Bug 646380
On/Off switch for "VPN Connections" makes no sense
Last modified: 2011-04-08 16:40:23 UTC
Created attachment 184821 [details] Screenshot of the switch The On/Off switch on the "VPN Connections" just confuses the heck out of me: - It's "On" even if you aren't connected to any VPN - Turning it off and then on causes a connection to an arbitrary VPN - Turning it off when you're connected doesn't seem to disconnect from a VPN I'm not sure if this switch is intentional, or just an artifact of code to add it for other sections.
(In reply to comment #0) > Created an attachment (id=184821) [details] > Screenshot of the switch > > The On/Off switch on the "VPN Connections" just confuses the heck out of me: > > - It's "On" even if you aren't connected to any VPN > - Turning it off and then on causes a connection to an arbitrary VPN > - Turning it off when you're connected doesn't seem to disconnect from a VPN > > I'm not sure if this switch is intentional, or just an artifact of code to add > it for other sections. It is both. It is there because code is shared, and it is there because otherwise you cannot disconnect from VPN. The behavior on activate (that is, from off to on) is the same as other devices: just connect to last used connection (with higher timestamp reported by NM).
Created attachment 184823 [details] [review] NetworkMenu: fix VPN connection state It was always reporting true, even if disconnected. At the same time, add a signal that is emitted when state changes and update the UI accordingly. In the future (with another libnm-glib API break) we should use the NMVPNConnection object to track the connection state, so that we can show if we're connecting or we need authentication.
Comment on attachment 184823 [details] [review] NetworkMenu: fix VPN connection state > this._device.activate(); > else > this._device.deactivate(); >+ >+ // Immediately reset the switch to false, it will be updated appropriately >+ // by state-changed signals in devices (but fixes the VPN not being in sync >+ // if the ActiveConnection object is never seen by libnm-glib) >+ this._switch.setToggleState(false); Hm... would it be better to set it false *before* calling this._device.activate(), in case it ends up emitting state-changed before returning from activate()? But it all seems to work fine for me (both ethernet and VPN). Is there some reason we don't get a "connecting..." status on the VPN item? It seems weird that it continues to show as "Off" while we're connecting.
(In reply to comment #3) > But it all seems to work fine for me (both ethernet and VPN). Is there some > reason we don't get a "connecting..." status on the VPN item? It seems weird > that it continues to show as "Off" while we're connecting. Giovanni said above: In the future (with another libnm-glib API break) we should use the NMVPNConnection object to track the connection state, so that we can show if we're connecting or we need authentication.
Review of attachment 184823 [details] [review]: In testing, this really makes a big difference - I'm not sure this is fully what we'd want for design long-term but it feels much more functional - it's confusing to have the state where the VPN is off, but it says ON, and the fact that it's off is indicated by the absence of a dot by the VPN connection. There is a slight inconsistency: - For Wireless, if no Wireless connection is active, the switch is on (because wireless is enabled) - For VPN, if no VPN connection is active, the switch is off But I'm not sure anybody would actually notice, and if we made the Wireless switch behave in the same fashion as VPN then you'd have no way to turn it off and keep it from autoconnecting. Code changes look good to me - in terms of coding patterns, I think Dan is right that it's better to set false first to be agnostic about whether activate/deactivate is sync or async, but it's not going to make a difference here in practice - activate/deactivate get sent pretty much immmediately to D-Bus, and D-Bus outgoing calls don't re-enter.
Created attachment 184954 [details] [review] NetworkMenu: fix VPN connection state It was always reporting true, even if disconnected. At the same time, add a signal that is emitted when state changes and update the UI accordingly. In the future (with another libnm-glib API break) we should use the NMVPNConnection object to track the connection state, so that we can show if we're connecting or we need authentication.
Attachment 184954 [details] pushed as d1a110d - NetworkMenu: fix VPN connection state
Created attachment 184965 [details] [review] network: fix logic bug in checking whether to activate or deactive Guess Dan and I should have kept our mouth shut about theoretical ordering considerations :-) ... as committed clicking on the switch when off didn't actually activate the VPN. === A cosmetic change recommended in review of the patch to fix the VPN Connections switch ended up introducing a logic error that made the switch not work properly. Fix.
(In reply to comment #8) > Created an attachment (id=184965) [details] [review] > network: fix logic bug in checking whether to activate or deactive > > Guess Dan and I should have kept our mouth shut about theoretical > ordering considerations :-) ... as committed clicking on the switch > when off didn't actually activate the VPN. > > === > A cosmetic change recommended in review of the patch to fix the > VPN Connections switch ended up introducing a logic error that > made the switch not work properly. Fix. Ugh... And I should have checked it before pushing... Anyway good to commit, with RT approval (they'll be very happy of our careful and attentive reviews...)
Pushed (didn't ask for r-t approval as it's just a correction to a previouly approved change) Attachment 184965 [details] pushed as 6c3300b - network: fix logic bug in checking whether to activate or deactive
*** Bug 647194 has been marked as a duplicate of this bug. ***