can we get NMRemoteSettings:connections too? (Tracking available connections is even harder than tracking devices or APs, since NMRemoteSettings only emits "added" signals and you have to watch each individual connection for "removed"...)

Behold!  It be done.  I haven't tested any of this FYI, but it's so simple as to be almost correct from the start :)

Wouldn't it be more helpful for libnm-glib at least to just emit a ConnectionRemoved signal from Settings too?  We can certainly have a couple signals for "removed" from a few different places for bindings convenience.

(In reply to comment #3)
> Wouldn't it be more helpful for libnm-glib at least to just emit a
> ConnectionRemoved signal from Settings too?

Yes.

I had assumed that you were going to add the corresponding properties to the libnm-glib side too... it's not just "other" bindings that these properties would be helpful for. And if you do NMDeviceWimax:nsps too, then we can get rid of the "pseudo property" stuff in NMObject, which would be great because NMObject is way too complicated.

But anyway, the branch looks good as far as it goes.

Updated with the ConnectionRemoved signal.

So how would we go about updating libnm-glib?  The problem these new properties always had was that they now require comparing the old and new, since they contain the change as a whole.  With the Added/Removed stuff, we don't need to do any of that, we can simply operate on one object at a time.  In some ways that's a lot easier to do than the whole compare thing...

We also still need to emit the Added/Removed gobject signals, so we'd need some hook when doing the array compare for the various objects to handle those events like they do now (mainly to emit signals).

So maybe object_property_complete() gets changed to do the diff in the odata->array case?

Naive implementation of that diff (completely untested) now in nm-object.c, and ported over NMDeviceWifi.  We end up saving code here, but only because we can remove some stuff that no longer makes send (enforcing 0-length AP list when card goes disabled, which NM should be enforcing anyway) and code cleanup due to that.  I guess the big code save will be when we can remove the pseudo-properties.

Any suggestions on optimizing the array diff in nm-object.c appreciated.

(In reply to comment #6)
> Naive implementation of that diff (completely untested) now in nm-object.c, and
> ported over NMDeviceWifi.  We end up saving code here, but only because we can
> remove some stuff that no longer makes send (enforcing 0-length AP list when
> card goes disabled, which NM should be enforcing anyway) and code cleanup due
> to that.  I guess the big code save will be when we can remove the
> pseudo-properties.
> 
> Any suggestions on optimizing the array diff in nm-object.c appreciated.

Hm... I'd been thinking the diff wouldn't be necessary; we could just replace the array-valued property wholesale, and the added/removed signals would just proxy the existing D-Bus signals. But I realize that won't work because then the signals would get emitted before NMObject had finished resolving the object paths... So that makes the whole thing less slick. Maybe just commit the earlier stuff for now and we can think about this again later. (If we got rid of the added and removed signals when we do the ABI break, and only had notify::access-points, then the need for diffing would go away, and I don't think clients would be much worse for it...)

Done, dropped the libnm-glib patch and rebased to master.

Pushed 3 !fixups,



Concerning "wifi: add AccessPoints property to NMDeviceWifi":

It's a quite unexpected, that PROP_ACCESS_POINTS returns something
different then impl_device_get_access_points().

Three options:

  1.) PROP_ACCESS_POINTS and impl_device_get_access_points() return something
      different.

  2.) change impl_device_get_access_points() to return all APs

  3.) PROP_ACCESS_POINTS only shall return only the APs with SSID, it would
      have to connect to the notify::ssid signals of each AP to see, when a AP
      becomes visible (which is relatively cumbersome).

looks good to me, modulo Thomas's point about PROP_ACCESS_POINTS

Yeah, it's a tough call.  NM has filtered out hidden SSIDs since 2007 apparently.  I had completely forgotten that NM did that.  gnome-shell, network-manager-applet, KDE, and gnome-control-center handle a missing SSID correctly.  But nmcli doesn't, but I've fixed that in the top commit.

I would prefer that NetworkManager returned all access points from GetAccessPoints(), even hidden ones as Thomas suggests in his option #2.

What do you guys think?  The downside of doing #2 is that we may break clients that don't expect an SSID, especially those that use libnm-glib.  I think that's probably worth the risk, but we should do some release notes or something for this new behavior.

We could deprecate GetAccessPoints() and add a new GetAllAccessPoints() and have all three entry points (those two plus the AccessPoints property) document the difference, both in the introspection files and the libnm-glib docs.

Re-pushed now; git.fdo was down last night again...

Yeah, I'll make that change.

