Created attachment 242585 [details] [review]
network: use custom checkConnection() for VPN connections

Review of attachment 242585 [details] [review]:

The issue is, we need a custom removeConnection too, so we can clear the signal on the .active object when it's dropped.
At which point, there is not much code sharing happening, so we can get rid of NMConnectionBased, fold it into NMDevice, and have the custom thing for VPN.

Review of attachment 242585 [details] [review]:

::: js/ui/status/network.js
@@ +1561,3 @@
+        let valid = this.connectionValid(connection);
+
+        if (exists && valid) {

connectionValid always returns true, so we'll never reach removeConnection anyway.

Review of attachment 242585 [details] [review]:

::: js/ui/status/network.js
@@ +1564,3 @@
+            // Update active connection
+            let obj = this._connections[pos];
+            obj.active = connection;

I think this is also wrong as well; connection here is a NMConnection, not an NMActiveConnection, which is what obj.active wants.

I would like to see that fixed for 3.8. And this isn't gonna fly if I start refactoring code. So do we agree on a custom removeConnection() for VPN + fixes following Jasper's review?

Since 3.10 is going to be a rewrite of this code because the design changes.

Created attachment 242763 [details] [review]
network: use custom checkConnection()/removeConnection() for VPN connections

Ping?

Just upgraded to 3.8.3, this problem is still present...

Just ran into this too; note that 3.10 shell has rewritten checkConnection and it no longer looks at the timestamp at all.  I'm not sure why it *ever* looked at the timestamp, the UUID is the only thing that should be used to uniquely identify a connection, and this is what 3.10 uses.

So I don't think the patches need to copy checkConnection; the right fix for 3.8 would be simply to compare the UUID in checkConnection instead of the (name+timestamp).

*** Bug 698963 has been marked as a duplicate of this bug. ***

Basically, does this patch fix things for anyone?

    checkConnection: function(connection) {
        let pos = this._findConnection(connection.get_uuid());
        let exists = pos != -1;
        let valid = this.connectionValid(connection);
        let same = false;
        if (exists) {
            let existing = this._connections[pos];

-            // Check if connection changed name or id
-            similar = existing.name == connection.get_id();
-                existing.timestamp == connection._timestamp;
+            same = existing.uuid == connection.get_uuid();
        }

    checkConnection: function(connection) {
        let pos = this._findConnection(connection.get_uuid());
        let exists = pos != -1;
        let valid = this.connectionValid(connection);
        let similar = false;
        if (exists) {
            let existing = this._connections[pos];

-            // Check if connection changed name or id
-            similar = existing.name == connection.get_id();
-                existing.timestamp == connection._timestamp;
+            similar = existing.uuid == connection.get_uuid();
        }

Fixed the patch...

Hello,
the patch in comment #13 seems to work for me. After applying it the switch seems to behave as expected.
--
Alessio Gaeta

Any news on this?

GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.