Created attachment 223354 [details] [review]
NetworkMenu: use async initialization for libnm-glib objects

NMClient recently got more heavyweight, with a property holding supported
connections. As fully initializing a NMObject is a recursive operation
and requires multiple DBus calls, switch to async initalization for NMClient
and NMRemoteSettings.

(In reply to comment #1)
> Created an attachment (id=223354) [details] [review]
> NetworkMenu: use async initialization for libnm-glib objects
> 
> NMClient recently got more heavyweight, with a property holding supported
> connections.

Does this mean that we can drop a lot of random code?

Review of attachment 223354 [details] [review]:

::: js/ui/status/network.js
@@ +1850,3 @@
             let connectionPath = activeConnections[i].connection;
             let connection = this._settings.get_connection_by_path(connectionPath)
+            if (!connection || this._ignoreConnection(connection))

This seems wrong to me.

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=223354) [details] [review] [details] [review]
> > NetworkMenu: use async initialization for libnm-glib objects
> > 
> > NMClient recently got more heavyweight, with a property holding supported
> > connections.
> 
> Does this mean that we can drop a lot of random code?

Yes. Except that to do so, we need to introduce more random code to sync this array property, so it's not a big gain, really. I may do it at some time in the future, maybe for 3.6.1, maybe for 3.7.1.

Created attachment 226565 [details] [review]
NetworkMenu: use async initialization for libnm-glib objects

NMClient recently got more heavyweight, with a property holding supported
connections. As fully initializing a NMObject is a recursive operation
and requires multiple DBus calls, switch to async initalization for NMClient
and NMRemoteSettings.

That bit was caused by NM bug 686226, I removed it now that I found the right fix.

So, while is not complete as we wish, is there any chance to merge it?

Dan, would you be the right person to review this ?

Comment on attachment 226565 [details] [review]
NetworkMenu: use async initialization for libnm-glib objects

>NMClient recently got more heavyweight, with a property holding supported
>connections.

NMClient has a property holding active connections, but not "supported" connections... you might be mixing up NMClient and NMRemoteSettings?

And none of these properties are new; the change is that since 0.9.6, NMClient does a blocking GetAll() at construct time to initialize all properties, rather than doing a separate blocking Get() for each individual property the first time you access it.


Anyway, the actual code looks right.

I was referring to AvailableConnections on NMDevice, which was new when I wrote the patch, and was the first time the NMClient hierarchy loaded connections.

(In reply to comment #9)
> I was referring to AvailableConnections on NMDevice, which was new when I wrote
> the patch

Huh, never saw that... but anyway, that was added to the D-Bus API, but not to libnm-glib, so NMDevice will just ignore it. (It will receive the property value in the original GetAll(), but it won't try to recursively resolve its contents.)

Created attachment 236293 [details] [review]
NetworkMenu: use async initialization for libnm-glib objects

NMClient recently got more heavyweight, with a property holding supported
connections. As fully initializing a NMObject is a recursive operation
and requires multiple DBus calls, switch to async initalization for NMClient
and NMRemoteSettings.

Rebased, per Bastien's request in bug 687853.
Btw, Dan, you said the code looks right, and since that you fixed NM
to available-connections exists now. Can I land this one?

Comment on attachment 236293 [details] [review]
NetworkMenu: use async initialization for libnm-glib objects

yeah, still looks right to me

Ok, let's get this in now.
Attachment 236293 [details] pushed as 382cc52 - NetworkMenu: use async initialization for libnm-glib objects