(In reply to comment #13)
> Re-pushed now; git.fdo was down last night again...
> 
> Yeah, I'll make that change.

So... were you going to make that change and push again?

Re-worked and re-pushed.  This time, I resurrected the object array property diff-ing code, and just got rid of the psuedo-property stuff.  I think the reduction in complexity is worth it.

(In reply to comment #15)

> libnm-glib: convert NMDeviceWimax NSPs to a real property
> libnm-glib: convert NMDeviceWifi AccessPoints to a real property

why do you remove the #defines DBUS_PROP_*? Doesn't that possibly break compilation for clients?


> libnm-glib: add testing framework and testcases

Pushed a !fixup on top of your branch




Nothing else, looks good.

(In reply to comment #16)
> (In reply to comment #15)
> 
> > libnm-glib: convert NMDeviceWimax NSPs to a real property
> > libnm-glib: convert NMDeviceWifi AccessPoints to a real property
> 
> why do you remove the #defines DBUS_PROP_*? Doesn't that possibly break
> compilation for clients?

They are all in the .c files so they aren't exposed to clients.  libnm-glib used to use those defines, but Dan Winship made some changes last year that make them unused in the code.  I guess we just forgot to remove them.

> > libnm-glib: add testing framework and testcases
> 
> Pushed a !fixup on top of your branch

Thanks, that fixup looks good, I'll take it.

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > 
> > > libnm-glib: convert NMDeviceWimax NSPs to a real property
> > > libnm-glib: convert NMDeviceWifi AccessPoints to a real property
> > 
> > why do you remove the #defines DBUS_PROP_*? Doesn't that possibly break
> > compilation for clients?
> 
> They are all in the .c files so they aren't exposed to clients.  libnm-glib
> used to use those defines, but Dan Winship made some changes last year that
> make them unused in the code.  I guess we just forgot to remove them.

Of course, I missed that :$

Maybe the DBUS_PROP removal stuff should happen as a separate commit?



> api/wifi: add GetAllAccessPoints() method

>+        retreive a list of all access points (including hidden ones) use the

"retrieve"



> libnm-glib: add support for non-pseudo-property added/removed signals

Maybe it would be cleaner to have separate add and removed callbacks?

Or... you could just pass the string "access-point"/"nsp"/"device", and NMObject could could concatentate that to "-added" and "-removed" and then directly g_signal_emit_by_name() rather than calling a callback, and the stuff that's currently in the callback functions could just go into the class's signal handler implementations (eg, wifi_class->access_point_added). Since the signals are defined as RUN_FIRST, that code would run before any g_signal_connect()ed signal handlers, so the semantics should be the same as the current code.



> libnm-glib: convert NMDeviceWimax NSPs to a real property

>+#define NM_DEVICE_WIMAX_NETWORK_SERVICE_PROVIDERS "network-service-providers"

I don't think we use the un-acronymized name anywhere else in the API.

Repushed.

Moved DBUS_PROP stuff to a separate commit and killed all of them.

Fixed "retrieve".

Changed as suggested to having NMObject emit the added/removed signal.

However, I don't think we can do the last one, because the NMObject property names need to correspond to the NetworkManager D-Bus interface names, and if we switch to "nsps" that won't be the case anymore.  We could add property name aliases though if you like?  That would be more consistent with "nsp-added" and "nsp-removed" and get_nsps().

I added shadow/override property names, since it wasn't that hard to do, and I'd rather keep the libnm-glib API consistent.

Changed commits:
libnm-glib: add support for non-pseudo-property added/removed signals
libnm-glib: convert NMDeviceWifi AccessPoints to a real property
libnm-glib: convert NMDeviceWimax NSPs to a real property
libnm-glib: convert NMClient Devices to a real property
libnm-glib: add testing framework and testcases
trivial: remove unused #defines

(In reply to comment #20)
> However, I don't think we can do the last one, because the NMObject property
> names need to correspond to the NetworkManager D-Bus interface names, and if we
> switch to "nsps" that won't be the case anymore.

Well, I mean, you can change the D-Bus property to "Nsps" too. (I guess I should have pointed out the naming problem in the commit where you added the D-Bus property, not in the commit where you updated libnm-glib to use it.)

Otherwise good. The shadow/override stuff is indeed simple, but I don't think you need it.

(In reply to comment #22)
> (In reply to comment #20)
> > However, I don't think we can do the last one, because the NMObject property
> > names need to correspond to the NetworkManager D-Bus interface names, and if we
> > switch to "nsps" that won't be the case anymore.
> 
> Well, I mean, you can change the D-Bus property to "Nsps" too. (I guess I
> should have pointed out the naming problem in the commit where you added the
> D-Bus property, not in the commit where you updated libnm-glib to use it.)
> 
> Otherwise good. The shadow/override stuff is indeed simple, but I don't think
> you need it.

Oh hah, I forgot I'd added that property in the first place, and I thought it was already established API.  You are 100% correct.

Fixed and re-pushed.

yay

Merged to master.