GNOME Bugzilla – Bug 732253
vapi: add vala bindings for new_async methods
Last modified: 2014-07-16 22:14:58 UTC
Created attachment 279274 [details] [review] patch v1 Currently, the vala bindings for libnm-glib/NMClient-1.0 skip the new_async methods in the RemoteSettings and Client classes because the cause errors. This patch add proper bindings for those methods.
Looks OK to me, any chance you could provide some example code so I could better test the changes? Thanks!
Created attachment 279445 [details] Demo usage of patch Sure. Here is a trivial demo. Compiles with: valac --pkg libnm-glib --pkg libnm-util --pkg gio-2.0 --pkg dbus-glib-1 --vapidir </path/to/libnm-*.vapi> AsyncInitDemo.vala </path/to/libnm-*.vapi> is the directory where libnm-util.vapi and libnm-glib.vapi are.
why do the existing methods cause errors, and is there some way we could annotate them so they didn't? otherwise I guess this is fine...
> why do the existing methods cause errors In the existing bindings, NM.Client.new_async is skipped, so it is not available at all. If you try to use the generic GLib.AsyncInitable.new_async (typeof(NM.Client)) and then pass the GLib.AsyncResult to NM.Client.finish (), then NM.Client.finish () throws and error because the GLib.AsyncResult did not come from NM.Client.new_async. In fact, the NM.Client.finish () method should not be publicly exposed in vala as such. It is called automatically by the vala async infrastructure when you call NM.Client.new_async.end (). > and is there some way we could annotate them so they didn't? I am new to vala, so there could be something that I am missing, but I was not able to find a cleaner way of doing the bindings. The only "prior art" I could find was the bindings for GIO. All of the async constructors there are implemented in a -custom.vala file [1] as I have done in my patch. Since the GIO bindings come from the vala project itself, I am fairly confident that there is not a better way to do this at this time. [1]: https://github.com/GNOME/vala/blob/master/vapi/metadata/Gio-2.0-custom.vala
So I wonder why NM.Client.new_async() got skipped; is it not possible to be bound? (I'm just curious, I probably know less about vala than anyone else on this bug...)
It's possible to bind it with just metadata by doing Client.new_finish symbol_type="function" And removing the existing Client.new_async skip line.
I am using vala 0.20 and if I remove the skip, I get the following errors: > NMClient-1.0.gir:940.7-940.64: error: `NM.Client' already contains a definition for `.new' > NMClient-1.0.gir:910.7-910.40: note: previous definition of `.new' was here > NMClient-1.0.gir:7361.7-7361.82: error: `NM.RemoteSettings' already contains a definition for `new_async' > NMClient-1.0-custom.vala:7.9-7.55: note: previous definition of `new_async' was here Should it work with a later version of vala/vapigen?
Created attachment 280764 [details] [review] vala: expose Client.new_async as an async function Don't *just* remove the skip. You need to replace it with the line from my comment. AFAIK it should work with any version of vala that supports setting the symbol type in metadata, so >= 0.18, though I've only tested with git master and 0.24.0. Patch attached.
Yes, I understood and that is what I did that produced the error.
0.20 works for me with my patch. Also 0.22, FWIW.
Maybe you're doing Client.new_async symbol_type="function" Instead of Client.new_finish symbol_type="function" ?
No, quite sure I have done exactly as you have suggested. Could you post your generated vapi so I can have a look?
Created attachment 280867 [details] vapi generated by attachment #280764 [details] Lines 1 and 176 are the interesting ones.
Created attachment 280885 [details] [review] update Evan's patch to include RemoteSettings.new_async/finish Thanks, Evan, for sticking with me while I was being dumb. Finally got it working. I must have had a typo or it didn't like having a split line like I had in my original patch or something like that. Uploading a new patch since Evan's only fixed one instance of new_asyc/finish (Client.new_async) and there are two (also RemoteSettings.new_async).
Pushed patch from comment 14 to git master and nm-0-9-10. Thanks David and Evan!