GNOME Bugzilla – Bug 700898
Explain what "automated" mean in the context of children created with a template
Last modified: 2013-07-26 17:59:07 UTC
SSIA, really. the naming scheme does not relate at all with templates: it almost looks like you could call these functions without the templates being involved. what's being "automated"? the child, the creation of the child, or the assignment of the child? all of them? the 'bind_child' macros also have a different naming scheme as opposed to the function they are wrapping.
(In reply to comment #0) > SSIA, really. > > the naming scheme does not relate at all with templates: it almost looks like > you could call these functions without the templates being involved. > > what's being "automated"? the child, the creation of the child, or the > assignment of the child? all of them? Simply said, it's the association of an instance variable with a child created in the template. This is a bit more than simply 'assigning' the variable to the child but essentially it's the assignment. a.) The assignment does not actually have to be made, instead it can be simple the association of a string with an instance built by the template (creating this association is important for dynamic languages which define GTypes dynamically but have no location in instance private data to store the pointer value). b.) It also implies automated ref counting, the assigned variable is guaranteed to exist after gtk_widget_init_template() and until "destroy" time. > the 'bind_child' macros also have a different naming scheme as opposed to the > function they are wrapping. I regret having named it 'automate', that's is admittedly very vague. And I do like the jargon I've been seeing with the 'bind' name, so let's do rename automate_child... to, say: gtk_widget_class_bind_child_full() The macro is, generally nice for C code, originally I had no convenience macro and the strings / function pointers looked redundant and took up much more space in class initializers, so I'd like to keep the macros as they are.
(In reply to comment #0) > SSIA, really. > > the naming scheme does not relate at all with templates: it almost looks like > you could call these functions without the templates being involved. Missed this comment... pasting here what I just wrote on IRC: I think it would create very long API names if we started to add the word 'template' into those functions, I think as another step, we should just make sure that those APIs are well documented to be related to gtk_widget_class_set_template() Right now we have: gtk_widget_class_bind_child() gtk_widget_class_bind_child_internal() And potentially automate_child() becomes: gtk_widget_class_bind_child_full() I think it would be uncomfortably long if we added 'template': gtk_widget_class_template_bind_child() gtk_widget_class_template_bind_child_internal() gtk_widget_class_template_bind_child_full()
it turns out that bind_template_child() and bind_template_child_internal() are not much longer that gtk_widget_class_set_template_from_resource(), now that bug 702563 removed the need to pass the private data structure. personally, I think these are 100% better than the current naming scheme.
Created attachment 248429 [details] [review] Rename the widget template API The macros and functions are inconsistently named, and are not tied to the "template" concept - to the point that it seems plausible to use them without setting the template. The new naming scheme is as follows: gtk_widget_class_bind_template_child_full gtk_widget_class_bind_template_callback_full With the convenience macros: gtk_widget_class_bind_template_child gtk_widget_class_bind_template_child_internal gtk_widget_class_bind_template_callback https://bugzilla.gnome.org/show_bug.cgi?id=700898 https://bugzilla.gnome.org/show_bug.cgi?id=700896
Review of attachment 248429 [details] [review]: Hi Emmanuele, Thanks a lot for doing this change :) I read through most of the documentation changes and they look good, I've collected some comments which I'm leaving here. Now, I recently noticed that these structure offset issues seem to not be a part of your patch (it seems I did not get a chance to review this prior change for private data)... however currently it seems things are quite broken, so I'm including those comments here. The meaning of structure offsets has changed for the new G_PRIVATE_OFFSET() meaning (negative private values), also allowing to set values to public struct members. The corresponding code which manages the actual object pointers needs to be updated, in these two places: https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n11398 https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n15653 Note currently they do: if (child_class->offset >= 0) Which must become: if (child_class->offset != 0) And then they proceed to do: class_private = G_TYPE_INSTANCE_GET_PRIVATE (widget, class_type, gpointer); destination = G_STRUCT_MEMBER_P (class_private, child_class->offset); Which, I presume must become something like: destination = G_STRUCT_MEMBER_P (widget, child_class->offset); (I think that's how the private offsets work, I could be wrong). Some documentation related issues in the patch: The macros: gtk_widget_class_bind_child() and gtk_widget_class_bind_child_internal() Were renamed to: gtk_widget_class_bind_template_child() and gtk_widget_class_bind_template_child_internal() But the gtk-doc annotations were not updated to reflect this change.
(In reply to comment #5) > Review of attachment 248429 [details] [review]: > > Hi Emmanuele, > > Thanks a lot for doing this change :) > > I read through most of the documentation changes and > they look good, I've collected some comments which > I'm leaving here. > > Now, I recently noticed that these structure offset > issues seem to not be a part of your patch (it seems > I did not get a chance to review this prior change > for private data)... however currently it seems > things are quite broken, so I'm including those > comments here. Correction. It seems I'm missing part of the patch here. Your attached patch says: -#define gtk_widget_class_bind_child(widget_class, data_type, member_name) \ - gtk_widget_class_automate_child (widget_class, #member_name, FALSE, \ - G_PRIVATE_OFFSET (data_type, member_name)) +#define gtk_widget_class_bind_template_child(widget_class, TypeName, member_name) \ + gtk_widget_class_bind_template_child_full (widget_class, \ + #member_name, \ + FALSE, \ + G_PRIVATE_OFFSET (TypeName, member_name)) But in git master it's still using G_STRUCT_OFFSET() and not G_PRIVATE_OFFSET(). So things are not broken as I suspected in master, but I'm not seeing the whole picture. Basically, as I mentioned, if we plan to change the G_STRUCT_OFFSET() to allow negative values then the related code which accesses G_STRUCT_MEMBER_P() must be updated too (to access the offset on the instance pointer, and not the private struct pointer).
as I said in comment 3, this patch requires attachment 248428 [details] [review] from bug 702563 to be applied first, which is independent of the rename. that patch, in turn, needs the patchset from bug 702996 to be applied. the sequence is: - apply the patchset from bug 702996, which transition gtk to use offsets to access the private data; - apply attachment 248428 [details] [review] from bug 702563, which move the template API to use generic offsets; - apply attachment 248429 [details] [review] from this bug, which renames the template API.
(In reply to comment #7) > as I said in comment 3, this patch requires attachment 248428 [details] [review] from bug 702563 > to be applied first, which is independent of the rename. that patch, in turn, > needs the patchset from bug 702996 to be applied. > Ah, sorry for the noise then (I missed the fact that bug 702563 is still not resolved). The only comment which applies to this bug then, is the one about changes to the macro names, but forgetting to change the names in the gtk-doc comment blocks (minor detail but still ;-) ). I'll take a look at the other patch too.