contrib/vapi: Fix vala bindings to correct ownership and API breaks

[ricotz]

No description provided.

[ximion]

Ah, so that's how you adjust the Vala metadata to incorporate preprocessor variables...
Were the changes in f3fdac7 not correct? I am pretty confident that the previous behavior was wrong, so wouldn't changing it back in the metadata result in a memory leak or crashes in Vala applications?

[ricotz]

No, your changes were wrong from the vala point-of-view. You literally reverted accepted and discussed introspection changes. (Vala relies on the internal memory-management of GPtrArray and the current state can lead to trouble when adding or removing elements from those lists)

[ricotz]

https://gitlab.gnome.org/GNOME/gobject-introspection/issues/305

[ximion]

This is really confusing...
On that discussion, you say:

Inserting an element depends on the (transfer) annotation, I guess that it is what we are doing in Vala:
On (transfer container) we don't have to increase the reference before adding and there is no free_function
On (transfer full) we have to increase the reference before adding and there is a free_function

Which sort of makes sense, but in the same way AppStream owns the contents of that array and will free them, you technically only need to free the container, not the individual elements. And according to the documentation, "container" should be used in that case as transfer annotation.

Is there a consensus on how GPtrArray etc. should be treated,m and if so can this be documented somewhere? (maybe on https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations/ ?).

In any case, I would really like to fix the introspection annotations in the code directly instead of working around this issue in the VAPI metadata (i.e. by reverting the patch that introduced the issue). I have no idea how other languages handle this case, but there must be some "right" way that's good for most of them. GPtrArrays aren't that rare afterall...

[ricotz]

Which sort of makes sense, but in the same way AppStream owns the contents of that array and will free them, you technically only need to free the container, not the individual elements. And according to the documentation, "container" should be used in that case as transfer annotation.

You are filling up a new GPtrArray (with set free_function) with newly created AsCompont objects. So the returned container with its elements is completely owned by the receiver.

In any case, I would really like to fix the introspection annotations in the code directly instead of working around this issue in the VAPI metadata (i.e. by reverting the patch that introduced the issue).

Yes, reverting your change would be proper solution.

Note that with your change the gobject annotations even mismatch now with AsCache.

If other languages are ignoring the fact that GPtrArray is not a stateless structure, they are doing it wrong.

[ximion]

AsCache is private API, so it receives less attention with regards to annotations.
Since this is clearly breaking stuff, I have reverted the original change. Can you adjust this PR accordingly, so I can merge the remaining changes?
Thanks for looking into this - these issues are hard to find if you don't work with the bindings regularly.

[ricotz]

@ximion Thank and done.

On that regard, AS_ISSUE_IMPORTANCE_* and AS_ISSUE_KIND_* were public API too.
FYI nemequ/vala-girs@c00c888#diff-886fad95d44557ce930ae5dd4c379da2

[ximion]

Yikes! Those weren't intended to be public. If those were in use, I may need to add some compatibility code to handle them (the new AsIssueSeverity is public API though).

[ximion]

Looks good, thanks for looking into this issue!
I am traveling currently for about three weeks, but there will be a new release containing these fixes once I am back.