GNOME Bugzilla – Bug 654417
[GSoC] Add <binding> element to GtkBuilder syntax
Last modified: 2014-04-19 03:18:13 UTC
Part of my Google Summer of Code project "GObject property binding support in GtkBuilder and Glade" [1] ( ) is to add support for a new <binding> element to GtkBuilder to describe bindings between properties. It can appear inside of an <object> element and has the following form: <binding to="target-property" from="source-property" source="source-object"/> This declares that the property "target-property" of the defined object should always have the same value as the property "source-property" of "source-object". The accompanying Glade branch [2] uses this to read and save property bindings. The code can be found in the "gtkbuilder-gbinding" branch on git.gnome.org (the GtkBuilder doc comment has also been updated): http://git.gnome.org/browse/gtk+/log/?h=gtkbuilder-gbinding This bug is a request for review and feedback, and for tracking the readiness of the branch for inclusion to GTK+ master. [1] http://www.google-melange.com/gsoc/project/google/gsoc2011/denisw/12001 [2] http://git.gnome.org/browse/glade/log/?h=gbinding
I recently added support for GBinding transformation functions to the branch (commit 173bd1618dbeff02805535791336d16d243812f0 in the gtkbuilder-gbinding branch). For this purpose, I introduced the following definitions to GtkBuilder's public API: gtk_builder_create_bindings (GtkBuilder *builder, gpointer user_data); gtk_builder_create_bindings (GtkBuilder *builder, GtkBuilderBindingFunc func, gointer user_data); typedef void (*GtkBuilderBindingFunc) (GtkBuilder *builder, GObject *source, const gchar *source_property, GObject *target, const gchar *target_property, const gchar *transform_func, GBindingFlags flags, gpointer user_data); The gtk_builder_create_bindings*() functions create physical GBindings from the definitions read from the interface definition. They work exactly like gtk_builder_connect_signals*() wrt locating transformation functions from their names. With this commit, the branch is essentially feature complete and doesn't need any substantial additions. Documentation and two test cases (one with and one without transformation functions) are also included. It should thus be "safe" to review. I would be very gladful about any reviews or other kind of feedback!
It's unfortunate that we cannot harness the default GSignal codepaths to use transform functions as callbacks instead of an assigned function pointer. Currently, I have some doubts about how worthwhile it is to support the transform function. Some things to consider: a.) So far, before adding the transform function support, the bindings branch extends the builder functionality by allowing implicit bindings to be declared, without requiring any extra work on behalf of the user. b.) If I understand correctly, now that we have the transform-function, the user must explicitly call gtk_builder_create_bindings(), which is a kindof annoying extra step. c.) Also, now that we have the transform-function, language bindings become more complex (this is why it would be best if the only way to load a function from a builder file would always be via a GSignal), now language bindings need to add custom code in order to be able to even use gtk_builder_create_bindings(). d.) Does the transform function bring that much added value ? Can't we achieve something close already by just using the generic g_value_register_transform_func()[0] ? Perhaps the GBinding code does not fallback on g_value_transform(), which might then be a bug, or perhaps the transform-function is only there to provide specific custom transforms for those properties. From what I can see it seems that the transform-function is indeed only useful for custom transforms for that binding in particular, it is indeed a nice feature to have, is there some way we can get this without the added apis and leverage GSignal for this instead ? Pie-in-the-sky-vague-idea: a.) Deprecate the transform-func in favor of a proper GSignal on the GBinding object proper b.) Use similar syntax that you currently use, except setup all the bindings unconditionally at parse time c.) While parsing bindings, add the actual resulting GBinding objects to the GtkBuilder id hashmap with some specialized identifiers d.) Collect the transform-functions as if they were gsignals to be connected to the GBinding objects you have in the hash e.) When and if the user ever calls gtk_builder_connect_signals(), then the "transform" callbacks GSignals will be connected as a result. This would mean bindings would work but any transform functions would not be connected until gtk_builder_connect_signals() is called, it would also come with the weird side-effect of having ids like: "generated-binding-togglebutton1:active" in the GtkBuilder objects hash table... but it would allow us to achieve the whole transform-function thing without demanding any extra work from the api user or language binding implementor. Perhaps even something more elegant can be done, perhaps something can be done using g_object_bind_property_with_closures() ? Maybe GtkBuilder should break down signal connection logic into 2 steps: 1.) Create/Collect the GClosures needed to run abstract client code (this step requires cooperation from language bindings) 2.) Connecting the closures to the said signals Having a logical split at that level would allow us to use a GClosure created by the GtkBuilder in the same way as it would be created for a signal... however that's all very complex and heavy... I dont know, we should ask ourselves though, - do we want a cleaner solution for loading client code from GtkBuilder ? - do we want to just ditch the 'transform-function' features from GBinding in GtkBuilder for now until we do address GtkBuilder apis more thoroughly ? - can we live with adding more api to GtkBuilder that essentially does the same thing as loading signals, and push that api on our users (program authors and binding authors alike)... and be stuck maintaining this new api. [0]http://developer.gnome.org/gobject/unstable/gobject-Generic-values.html#g-value-register-transform-func
(In reply to comment #2) > From what I can see it seems that the transform-function is indeed only > useful for custom transforms for that binding in particular, it is indeed > a nice feature to have, is there some way we can get this without the > added apis and leverage GSignal for this instead ? Transformation functions are for a particular binding and are indeed pretty useful. Omitting them would seriously limit the utility of the whole feature. I chose my solution so that it requires a minimal amount of changes to the code and especially the public API. The reason that I didn't put it in gtk_builder_connect_signals() is that GtkBuilderConnectFunc doesn't provide all information needed for property bindings to be created (e.g., the binding source object and binding flags). If GBinding would be changed to use a signal as callback, it might be possible to ditch the new functions, but is it worth the required changes? What do others think?
On request of my mentor and Tristan, transformation function support has been removed for now in the "gtkbuilder-gbinding" branch because my implementation would have required special case logic in language bindings to make the automatic locating of transformation functions work (like is needed for gtk_builder_connect_signals() currently), and that other implementations would require changes to GBinding itself. For reference, the old code including transformation function support can still be found in a newly created "gtkbuilder-gbinding-transform" branch.
Juan Pablo Ugarto, now (co-?)maintainer of Glade, wrote me today and expressed his wish to merge my GSoC branch into Glade master, preferably before GNOME 3.4 feature freeze (if this is still possible). This depends on the "gtkbuilder-gbinding" branch landing in GTK+, so I would be really glad if someone could review this now. As a convenience, I'll attach the diff to this bug (it's not big).
Created attachment 207588 [details] [review] Diff between "gtkbuilder-gbinding" branch and master
Review of attachment 207588 [details] [review]: ::: gtk/gtkbuilder.c @@ +138,3 @@ + * property "from" of object "source" is set to, even if that property + * is later set to another value. For more information about the property + * binding mechanism, see #GBinding (which GTK+ uses internally). In addition to this, the relax ng schema should be updated to reflect this new syntax. @@ +890,3 @@ + g_object_bind_property (source, binding->from, + target, binding->to, + G_BINDING_SYNC_CREATE); Do we need to allow other binding flags to be specified in the xml ? I think we may need to. ::: gtk/gtkbuilderparser.c @@ +1145,3 @@ + "Duplicate binding for property: `%s'", + b->to); + } What about having both <binding to="foo"... and <property name="foo"... ? Your code doesn't seem to check for that.
Hmm its already Wednesday... so I think it will be better to leave this for next release. Ok so my idea is to make a stable release right after UI freeze (3.12) so then we can merge gbindings
Created attachment 207707 [details] [review] New "gtkbuilder-gbinding" / master diff The branch now includes the needed changes to the GtkBuilder schemas and checks for conflicting <binding> and <property> elements. As to binding flags, I haven't added this feature as the Glade branch currently doesn't deal use this. However, I think that the patch in its current form should make it possible to add it quite easily, possibly at a later time, and at least G_BINDING_INVERT_BOOLEAN would probably be good to have. In any case, G_SYNC_CREATE should be hardcoded as not setting that flag would leave the UI in an inconsistent state at start (the bound property might stay at its default value until the source property is changed).
As Glade 3.12 has been released now and the Glade gbinding branch could be theoretically merged now, I would be happy if the improved patch could be reviewed somewhere in the near future.
*** Bug 494329 has been marked as a duplicate of this bug. ***
Created attachment 274039 [details] [review] Adds <binding> new tag to GtkBuilder Hi guys I cleaned up and updated this patch to master. I assume we still want to have this new functionality. I know its been a long time but I cant remember if there any reason why we dont want this to be added to <property> tag? <property name="sensitive" bind-to="object.property" bind-flags="G_BINDING_SYNC_CREATE"/>
Created attachment 274140 [details] [review] Adds binding ssupport in <property> tag This patch implements a binding by adding a few extra parameters to <property> "bind-source" to specify the source object "bind-property" to specify the source property "bind-flags" to specify the binding flags
Review of attachment 274140 [details] [review]: ::: gtk/gtkbuilderprivate.h @@ +65,3 @@ gchar *data; + gboolean translatable:1; + gboolean is_binded:1; that should probably by named 'bound' instead of 'is_binded'
Review of attachment 274039 [details] [review]: ::: gtk/gtkbuilder.c @@ +136,3 @@ * + * It is also possible to define the value of a property by binding it to + * another property with a <binding> element. This causes the value We don't use xml escaping anymore - just say <binding>
I like the <property> extension slightly better, I think. If we ever make bindings more flexible (eg multiple sources being combined in various ways), then the <binding> syntax might be a better fit.
That is exactly how I feel, if we are going to keep this thing simple, and I think we all agreed on this, then I also like the <property> extension better. So in the current patch it will look like <property name="sensitive" bind-source="object" bind-property="property" bind-flags="G_BINDING_SYNC_CREATE"/> But I think specifying both the object and property in one parameter would be better <property name="sensitive" bind-source="object::property" bind-flags="G_BINDING_SYNC_CREATE"/> BTW any reason not to add "transform-to" and "transform-from" signals to GBinding? That is the only feature I can think of that can not be used creating a GBinding object directly in GtkBuilder
(In reply to comment #17) > That is exactly how I feel, if we are going to keep this thing simple, and I > think we all agreed on this, then I also like the <property> extension better. > So in the current patch it will look like > > <property name="sensitive" bind-source="object" bind-property="property" > bind-flags="G_BINDING_SYNC_CREATE"/> > > But I think specifying both the object and property in one parameter would be > better > > <property name="sensitive" bind-source="object::property" > bind-flags="G_BINDING_SYNC_CREATE"/> Thats only going to require you to add a pointless parser to gtkbuilder. Does it have any real advantage ? I'm also afraid we haven't declared "::" in ids illegal > BTW any reason not to add "transform-to" and "transform-from" signals to > GBinding? What would this do ? For the custom transform support ?
(In reply to comment #18) > Thats only going to require you to add a pointless parser to gtkbuilder. > Does it have any real advantage ? Not really, I meant it would look better more compact > I'm also afraid we haven't declared "::" in ids illegal fair enough, no problem then > > BTW any reason not to add "transform-to" and "transform-from" signals to > > GBinding? > > What would this do ? For the custom transform support ? Right, that would be the easiest way to define custom transform functions in builder if you want to create an gbinding object directly (without using this <property> extension) Anyways I am getting offtopic :)
Created attachment 274317 [details] [review] Binding support in <property> element Same patch, relax ng schema files fixed
Review of attachment 274317 [details] [review]: ::: gtk/gtkbuilderprivate.h @@ +65,3 @@ gchar *data; + gboolean translatable:1; + gboolean is_binded:1; Still: 'bound'
Created attachment 274330 [details] [review] Binding support in <property> element ahh, forgot to amend :)
As you can see I am not fond of irregular verbs :D
as discussed on IRC, storing the GBinding instance inside GtkBuilder should go away.
Created attachment 274705 [details] [review] Binding support in <property> element (updated) This patch does not expose gbindings objects in GtkBuilder
Pushed to master!