GNOME Bugzilla – Bug 690394
GBinding: g_object_bind_property_with_closures is not introspection friendly
Last modified: 2018-05-24 14:54:34 UTC
I am trying to replace the static exposure of GObject.Object.bind_property/_full in pygobject with introspection based bindings that will also allow transform closures. g_object_bind_property_with_closure is being exposed as "bind_property_full" by using the "Rename to" annotation. This is callable from python introspection, but the gbinding transform closure marshalling expects the transform result to be set as the third value of the closure params list. The closure return value is then used as a boolean return code, see: gobject/gbinding.c:bind_with_closures_transform_to Due to this, there does not seem to be a way to successfully use bind_property_full from introspection based bindings. There is no generic way to know that the third param is intended as an "out" param from what I can tell. Furthermore, the closures params list is also generally marked as const. I think we can remove the "Rename to" annotation on g_object_bind_property_with_closure and use g_object_bind_property_full directly through introspection with some annotation updates. Patch with proposed annotation fixes to follow shortly.
Created attachment 231784 [details] [review] Make g_object_bind_property_full introspection friendly I've verified this fixes things so python can successfully use introspection to bind properties with transform closures. However, this also revealed some potential problems in pygobject which also need to be fixed before we move off the static bind_property bindings. Does this seem like a reasonable approach to make bind_property_full to work through introspection?
the reason why the with_closures() was added was because introspection-based language bindings could not cope with two closures using a single user_data - i.e. there was no way to actually free the user_data once instead of twice.
Thanks Emmanuele, Is there an example of the with_closures version working through introspection? It doesn't make sense to me how it can work due to the lack of some way to discover parameter 3 is an out param for the closure (please clarify if I am wrong about this assumption). It could be fixed by changing the closure to return the transform result rather than a return code. But would this be an API break and there would also be no way to tell if an error occurred. On the other hand, the proposed annotation patch seems to supply the correct information about the shared user data in the gir/typelib. It it would then be a matter of interpreting it correctly within pygobject (somewhat difficultly).
Created attachment 263266 [details] [review] annotations: Update g_object_bind_property_with_closures with allow-none Add (allow-none) annotations to both transfer_to and transfer_from arguments of g_object_bind_property_with_closures. This is an updated patch which simply adds allow-none to bind_property_with_closures and leaves the rest alone.
(In reply to comment #0) > ... there does not seem to be a way to successfully use > bind_property_full from introspection based bindings. There is no generic way > to know that the third param is intended as an "out" param from what I can > tell. Furthermore, the closures params list is also generally marked as const. A bit of follow up in regards to why g_object_bind_property_with_closures doesn't work well with Python via introspection. I was a bit confused about the internals of GBinding when I initially logged this. After a bit of reading the code, I found that the closures out argument is a GValue wrapped with a GValue. This makes sense given the args list to GClosureMarshal is: const GValue *args, and a GValue wrapped GValue provides a way to use out args in the list. Unfortunately this wasn't totally obvious as it is not common to use pass by reference semantics for out arguments (at least in Python). There also doesn't seem to be a way to annotate GClosure argument as out (possibly related to bug 636812). Additionally there is a problem with PyGObjects marshaling of a closures GValue arguments in that they are recursively coerced into native value types. This renders the GClosure argument useless as a pass by ref out argument because we just get a Python int of 0. I dug up this commit from long ago which added the recursion: https://git.gnome.org/browse/pygobject/commit/?id=fe3966c2 I've verified bind_property_full is also broken with Gjs for a different reason. It doesn't do the recursive marshalling but probably copies of the boxed GValue intended as a pass by ref out arg which also renders it useless for setting the value (I will attach a Gjs example showing this). I think even if PyGObject removed the recursive marshalling, we would run into the same problem as Gjs because given the lack of ownership of the GValues passed into the closure, a copy needs to be made. Granted the Gjs example is not faulty, are there any thoughts on how to fix this? The options I've thought of could be to allow annotations for GClosure arguments which might work. But still gives us sort of a strange pass by ref API for high level languages. Another option would be to change how the closures of bind_property_with_closures work and instead of using a GValue wrapped GValue out argument, use the returned GValue as the transformed value. I have an experimental patch I will follow up with...
Created attachment 263267 [details] Gjs example showing bind_property_full problems Gjs example showing transform closures do not work.
Created attachment 263268 [details] [review] GBinding: Change bind_property_with_closures to use return value Use a closures return value as the transform value rather than a pass by reference GValue wrapped GValue. This fixes transform callbacks for languages like Python and Gjs which can now simply return the transformed value. Notes: This patch is just a test to get feedback on a potential solution. I have uncertainties regarding how to properly test if the return is "unset" as well as if this is an acceptable API change. Given this function is intended for language bindings and both Python and Gjs are broken, it seems somewhat reasonable. If it is not, we could always add a bind_property_with_closures2 and use that as the renamed version.
Review of attachment 263268 [details] [review]: I'm only somewhat following the details of this bug. A few high level comments: * From a quick grep across my sources, there are no direct users of bind_property_with_closures - Vala only has it mentioned in the .vapi, not clear if any vala consumers are using it - Hence, we could consider just entirely breaking this API and fixing it * I also don't see how the closure can unset a return value * This would all be possibly clearer with the addition of a *C* test case to demonstrate any changes in semantics
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/646.