After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 732253 - vapi: add vala bindings for new_async methods
vapi: add vala bindings for new_async methods
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: API
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-06-25 23:14 UTC by David Lechner
Modified: 2014-07-16 22:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch v1 (2.24 KB, patch)
2014-06-25 23:14 UTC, David Lechner
none Details | Review
Demo usage of patch (443 bytes, text/plain)
2014-06-27 21:09 UTC, David Lechner
  Details
vala: expose Client.new_async as an async function (920 bytes, patch)
2014-07-15 23:03 UTC, Evan Nemerson
none Details | Review
vapi generated by attachment #280764 (56.42 KB, text/plain)
2014-07-16 15:36 UTC, Evan Nemerson
  Details
update Evan's patch to include RemoteSettings.new_async/finish (917 bytes, patch)
2014-07-16 19:16 UTC, David Lechner
none Details | Review

Description David Lechner 2014-06-25 23:14:32 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.
Comment 1 Dan Williams 2014-06-27 20:42:57 UTC
Looks OK to me, any chance you could provide some example code so I could better test the changes?  Thanks!
Comment 2 David Lechner 2014-06-27 21:09:51 UTC
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.
Comment 3 Dan Winship 2014-07-11 16:52:13 UTC
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...
Comment 4 David Lechner 2014-07-11 17:19:33 UTC
> 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
Comment 5 Dan Williams 2014-07-15 16:34:09 UTC
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...)
Comment 6 Evan Nemerson 2014-07-15 21:00:18 UTC
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.
Comment 7 David Lechner 2014-07-15 22:35:37 UTC
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?
Comment 8 Evan Nemerson 2014-07-15 23:03:09 UTC
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.
Comment 9 David Lechner 2014-07-15 23:10:56 UTC
Yes, I understood and that is what I did that produced the error.
Comment 10 Evan Nemerson 2014-07-16 01:34:04 UTC
0.20 works for me with my patch.  Also 0.22, FWIW.
Comment 11 Evan Nemerson 2014-07-16 03:39:58 UTC
Maybe you're doing

  Client.new_async symbol_type="function"

Instead of

  Client.new_finish symbol_type="function"

?
Comment 12 David Lechner 2014-07-16 12:47:58 UTC
No, quite sure I have done exactly as you have suggested. Could you post your generated vapi so I can have a look?
Comment 13 Evan Nemerson 2014-07-16 15:36:37 UTC
Created attachment 280867 [details]
vapi generated by attachment #280764 [details]

Lines 1 and 176 are the interesting ones.
Comment 14 David Lechner 2014-07-16 19:16:54 UTC
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).
Comment 15 Dan Williams 2014-07-16 22:14:58 UTC
Pushed patch from comment 14 to git master and nm-0-9-10.

Thanks David and Evan!