Shouldn't this be titled "UI Builder support in GTK+" or are you
suggesting that we actually incorporate glade (not libglade) into
the GTK+ tree?

Owen: Right, updating summary.

Created attachment 39767 [details] [review]
v1: first draft of GtkBuilder

Adds GtkBuilder, an example, a glade file and a DTD.
GObject support over libglade. GtkUIManager support in xml format.
GtkSizeGroup, GtkTreeViewColumn and GtkCellRenderers supported.
Basic support for deprecated widgets, not complete backwards compatible with
libglade. Broken widgets removed (and GtkPreview)
No interface implemented for containers

Documentation needs to be updated and added for new methods. Is the name okay?
Do we need an interface or can it wait? 
Do we need better backwards compatibility with libglade?

I feel like if we're going to have the chance to break file-format
compatibility, then we should break it a little more drastically than this.
There are a bunch of odd special cases in the format that could be fixed if
we allowed <widget> to be a child of <property>. Eg, instead of saving
notebook tab labels as children with special magic packing types, we could
just store them directly as the "tab_widget" child property of their
corresponding page. Likewise for expander and frame labels, and the menu
"children" of GtkOptionMenu and GtkMenuItem.

Less breakingly, I'd suggest changing the signature of the custom widget
creation function, to have a user_data at the end (which was passed into
gtk_builder_new). The current glade way requires you to use global variables
if you need to pass data to the creation function.

Also, if anyone ever succeeds in writing an app that uses two different gtk
language bindings simultaneously, gtk_builder_set_custom_handler() would
have to be per-builder, not global. (In fact, it could just be a virtual
method, and bindings authors could subclass GtkBuilder to override it.)

Punting some big, unfinished features from 2.8

Created attachment 65210 [details] [review]
v2: rewrite from scratch.

Rewrite of libglade providing most needed functionallity.
Parsing is now doing in one step instead of two for increased speed and decreased footprint.
For more information see http://article.gmane.org/gmane.comp.gnome.gtk%2B.devel.general/10160

The patch is not yet complete, but from an API point of view it should be pretty stable. More functionallity, especially in the parser can be added without breaking the API.

Created attachment 65329 [details] [review]
v3: include tests, move out widget & window property handling

This moves the handling of GtkWidget::has_focus/has_default and visible for GtkWidnow subclasses out of GtkBuilder. It also removes the special treatment of GtkAdjustment and strncmp abuse.

Created attachment 65787 [details] [review]
v4: treeview/celllayout, liststore, tooltips, dialog, signals autoconnect, 

Adds support for treeview to displaying columns and cells, tooltips & dialogs are supported, signal autoconnect works aswell.

First impressions, after reading the first half of the patch:

Meta comment
------------

Hard to review a 8000 line patch. I'd like to see this broken up
in a few smaller patches (or as the kernel guys say, "a proper
patch sequence"):
1) necessary preparations (like adding the GtkCellView::model property)
2) add the GtkBuildable interface and GtkBuilder itself
3) add tests
4) implement GtkBuildable throughout the hierarchy


Bigger things
-------------

I think the GtkBuildable interface needs some documentation explaining
when I have to implement which functions, and what they are supposed
to do. The applies to some of the peculiarities of GtkBuilder, e.g.
delayed properties. 

Do you have any plan for documenting the allowed values for the
type parameter of GtkBuildable::add ?

Can a GtkBuilder be used only once, or can I give it multiple files ?
I think it would be slightly nicer to have a _new() function which
just constructs a new GtkBuilder, and a separate _parse() function
which parses a file.

Why are GtkActions and action groups buildable ? That goes against my 
intuition that actions are defined by the code, and the ui definition 
hooks them up with a control interface.

You might want to hold off on making tooltips buildable, since 
Kris wants to land his new tooltip api soon.

The API documentation is obviously not quite done yet. We also need
docs for the xml format - not necessarily a DTD, but some description
of the general structure, and also any special tags which are
supported for individual objects. Including examples. Ideally,
we'd also add a chapter to our migration guides explaining how
to convert from glade to the new format (what is the name of the
new format, btw ?)


Stylistic stuff
---------------

Most comments are only exemplaric, and apply to other places as well.


+static void
+gtk_widget_buildable_set_property (GtkBuildable        *buildable,
+				   GtkBuilder          *builder,
+				   const gchar         *name,
+				   const GValue        *value)
+{
+  if (!strcmp (name, "has_default") && g_value_get_boolean (value) == TRUE)
+    g_signal_connect_swapped (builder, "finish",
+			      G_CALLBACK (gtk_widget_grab_default),


What if the name is given as "has-default" ?
Don't do == TRUE for booleans


+static GMarkupParser size_group_parser =
+  {
+    size_group_start_element
+  };
+


Should be const


+G_DEFINE_TYPE_WITH_CODE (GtkSizeGroup, gtk_size_group, G_TYPE_OBJECT,
+			 G_IMPLEMENT_INTERFACE (GTK_TYPE_BUILDABLE,
+						gtk_size_group_buildable_init));


No ; after G_DEFINE_TYPE


+      parser_data = g_new0 (GSListSubParserData, 1);


Could use g_slice here


+static void
+gtk_size_group_buildable_custom_tag_end (GtkBuildable  *buildable,
+					 GtkBuilder    *builder,
+					 GObject       *child,
+					 const gchar   *tagname,
+					 gpointer       data)
+{
+  g_signal_connect (builder, "finish",
+		    (GCallback)builder_finish_cb, data);
+}


Is it necessary to do this in custom_tag_end, or can we move
it to custom_tag_start, and do away with the custom_tag_end ?


+static void
+gtk_expander_buildable_init (GtkBuildableIface *iface)
+{
+  parent_buildable_iface = g_type_interface_peek_parent (iface);


parent_buildable_iface seems unused in gtkexpander.c


+gboolean
+gtk_cell_layout_buildable_custom_tag_start (GtkBuildable  *buildable,


Should be static, or if needed in other source files, _-prefixed.
This is not public api, right ?


+/* gtkbuilderparser.h


Comment and guard define don't match the header filename.


+  g_object_set_data_full (obj,
+			  "gtk-builder-name",
+			  g_strdup (info->id),
+			  g_free);


I wonder why you add a GtkSizeGroup private just for the name, when
you set it as object data on all objects, anyway.



+      if (!GTK_BUILDABLE_GET_IFACE (info->object) ||
+	  !GTK_BUILDABLE_GET_IFACE (info->object)->get_internal_child)
+	continue;
+      
+      obj = gtk_buildable_get_internal_child (GTK_BUILDABLE (info->object),
+					      builder,
+					      childname);


This should just be 
   if (GTK_IS_BUILDABLE (info->object)
     obj = gtk_buildable_get_internal_child (GTK_BUILDABLE (info->object),
                                             builder,
                                             childname);
and gtk_buildable_get_internal_child should just return NULL if 
get_internal_child is NULL
  

+void
+_gtk_builder_finish (GtkBuilder  *builder,
+		     const gchar *domain)


Why is the domain passed in here ? Looking at parse_buffer, it
appears the domain is already known before the parsing starts,
so you could just set it on the builder there.


+const gchar*   gtk_builder_get_name           (GtkBuilder *builder,
+					       GObject *object);


Should this be get_object_name ? It is not returning the name
of the builder, right ?


+  item = g_new (gpointer, 2);
+  item[0] = g_strndup (text, text_len);
+  item[1] = parser_data->response;


How about using a struct here, for clarity ?


+  /* Do not free the signal list, the GtkBuilder takes
+     ownership of it */
+  g_slist_free (info->signals);  


What gives ? 


+    g_error ("<property> needs a name");


It would be nice if these errors could contain some location
information, like filename:linenumber. Also, there are a lot
of them with similar strings, so it might be nice to parametrize
that a bit:
   g_error ("%s:%d: Tag <%s> requires attribute "%s"", 
            filename, lineno, tag, attribute);


+  if (G_UNLIKELY (gtk_debug_flags & GTK_DEBUG_BUILDER))
+    {
+      GString *tags = g_string_new ("");
+      int i;
+      for (i = 0; names[i]; i++)
+	g_string_append_printf (tags, "%s=\"%s\" ", names[i], values[i]);
+      
+      if (i)
+	{
+	  g_string_insert_c (tags, 0, ' ');
+	  g_string_truncate (tags, tags->len - 1);
+	}
+      g_print ("<%s%s>\n", element_name, tags->str);
+      g_string_free (tags, TRUE);
+    }
+


Some of the excessive debug code should either be removed, or surrounded
by
#ifdef GTK_ENABLE_DEBUG
#endif


+  else if (!strcmp (element_name, "glade-interface"))


What is glade-interface ?

> Hard to review a 8000 line patch. I'd like to see this broken up
> in a few smaller patches (or as the kernel guys say, "a proper
> patch sequence"):
> 1) necessary preparations (like adding the GtkCellView::model property)
> 2) add the GtkBuildable interface and GtkBuilder itself
> 3) add tests
> 4) implement GtkBuildable throughout the hierarchy

I'll swap 3 & 4 and insert a new one in the beginning for the lazy type utilities.

> I think the GtkBuildable interface needs some documentation explaining
> when I have to implement which functions, and what they are supposed
> to do. The applies to some of the peculiarities of GtkBuilder, e.g.
> delayed properties. 

Agreed. I started doing this, I will complete the rest of the documentation.

> Do you have any plan for documenting the allowed values for the
> type parameter of GtkBuildable::add ?

It's defined per widget, similar to custom tags.
Currently only used by GtkExpander, GtkFrame and GtkNotebook.

> Can a GtkBuilder be used only once, or can I give it multiple files ?
> I think it would be slightly nicer to have a _new() function which
> just constructs a new GtkBuilder, and a separate _parse() function
> which parses a file.

Sure, sounds good. I'm planning to add xul like overlays at a future point, which is one way of allowing you to dynamically merge in new interfaces.
The behavior of calling parse multiple times is undefined.

> Why are GtkActions and action groups buildable ? That goes against my 
> intuition that actions are defined by the code, and the ui definition 
> hooks them up with a control interface.

I thought it's a nice feature to have, to be able to implement actions
inside a ui manager. Of course, you'll not be forced to use them. 
In my opinion, as much as possible should be doable in a user interface
designer.

> The API documentation is obviously not quite done yet. We also need
> docs for the xml format - not necessarily a DTD, but some description
> of the general structure, and also any special tags which are
> supported for individual objects. Including examples. Ideally,
> we'd also add a chapter to our migration guides explaining how
> to convert from glade to the new format (what is the name of the
> new format, btw ?)

I'll improve the documentation, I hesitated to write it properly, to be able to
focus on the implementation details. Agreed a description of the general
structure is necessary.

> +static void
> +gtk_size_group_buildable_custom_tag_end (GtkBuildable  *buildable,
> +                                        GtkBuilder    *builder,
> +                                        GObject       *child,
> +                                        const gchar   *tagname,
> +                                        gpointer       data)
> +{
> +  g_signal_connect (builder, "finish",
> +                   (GCallback)builder_finish_cb, data);
> +}
> 
> Is it necessary to do this in custom_tag_end, or can we move
> it to custom_tag_start, and do away with the custom_tag_end ?

Good point. I think custom_tag_end can go away entierly.
It'll make it a little tricker when having multiple parsers (eg disconnecting
the signal when done)

> +gboolean
> +gtk_cell_layout_buildable_custom_tag_start (GtkBuildable  *buildable,
> 
> Should be static, or if needed in other source files, _-prefixed.
> This is not public api, right ?

I made it public to be able to use attributes for objects implementing GtkCellLayout which is not inside gtk.

> +      if (!GTK_BUILDABLE_GET_IFACE (info->object) ||
> +         !GTK_BUILDABLE_GET_IFACE (info->object)->get_internal_child)
> +       continue;
> +      
> +      obj = gtk_buildable_get_internal_child (GTK_BUILDABLE (info->object),
> +                                             builder,
> +                                             childname);
> 
> 
> This should just be 
>    if (GTK_IS_BUILDABLE (info->object)
>      obj = gtk_buildable_get_internal_child (GTK_BUILDABLE (info->object),
>                                              builder,
>                                              childname);
>
> and gtk_buildable_get_internal_child should just return NULL if 
> get_internal_child is NULL

That won't work for some dialogs, created in the glade way. You need to be able
to reference an internal child which is not in a direct parent:
<fontselectiondialog>
  <object ... internal-child="action_area">
    <object ... internal-child="ok_button">

ok_button is an internal child (eg attribute) of fontselectiondialog, not of action_area.

> +_gtk_builder_finish (GtkBuilder  *builder,
> +                    const gchar *domain)
> 
> 
> Why is the domain passed in here ? Looking at parse_buffer, it
> appears the domain is already known before the parsing starts,
> so you could just set it on the builder there.

It's not always known, there are two ways of specify the domain;
1) in the constructor, just like libglade
2) as tag at the root element

1) is the only way you can do in libglade.
2) was added to gazpacho, which is very useful for interfaces which 
are part of libraries, where the domain is static but different 
from the default one (application defined).

> +const gchar*   gtk_builder_get_name           (GtkBuilder *builder,
> +                                              GObject *object);
>
>
> Should this be get_object_name ? It is not returning the name
> of the builder, right ?

That was actually fixed in v4 of the patch.

> +  /* Do not free the signal list, the GtkBuilder takes
> +     ownership of it */
> +  g_slist_free (info->signals);  
> 
> What gives ? 

Not sure I understand what you mean here.
I'm reusing the content of the list in signal autoconnect, because of that
it should not be freed by the parser itself.

> Some of the excessive debug code should either be removed, or surrounded
> by
> #ifdef GTK_ENABLE_DEBUG
> #endif

I added ifdef guards around the start/end element output.
I still kept the object/property output enabled, because it's more useful.

> +  else if (!strcmp (element_name, "glade-interface"))
> 
> 
> What is glade-interface ?

The root element, from libglade. 
I'll rename it to interface and actually make sure it's the root element.

>> This should just be 
>>    if (GTK_IS_BUILDABLE (info->object)
>>      obj = gtk_buildable_get_internal_child (GTK_BUILDABLE (info->object),
>>                                              builder,
>>                                              childname);
>>
.> and gtk_buildable_get_internal_child should just return NULL if 
>> get_internal_child is NULL
>
>That won't work for some dialogs, created in the glade way. You need to be able
>to reference an internal child which is not in a direct parent:
><fontselectiondialog>
>  <object ... internal-child="action_area">
>    <object ... internal-child="ok_button">
>
>ok_button is an internal child (eg attribute) of fontselectiondialog, not of
>action_area.

Hmm, not sure I see why this is a problem. I think the outcome will be just the
same if the code is changed as I proposed, but I may be missing something.

>> Why is the domain passed in here ? Looking at parse_buffer, it
>> appears the domain is already known before the parsing starts,
>> so you could just set it on the builder there.
>
>It's not always known, there are two ways of specify the domain;
>1) in the constructor, just like libglade
>2) as tag at the root element
>
>1) is the only way you can do in libglade.
>2) was added to gazpacho, which is very useful for interfaces which 
>are part of libraries, where the domain is static but different 
>from the default one (application defined).

Makes a lot of sense.

>> +  /* Do not free the signal list, the GtkBuilder takes
>> +     ownership of it */
>> +  g_slist_free (info->signals);  
>> 
>> What gives ? 
>
>Not sure I understand what you mean here.

The comment seems to contradict the code immediately following it...

Created attachment 66079 [details] [review]
v5: typeutils

Also includes debugging support in gtkmain.

Created attachment 66080 [details] [review]
v5: gtkbuildable, gtkbuilder & gtkbuilderparser.c

Created attachment 66081 [details] [review]
v5: implement buildable

Created attachment 66082 [details] [review]
v5: Add GtkCellView:model property

Created attachment 66083 [details] [review]
v5: add tests

Comment on attachment 66082 [details] [review]
v5: Add GtkCellView:model property

The GtkCellView::model
patch is good, please commit.

Comment on attachment 66079 [details] [review]
v5: typeutils

Do you envision gtk_enum_from_string and
gtk_flags_from_string to move to gobject at some point ? In that case, we may want to keep them private for now. 

gtk_lazy_type_from_name
is not a very clear name, 
IMO. You could argue that g_type_from_name is actually lazier than your
variant...

Is the type registration thing supposed to be extensible at runtime, ie 
is gtk_type_register meant to be called outside gtk ?

I would love to avoid the duplication between the relocation-causing array and the hash table.

Comment on attachment 66080 [details] [review]
v5: gtkbuildable, gtkbuilder & gtkbuilderparser.c



+gboolean gtk_builder_value_from_string (GParamSpec  *pspec,
+					const gchar *string,
+					GValue      *value);
+gboolean gtk_builder_value_from_string_type (GType        type,
+					     const gchar *string,
+					     GValue      *value);

Same question as for the type utils: is this needed as public api ?

What is the relation between gtk_builder_get_object_name
and gtk_buildable_set_name ?
Is the name stored via the interface or as qdata ?

Comment on attachment 66080 [details] [review]
v5: gtkbuildable, gtkbuilder & gtkbuilderparser.c

Why is GtkBuilderPrivate.delayed_properties 
freed in finish, but the rest in finalize ? If you want to make repeated parse  calls work, finish
should probably return to the same state as init

(In reply to comment #19)
> (From update of attachment 66079 [details] [review] [edit])
> Do you envision gtk_enum_from_string and
> gtk_flags_from_string to move to gobject at some point ? In that case, we may
> want to keep them private for now.

Yes, I hope they'll make it to glib at some point. They seem to be generally useful. So I'll make them private.

> gtk_lazy_type_from_name
> is not a very clear name, 
> IMO. You could argue that g_type_from_name is actually lazier than your
> variant...

I don't know of a better name, do you have any suggestions?
gtk_type_from_name is already taken.

> Is the type registration thing supposed to be extensible at runtime, ie 
> is gtk_type_register meant to be called outside gtk ?

Yes, that's why it was added.
Otherwise "FooWidget" won't work unless you force a call to foo_widget_get_type
before instantiating your builder object.

> I would love to avoid the duplication between the relocation-causing array and
> the hash table.

Would changing it to be a long list of g_hash_table_insert calls solve that?

> +gboolean gtk_builder_value_from_string (GParamSpec  *pspec,
> +                                       const gchar *string,
> +                                       GValue      *value);
> +gboolean gtk_builder_value_from_string_type (GType        type,
> +                                            const gchar *string,
> +                                            GValue      *value);
> 
> Same question as for the type utils: is this needed as public api ?

These ones are needed to be able to implement custom parsers outside of gtk.
Tim does not want them in glib, but I would prefer making them public, to avoid having to reimplement them.

> What is the relation between gtk_builder_get_object_name
> and gtk_buildable_set_name ?
> Is the name stored via the interface or as qdata ?

gtk_buildable_set_name sets the name as qdata, it also allows the object to synchronize the name of the builder with it's internal name, if it has one.
GtkWidget implements this and updates it's own name attribute.
This is useful so widgets created by the builder will have the same name as they do in the xml representation.

(In reply to comment #21)
> (From update of attachment 66080 [details] [review] [edit])
> Why is GtkBuilderPrivate.delayed_properties 
> freed in finish, but the rest in finalize ? If you want to make repeated parse 
> calls work, finish
> should probably return to the same state as init

Good point.

Repeated call to parse() *might* require the whole tree saved.

Comment on attachment 66080 [details] [review]
v5: gtkbuildable, gtkbuilder & gtkbuilderparser.c

I think the way the GtkBuildable interface is documented is ideal.

"This method can be implemented to...". The documentation for gtk_buildable_... should just explain what this function does, we need some other way to document the interface for people who want to implement it. I know that we have no prior art for that...

>Yes, that's why it was added.
>Otherwise "FooWidget" won't work unless you force a call to foo_widget_get_type
>before instantiating your builder object.



What is the practical difference between calling

gtk_type_register (foo, bar)

before instantiating the builder and calling

foo_get_type () 

?

(In reply to comment #26)
> >Yes, that's why it was added.
> >Otherwise "FooWidget" won't work unless you force a call to foo_widget_get_type
> >before instantiating your builder object.
> 
> 
> 
> What is the practical difference between calling
> 
> gtk_type_register (foo, bar)
> 
> before instantiating the builder and calling
> 
> foo_get_type () 
> 
> ?

gtk_type_register can be done in foo_init (), to make sure that the type is not actually registered until it's needed. It might be a big deal for widget libraries which registers 10-15 widgets or more.

I doubt that registering 10-15 types can be a big deal, but ok

lets get this in early in the 2.12 cycle.

Can I ask about the status of this bug?  Did this manage to make it into the 2.12 cycle, or will we have to wait until 2.14 to start developing for it?  The CVS repository for gtk+ doesn't seem to contain it, but I'm not sure if it's too late to get it included in 2.12...

Created attachment 75396 [details] [review]
v6: typeutils

Avoid relocations by using direct calls to g_hash_table_insert.
gtk_lazy_type_from_name -> gtk_registered_type_from_name
Made the flag/enum -> string functions private.
Removed debugging parts which belongs to the builder patch.

Created attachment 75397 [details] [review]
v6: gtkbuildable, gtkbuilder & gtkbuilderparser.c

Frees delayed properties in finalize.
Documentation updates
Avoids exporting symbols as PLT.

Created attachment 75398 [details] [review]
v6: implement buildable

Updated for trunk and API changes. Avoids PLT entries.

Created attachment 75399 [details] [review]
v6: tests

Updated for trunk

Created attachment 75411 [details] [review]
v6.1: gtkbuildable, gtkbuilder & gtkbuilderparser.c

Contains a couple of documentation fixes and some unused code removed.

I am about to attach a storm of patches, this is basicly my
own interpretation of Johan's work... I picked up his tree
and expanded on it... I will include a detailed changelog of
everything I did at the end...

Created attachment 77648 [details] [review]
gtkbuilder/gtkbuildable/gtkbuilderparser portion

This is an incremental patch that assumes you have already
applied Johan's work to gtk+ HEAD.

Created attachment 77649 [details] [review]
typeutils portion (tristan's patches)

This is an icremental patch that assumes you have Johan's work
applied.

Created attachment 77650 [details] [review]
implement buildable portion (tristan's patches)

Again, an incremental patch on Johan's work

Created attachment 77651 [details] [review]
tests (tristan's patches)

Another incremental patch on Johan's patches...

Note that the tests arent very clean the way I left them,
but they do demonstrate ALOT of capacities of the builder...
the glade file now references Fry.png... I guess I'll attach that too.

Created attachment 77652 [details]
The png file for gtk+/tests/Fry.png

Created attachment 77653 [details] [review]
A Convenience "all-in-one" patch against cvs (tristan's patches)

With the above changes, you should be able to load any glade file
with the GtkBuilder with the sole exception of glade files containing
GtkCList widgets (look at libglade/glade/glade-gtk.c:clist_build_children())
to understand why ;-p

Here is a changelog of what I did in these patches:

   - Implemented accelerators (work should be done on making sure
     accelerators dont conflict with those created by uimanager).

   - Implemented versioning: <glade-interface version="1">

   - Parse adjustments and pixbufs the old fashioned way for version 1 
     glade files.

   - Handle unsupported atk property tags so that they are not mistaken for
     custom tags (preemptively building the object and the constructor ignoring 
     any trailing properties, such as accelerators).

   - Ported BOOL macros to the private header from libglade (this fixes non 0
     integer values, case insensitive yes/true as valid truth strings from
     the xml, formerly it was only "yes" that was supported, but libglade 
     supports more).

   - Handle packing properties in the builder parser proper, added 
     buildable->child_set_property() method and implemented it in GtkContainer,
     this allows non-gtkcontainers to have packing properties without
     doing that complex parsing jargon and also allows GtkContainer subclasses
     to handle buildable->child_set_property() in thier own way and chain up.

   - Allow virtual properties (packing or otherwise), properties listed in 
     the glade file that dont have coresponding pspecs installed will be 
     delivered to the buildable's set_property()/child_set_property()
     method as G_TYPE_STRING.

     The rationale behind this change is that it allows us to port in all the
     special-case code from libglade without opening up the can-of-worms 
     attached to making these special-case properties "real" object properties,
     this also made it rediculously easy to port all the support code from
     libglade in a safe manner.

   - Icky special case code added for GtkDialog --> GtkButton relationships
     (wrt response-id), added in the buildable code strictly for version 1
     glade files.

   - Dealt with digging out "type" packing properties in the case of
     version 1 glade files. (the new style is <child type="tab">, older
     glade files store "tab" in a packing property).

That pretty much describes what I did to the builder in short, I'll let
you guys draw your own conclusions... hoping to hear what people have
to say :)

w00t.  So cool...
Lets get this rolling again.

Tristan: Thanks for taking the time to do this work. However as there are already enough patches on this bug, I think it's better if you open up a new bug and make it depend on this one.

I'll review your patch(es) as soon as Matthias has given a go on the rest.

Matthias, ping?

Hi,

more than a bug it's a RFE:

using Glade-GUI I would like add for a widget's signal (example a button) in "Object" field, a function-pointer for a generic function.

Example:

void on_button_clicked(GtkButton * button, gpointer user_data)
{
	...
	void (* func) (...) = user_data;
        ...
}

but actually, in run-time you'll get "user_data = 0x0".

Could be it a good "future-feature"?

Thanks,
Giulio

the enum lookup function can't be moved to glib and can't go into gtk either, because it is inherently broken:

>gint
>+_gtk_enum_from_string (GType type, const char *string)
>+{
>+  GEnumClass *eclass;
>+  GEnumValue *ev;
>+  gchar *endptr;
>+  gint ret = 0;
>+
>+  ret = strtoul (string, &endptr, 0);
>+  if (endptr != string) /* parsed a number */
>+    return ret;
>+
>+  eclass = g_type_class_ref (type);

here, you load the type's .so file so it's strings are in memory.

>+  ev = g_enum_get_value_by_name (eclass, string);
>+  if (!ev)
>+    ev = g_enum_get_value_by_nick (eclass, string);
>+
>+  if (ev)
>+    ret = ev->value;

here, you store a constant string pointer to a string.

>+
>+  g_type_class_unref (eclass);

here, you *unload* the type's .so file, so all it's functions, data and strings are removed from memory.

>+
>+  return ret;

at this point, the string pointer is an INVALID MEMORY REFERENCE.
>+}

so either you strdup the returned string before doing class_unref, or you use the existing GLib API for enum value lookups which force the caller to keep the enum class alive around calling the lookup function.

i'm sorry, Maxim Udushlivy correctly pointed out to me in private email that i've been misanalyzing the following code snippet:

(In reply to comment #47)
> the enum lookup function can't be moved to glib and can't go into gtk either,
> because it is inherently broken:
[...]
>+  gint ret = 0;
[...]
> >+  if (ev)
> >+    ret = ev->value;
> 
> here, you store a constant string pointer to a string.
[...]
> >+  return ret;

the code is not broken with regards to string references, because the extracted and returned value is an integer and not a string. it's all my fault, i've made an attempt at explaining why stale string references are bad while being completely blind to the fact that the affected type was an integer ;-)

Coming back after a long time, here are some comments on v6: typeutils

I don't particularly like the gtk_type_register stuff, but since we don't 
have introspection, it is clear that we need it. 

You might run into bug 403823 with gtk_registered_type_from_name().

gtk_type_register needs to be documented, and there probably also needs to be some higher-level documentation explaining that it is necessary to call gtk_type_register to make your library "GtkBuilder-ready". How do we expect third-party libraries to do that, btw ? export an init() function ?

(In reply to comment #49)
> Coming back after a long time, here are some comments on v6: typeutils
> 
> I don't particularly like the gtk_type_register stuff, but since we don't 
> have introspection, it is clear that we need it. 

Right. It's likely to be deprecated as soon as we get introspection.

> You might run into bug 403823 with gtk_registered_type_from_name().

Ouch, should probably fixed in glib, not sure it's worth working around that in this bug.

> gtk_type_register needs to be documented, and there probably also needs to be
> some higher-level documentation explaining that it is necessary to call
> gtk_type_register to make your library "GtkBuilder-ready". How do we expect
> third-party libraries to do that, btw ? export an init() function ?

While writing the API I envisioned the third-party libraries to call gtk_type_register some time during initialization (just after gtk_init for instance):

  gtk_init(..)
  gtk_type_register("FooButton", foo_button_get_type)
  gtk_type_register("BarEntry", bar_entry_button_get_type)

I'll add documentation and follow up with a new patch shortly.

Well, since they are libraries, they don't really have any business calling gtk_init()... so, you'd have to add an init() function to each library and
have the application call that:

gtk_init (...);
libgnome_init ();
libgnomecanvas_init ();

which is not nice. There is not a lot of good alternatives, though

- if you play games with __attribute__(constructor), 
  that gets executed before main()

- if you try to heuristically guess the mapping from type name to get_type()
  function name, it is going to fail sometimes

- you could try to dump that mapping into some files somewhere in the filesystem
  and parse those at gtk_init() time - that might be the least evil option

(In reply to comment #51)
> Well, since they are libraries, they don't really have any business calling
> gtk_init()... so, you'd have to add an init() function to each library and
> have the application call that:
> 
> gtk_init (...);
> libgnome_init ();
> libgnomecanvas_init ();

Oh that's true, I forgot about that.

> - you could try to dump that mapping into some files somewhere in the
> filesystem
>   and parse those at gtk_init() time - that might be the least evil option

GObject introspection would already do this if I'm not mistaken.

A fourth option could also be, to solve this problem in a generic way:

- add something like GnomeProgram which in combination with a library 
  specific startup routine.

> - if you try to heuristically guess the mapping from type name to get_type()
>   function name, it is going to fail sometimes

This is not too bad actually, since most well behaved libraries will use the
same naming mechanism as GTK+. For not well behaved, just require a _init function or wait for introspection.

How does that sound?

> > - you could try to dump that mapping into some files somewhere in the
> > filesystem
> >   and parse those at gtk_init() time - that might be the least evil option
> 
> GObject introspection would already do this if I'm not mistaken.

Yes, this would essentially be the very-very-minimal introspection just for getting this going.

> > - if you try to heuristically guess the mapping from type name to get_type()
> >   function name, it is going to fail sometimes
> 
> This is not too bad actually, since most well behaved libraries will use the
> same naming mechanism as GTK+. For not well behaved, just require a _init
> function or wait for introspection.

I guess we can try that and see how it goes. One other option might be to just
let GUI builders dump the map into the xml somewhere, since they most certainly know. If calling g_type_from_name() fails and there is no map in the xml, you
can still fall back to heuristics to go from type name to get_type function.

Might be worth taking this to gtk-devel-list.

comments on buildable v6.1:

- could probably use g_type_register_static_simple()

- why is gtk_buildable_set_name() always storing a copy of the
  name as object data ? I think it would be cleaner to also
  virtualize get_name, and then do away with the extra copy of the
  name if the object already has a name property.

- it also seems slightly odd that there is a gtk_builder_get_object_name.
  why is that needed, in addition to gtk_buildable_get_name ?

- The docs for the type parameter of gtk_buildable_add() could be
  improved. "how the child should be added" doesn't really say a lot.
  How about "Some objects have different 'slots' in which children 
  can be added. The @type parameter can be used to determine this 
  slot." Or something to that effect. And then give a very
  contrete example:

   #GtkNotebook uses the @type to distinguish between page labels
   (@type = "page-label") and normal children.

   or whatever type GtkNotebook uses for that...

- In general, the documentation needs some polish, but I guess
  we are not at the point to worry about that yet.

- Are some vfuncs mandatory to implement ? I don't see NULL
  checks for construct_child, custom_start_tag and custom_end_tag.
  Or rather, they have return_if_fail checks. 

- In gtk_builder_finalize:

+  g_hash_table_foreach (priv->objects, (GHFunc)g_free, NULL);
+  g_hash_table_destroy (priv->objects);

You could save that foreach if you used new_full to construct the hash table.

- The docs for gtk_builder_value_from_string_type refer to the 
  wrong function.

- In gtk_builder_value_from_string_type, I'm pretty sure you actually want
  to handle errors from strtol and friends. 

- Can we just move _gtk_{enum,flags}_from_string into gtkbuilder.c and
  make them static ? They seem to fit really well here. Or are they used
  somewhere else ?

- Do we want to make gtk_builder_parse_{filename,buffer} return a boolean
  indicating success, like many other GError-taking functions do ?

- gtk_builder_set_translation_domain() should be a regular setter
  for the domain property. Ie either change its name, or change the name
  of the property, and add change notification, and make set_property
  use the setter, perhaps.
  And where there is a setter, there shall also be a getter...


- Is there something missing here ? Other than a comment...
+  else if (!strcmp (element_name, "placeholder"))
+    {
+    }

 
- Do you want to pass G_MARKUP_TREAD_CDATA_AS_TEXT here ?
+  data->ctx = g_markup_parse_context_new (&parser, 0, data, NULL);

some initial comments on buildable-implementation v6:

- still doesn't make much sense to me to have actions and action groups
  defined by the xml, since they are supposed to correspond to the features
  exposed by your code...

- the names used for internal children and for slots to add children need to be 
  documented 

Some good ideas picked up from gtk-devel discussion:

- replace the GtkBuilder::finish signal with a
  GtkBuildable::parsing_finished vfunc

- add a GtkBuildable::set_child_property vfunc 

Created attachment 89239 [details] [review]
v7: typeutils

Do you want to add a way to add to the map in the xml ? 
Like 

<types>
  <type name="GtkUIManager" get_type_func="gtk_ui_manger_get_type"/>
  <type name="FooBar" get_type_func="FooBarTypeNum"/>
  ...

The parser would simply call gtk_register_type on the pairs.

(In reply to comment #58)
> Do you want to add a way to add to the map in the xml ? 
> Like 
> 
> <types>
>   <type name="GtkUIManager" get_type_func="gtk_ui_manger_get_type"/>
>   <type name="FooBar" get_type_func="FooBarTypeNum"/>
>   ...
> 
> The parser would simply call gtk_register_type on the pairs.
> 

That's an idea, it's not ideal to require that information to specified in all xml files though, but I guess acceptable until we have introspection.

Another idea, is to allow a symbol name to be specified instead of a type name when constructing an object:

either

  <object type="GtkUIManager:gtk_ui_manger_get_type">

which would mean, use GtkUIManager if registered otherwise call gtk_ui_manager_get_type to get the type.

or just

  <object type="gtk_ui_manger_get_type">

I'd rather add a separate attribute then

 <object type="GtkUIManager" get_type_func="gtk_ui_manager_get_type">

Since we are already dealing with xml, we can just as well let the xml parser
work for us rather then rolling an attribute-value-parser ourselves...

Yay, great to see this ball rolling... just adding a few fringe
comments...

(In reply to comment #54)
> comments on buildable v6.1:
[...]
> 
> - it also seems slightly odd that there is a gtk_builder_get_object_name.
>   why is that needed, in addition to gtk_buildable_get_name ?

Not every built object is GtkBuildable, only objects that require
special case code are GtkBuildable.

[...]
> - Can we just move _gtk_{enum,flags}_from_string into gtkbuilder.c and
>   make them static ? They seem to fit really well here. Or are they used
>   somewhere else ?

Those functions can be useful when custom parsing properties
in GtkBuildable code, for that reason I'd go so far as suggesting
we export those symbols for the sake of third party widgets.

> - Is there something missing here ? Other than a comment...
> +  else if (!strcmp (element_name, "placeholder"))
> +    {
> +    }

It prevents the parser code to fall into the
following "else choke_on_bad_xml()" clause (I did the same in
my patches to avoid choking on atk properties).

I wonder if it would be better policy to just ignore unrecognized
portions of the xml.

(In reply to comment #54)
> comments on buildable v6.1:
> 
> - could probably use g_type_register_static_simple()

Done, but it doesn't save too much..

> - why is gtk_buildable_set_name() always storing a copy of the
>   name as object data ? I think it would be cleaner to also
>   virtualize get_name, and then do away with the extra copy of the
>   name if the object already has a name property.

I can't remember the original reason, perhaps I just wanted to avoid
implementing get_name() in a couple of places.
Anyway I changed this and it looks a lot better.

> - it also seems slightly odd that there is a gtk_builder_get_object_name.
>   why is that needed, in addition to gtk_buildable_get_name ?

Can't remember, I removed it in favor of gtk_buildable_get_name.
Perhaps it was for libglade compatibility.

> - The docs for the type parameter of gtk_buildable_add() could be
>   improved. "how the child should be added" doesn't really say a lot.
>   How about "Some objects have different 'slots' in which children 
>   can be added. The @type parameter can be used to determine this 
>   slot." Or something to that effect. And then give a very
>   contrete example:
> 
>    #GtkNotebook uses the @type to distinguish between page labels
>    (@type = "page-label") and normal children.
> 
>    or whatever type GtkNotebook uses for that...
> 
> - In general, the documentation needs some polish, but I guess
>   we are not at the point to worry about that yet.

I skipped this for now, I'll go back to these items when the rest is
acceptable.

> - Are some vfuncs mandatory to implement ? I don't see NULL
>   checks for construct_child, custom_start_tag and custom_end_tag.
>   Or rather, they have return_if_fail checks. 

Depends.
They are not mandatory to implement unless you add custom tags
in your buildable XML definition.

> - In gtk_builder_finalize:
> 
> +  g_hash_table_foreach (priv->objects, (GHFunc)g_free, NULL);
> +  g_hash_table_destroy (priv->objects);
> 
> You could save that foreach if you used new_full to construct the hash table.

Look reasonable, done.

> - The docs for gtk_builder_value_from_string_type refer to the 
>   wrong function.

And the arguments were wrong too, updated.

> - In gtk_builder_value_from_string_type, I'm pretty sure you actually want
>   to handle errors from strtol and friends. 

Assume you also meant g_strtod (which is changed to g_ascii_strtod). Changed
to check for errno and print a warning for overflows and other errors.

> - Can we just move _gtk_{enum,flags}_from_string into gtkbuilder.c and
>   make them static ? They seem to fit really well here. Or are they used
>   somewhere else ?

Not quite, they're used by a few of the buildable implementations.
I added a gtk_builder prefer to them, made them public and moved them
into gtkbuildable.c

> - Do we want to make gtk_builder_parse_{filename,buffer} return a boolean
>   indicating success, like many other GError-taking functions do ?

Sure, not a bad idea at all.

> - gtk_builder_set_translation_domain() should be a regular setter
>   for the domain property. Ie either change its name, or change the name
>   of the property, and add change notification, and make set_property
>   use the setter, perhaps.
>   And where there is a setter, there shall also be a getter...

Agreed, I changed the property name to translation-domain, added a getter
and made gtk_builder_set_property call the setter.

> - Is there something missing here ? Other than a comment...
> +  else if (!strcmp (element_name, "placeholder"))
> +    {
> +    }

Tristian already pointed out why, I added a comment explaining.

> - Do you want to pass G_MARKUP_TREAD_CDATA_AS_TEXT here ?
> +  data->ctx = g_markup_parse_context_new (&parser, 0, data, NULL);

Right, that's probably a good idea too. Added.

(In reply to comment #56)
> Some good ideas picked up from gtk-devel discussion:
> 
> - replace the GtkBuilder::finish signal with a
>   GtkBuildable::parsing_finished vfunc

Good idea, it was a bit of work but it solves the bug Tristan pointed out.
Also resolved.

> - add a GtkBuildable::set_child_property vfunc 

I'm not so sure about this, since a set_child_property() would be specific to GtkContainer, should it really be exposed inside the API?
Also, it shouldn't really be necessary, there's no real need to override the way to set child properties is there?

Created attachment 89349 [details] [review]
v8: typeutils

Created attachment 89350 [details] [review]
v8: buildable

Created attachment 89351 [details] [review]
v8: buildable implementation

Created attachment 89352 [details] [review]
v8: tests

(In reply to comment #62)
[...]
> > - add a GtkBuildable::set_child_property vfunc 
> 
> I'm not so sure about this, since a set_child_property() would be specific to
> GtkContainer, should it really be exposed inside the API?
> Also, it shouldn't really be necessary, there's no real need to override the
> way to set child properties is there?

I disagree that child properties are limited to GtkContainer, as
soon as we enter the realm of building non widget objects in the
builder there becomes instances when a delagate object can
be parented by another object - these occurrences can as easily
have packing properties as widgets do in gtkcontainers.

As far as needing to override them from the buildable iface goes,
I think ideally we shouldnt need to override set_property()
or child_set_property(); maybe:
  - the object maybe should simply check if its being loaded at 
    set_[child_]property() time if theres any special code to write ?
  - objects could have a new property flag, along the lines of
    G_PARAM_CONSTRUCT, could be G_PARAM_BUILABLE[_ONLY] ?
  - the object's author should simply write code that works regardless
    of the context in which properties are being set ?

That idealistic notion aside, I think its important to not leave
packing properties out of the picture, so if there is a way to 
override how a property is set on an object, please let there
be a way to override how a packing property is set for a child
object.

One thing that is still unimplemented is unreffing of objects after the builder is finalized. I discussed this with James the last time he was in Brazil and left the following notes which I'm adding here so I won't lose them (which I almost did):

Builder should have two hash tables:
1: name -> object
2: object ->name.

There should be a weak ref handler on each object which does:

1. lookup object name in second hash
2. lookup object in first hash
3. If objects are the same, remove entry from 1st hash
4. remove entry from 2nd hash which frees the name

In the builders finalize, do the following:

1. g_hash_table_foreach_remove on 2nd hash table
   * remove reak ref from object
   * return True to remove entry from hash table
2. unref 1st and 2nd hash table

For GObjects where we own the reference return by g_object_new, add to a list

when building is finished unref each object in list.

should work for size groups, tree models, text buffers, but don't do this for GtkWindow, which is floating.

Small note on this patch: maybe it's better to move GtkBuilderPrivate *priv to the GtkBuilder structure and call GTK_BUILDER_GET_PRIVATE only once in the gtk_builder_init function, just like most of the widgets in gtk do.

> I disagree that child properties are limited to GtkContainer, as
> soon as we enter the realm of building non widget objects in the
> builder there becomes instances when a delagate object can
> be parented by another object - these occurrences can as easily
> have packing properties as widgets do in gtkcontainers.

A likely candidate for non-container child properties would be 
cell layout and cell renderers, though I don't think they really 
have any packing properties right now.

One good comment from recent mailing list discussion was to add a virtual 
function for type resolution, so that, e.g. gtkmm can override it. 

I'd also like to bring up the idea of an xml attribute to specify the get_type
function again. 

Ideally, I'd like the type registration stuff to be not exposed as public api 
at all. Do you think that is feasible when we have the heuristic name guessing 
and the get_type attribute ?

(In reply to comment #71)
> One good comment from recent mailing list discussion was to add a virtual 
> function for type resolution, so that, e.g. gtkmm can override it. 

This is rather ugly IMO, I'll talk to Murray first and see if it's possible to avoid it, if not I'll just port over the vtable func from libglade.

> I'd also like to bring up the idea of an xml attribute to specify the get_type
> function again. 

I've done this locally already. I need to test it first although. Consider it done.

> Ideally, I'd like the type registration stuff to be not exposed as public api 
> at all. Do you think that is feasible when we have the heuristic name guessing 
> and the get_type attribute ?

As mentioned on IRC:

<johan> mclasen: possibly
<mclasen> would be much nicer imo, if we could keep gtk out of the type registration business, and limp along with heuristics and xml attributes for now
<johan> agreed
<johan> it's fine assuming gobject-introspection will be ready in the not too distant future
<johan> there are workaround today aswell, such as forcing registration of these types before loading the xml
<mclasen> exactly. Also, I need to ask rambokid about fixing the g_type_from_name() segfault
<johan> another workaround is renaming the get_type functions to use compatible naming or adding an additional/alias

With GtkBuildable::parser_finished in place, do we still need GtkBuilder:finish ?

(In reply to comment #73)
> With GtkBuildable::parser_finished in place, do we still need GtkBuilder:finish
> ?
> 

It's still used for;
GtkWidget::has-default -> gtk_widget_grab_default()
GtkWidget::has-focus -> gtk_widget_grab_focus()
GtkWindow::visible -> gtk_widget_show()

All of these needs to be done after the whole interface is constructed.
For visible to avoid showing the construction of the interface until it is completed, for focus/default to be able to set them for the right widget.

Considering that ::finish is emitted before the parser_finished cbs are executed, you could just as well do this from parser_finished, no ?

Also, somebodys earlier comment about disconnecting the signal handlers still stands.

(In reply to comment #74)
> (In reply to comment #73)
> > With GtkBuildable::parser_finished in place, do we still need GtkBuilder:finish
> > ?
> > 
> 
> It's still used for;
> GtkWidget::has-default -> gtk_widget_grab_default()
> GtkWidget::has-focus -> gtk_widget_grab_focus()
> GtkWindow::visible -> gtk_widget_show()
> 
> All of these needs to be done after the whole interface is constructed.
> For visible to avoid showing the construction of the interface until it is
> completed, for focus/default to be able to set them for the right widget.
> 

If GtkWidget does those things for those properties and
handles GtkBuilder:finish, it would also have to unregister
to the GtkBuilder:finish callback afterwords.

Rather, I would have expected the GtkBuilder to emit the
buildable::parser_finished signal on all buildable objects 
after parsing the entire interface.

note also this removes the need for explicit signal connections
on a per property basis in gtkwidget.c afaics, just do the
appropriate activity inside the parser_finished() implementation
(i.e. grab_focus/grab_default/show if appropriate all in one
place).

(In reply to comment #75)
> Considering that ::finish is emitted before the parser_finished cbs are
> executed, you could just as well do this from parser_finished, no ?

(In reply to comment #76)
> Also, somebodys earlier comment about disconnecting the signal handlers still
> stands.

Doable yes, but I need to figure out how to persist data from custom_set_property() to parser_finished(). Perhaps qdata or priv is the right solution here.

(In reply to comment #71)
> One good comment from recent mailing list discussion was to add a virtual 
> function for type resolution, so that, e.g. gtkmm can override it. 
> 
> I'd also like to bring up the idea of an xml attribute to specify the get_type
> function again. 
> 
> Ideally, I'd like the type registration stuff to be not exposed as public api 
> at all. Do you think that is feasible when we have the heuristic name guessing 
> and the get_type attribute ?
> 

Personally I think that specifying the get_type() function is what
should be primarily relied upon, rather than opening up the can of worms
of making rules about proper naming conventions for registered types.

Maybe this would be an interesting approach:
   - Make a type resolver vfunc to GType get_type (char *type_name);
   - Default implementation assumes that "type_name" is the name
     of the function that returns the type ("foo_bar_get_type").
   - language bindings can override the vfunc and do whatever method
     they see fit, if a special type isnt found; chain up to the default
     implementation.

(In reply to comment #70)
> > I disagree that child properties are limited to GtkContainer, as
> > soon as we enter the realm of building non widget objects in the
> > builder there becomes instances when a delagate object can
> > be parented by another object - these occurrences can as easily
> > have packing properties as widgets do in gtkcontainers.
> 
> A likely candidate for non-container child properties would be 
> cell layout and cell renderers, though I don't think they really 
> have any packing properties right now.
> 

They aren't actually child properties in the sense that they have a pspec.
They're attributes which are mapped to real properties.

If we want to make something generic we'd have to avoid exposing pspecs.

Looking at the delayed properties implementation, I notice that it currently does not handle construct-only object properties. One such example is
GtkFileChooserButton::dialog. 

What should probably happen here is that the builder should pass construct-only object parameters into the constructor if the object has already been constructed,
and fail with a clear error otherwise. It will then be the responsibility of the ui builder to put objects in the xml in the right order.

(In reply to comment #80)
> > 
> > A likely candidate for non-container child properties would be 
> > cell layout and cell renderers, though I don't think they really 
> > have any packing properties right now.
> 
> They aren't actually child properties in the sense that they have a pspec.
> They're attributes which are mapped to real properties.
> 
> If we want to make something generic we'd have to avoid exposing pspecs.
> 

It might be a little late in the game but its really not a long
shot away to do GContainerIface, in which case packing properties
can be introspected with their pspecs and all (would just have
to be implemented by GtkContainer and accessed as GContainer
from the builder, any object at that point would be free to
implement GContainer and publish packing properties).

Warning: API nitpicks ahead (from the "user == developer" POV):

gtk_builder_parse_filename() should be renamed (it doesn't parse the file name)

Now, I'm thinking there are few options for this, the obvious one being *_parse_file().

GtkUIManager and loads of other API seem to use *_from_file() as the term for "load stuff from file" (which makes sense).

Also the word "parse" seems to be more description of what happens during the function, not of what the developer actually wants to happen. So perhaps "build" would be more accurate (though "builder_build" is a bit funny), or even go the UIManager path and make it gtk_builder_add_ui_from_file(). This would align nicely the two conceptually similar APIs.

"Add" would also support the fact that you actually can add UI multiple times to the same builder (is this "officially" supported btw?), barring the fact that if you call signal_autoconnect() multiple times (it registers multiple handlers). I'm sure that wouldn't be too hard to fix though. You likely never want to connect the same handler to the same object twice, right?

Another thing that should be considered is the parse_buffer(). I think for much the same reasons as above, this should be *_from_string(). Also for internal consistency since the flag, value and enum parsers are *_from_string() and operate on the same parameter type.

> It might be a little late in the game but its really not a long
> shot away to do GContainerIface, in which case packing properties
> can be introspected with their pspecs and all (would just have
> to be implemented by GtkContainer and accessed as GContainer
> from the builder, any object at that point would be free to
> implement GContainer and publish packing properties).

Maybe we are all on the same page here, but child properties can already be introspected with pspecs and everything from GtkContainer today.

Yes they can, its just that once the moment comes that child
properties are used for object children that are not GtkContainers,
then this cant be done (the patches I wrote on bug 383035 do
a special case exception "if (GTK_IS_CONTAINER())" to introspect
packing properties, if any other packing properties are found then
they are wrapped into a GParamSpecString and passed along without
any type conversions)

this would ofcourse work better if non GtkContainer parents were
also allowed to declare packing properties by implementing GContainer.

Okay, time to attach another round of patches, changes since the last revision:

* Rename parser_finished to custom_finished, this is called one time for each
  custom parser specified, this simplifies writing custom parsers which need to 
  have things done when parsing is finished (eg, sizegroup, dialog, treestore).
* Add parser_finished vfunc and removed GtkBuilder::finished signal
* Add tests for GtkWidget::has-default/has-focus
* Change custom_tag_start/end user_data argument to bea gpointer *, instead of gpointer.
* Add vfunc get_type_from_name, similar to GladeXML->lookup_func (for gtkmm)
* Add support for construct only object properties
* Add beginning of GError support, much more work will be done here for improved
  error reporting which will not add significant new API
* As suggested by Kalle, rename parse_filename to add_from_file and parse_buffer to add_from_string. While calling add_from_file is currently untested/undefined it is probably already working in some case, future work is required to test and define what is going to happen.
* get_type symbol name guessing heuristic is improved to handle all known types in Gtk+.
* gtk_type_register & type table is removed

Created attachment 89557 [details] [review]
v9: tests

Created attachment 89558 [details] [review]
v9: buildable

Created attachment 89559 [details] [review]
v9: buildable implementation

Created attachment 89560 [details] [review]
v9: typeutils

> * Change custom_tag_start/end user_data argument to bea gpointer *, instead of
> gpointer.

Hmm, how is this supposed to be used, and how does the memory management work ?
E.g. looking at 

+gtk_list_store_buildable_custom_tag_start
+gtk_list_store_buildable_custom_tag_end

I find it slightly confusing that the start function allocates the user_data,
but end does not free it.

The documentation for user_data is not really clarifying the situation:
+ * @data: user data that will be passed in to parser functions


> * gtk_type_register & type table is removed

With this gone, there is not much point in having gtk_registered_type_by_name()
public either. I'd really really like gtk to stay out of the type registration business 


> +      object_type = gtk_builder_get_type_from_name (builder, G_OBJECT_TYPE_NAME (object));

This looks fishy. If you already have an object, there is no need to go to such length to get its type...

(In reply to comment #91)
> > * Change custom_tag_start/end user_data argument to bea gpointer *, instead of
> > gpointer.
> 
> Hmm, how is this supposed to be used, and how does the memory management work ?
> E.g. looking at 
> 
> +gtk_list_store_buildable_custom_tag_start
> +gtk_list_store_buildable_custom_tag_end
> 
> I find it slightly confusing that the start function allocates the user_data,
> but end does not free it.
> 
> The documentation for user_data is not really clarifying the situation:
> + * @data: user data that will be passed in to parser functions

I can't remember of my head, but I think that's a leak in each case, the structures allocated in start should definitely be freed end.

The documentation has not really seen the love it needed in the last couple of rounds, it's all subject to change (or be written at all!).

> > * gtk_type_register & type table is removed
> 
> With this gone, there is not much point in having gtk_registered_type_by_name()
> public either. I'd really really like gtk to stay out of the type registration
> business 

It is just (very) badly named, the only thing it does now is guessing the name of the types corresponding *_get_type function if g_type_name returns G_TYPE_INVALID. Perhaps this should be moved to glib itself, I definitely think that there's a use case for this kind of function outside of the builder itself.

> > +      object_type = gtk_builder_get_type_from_name (builder, G_OBJECT_TYPE_NAME (object));
> 
> This looks fishy. If you already have an object, there is no need to go to such
> length to get its type...

gtk_builder_get_type_from_name is used to be able to support overloading of types which apparently is required by gtkmm, I replaced all g_type_name with that call for consistency.

> It is just (very) badly named, the only thing it does now is guessing the name
> of the types corresponding *_get_type function if g_type_name returns
> G_TYPE_INVALID. Perhaps this should be moved to glib itself, I definitely think
> that there's a use case for this kind of function outside of the builder
> itself.

I know that it does that, I just wonder if we can avoid exporting this function from gtk. If there is a use case for this function, then let it be reported separately and lets have the function added to gobject. If we don't export it from gtk at this point, we can then later simply switch to using the glib function.
If we export it from gtk, we're stuck with it forever, and the best we can do is deprecate it. 


> gtk_builder_get_type_from_name is used to be able to support overloading of
> types which apparently is required by gtkmm, I replaced all g_type_name with
> that call for consistency.

Well, look at the code. You have an object, and you want a type. 
What is wrong with simply using G_OBJECT_TYPE (object) there ?
 

Created attachment 89600 [details] [review]
Patch to make gtk_builder_signal_autoconnect() reject double handlers

(In reply to comment #83)
> "Add" would also support the fact that you actually can add UI multiple times
> to the same builder (is this "officially" supported btw?), barring the fact
> that if you call signal_autoconnect() multiple times (it registers multiple
> handlers). I'm sure that wouldn't be too hard to fix though. You likely never
> want to connect the same handler to the same object twice, right?
[...]
(In reply to comment #86)
> * As suggested by Kalle, rename parse_filename to add_from_file and
> parse_buffer to add_from_string. While calling add_from_file is currently
> untested/undefined it is probably already working in some case, future work is
> required to test and define what is going to happen.

Well, at least for the simple case of having two toplevel windows in different xml:s and reusing the same handler in both works right with the attached patch (ie. doesn't connect the callback twice to the objects in the last loaded UI).

It does this by looking up the signal id for the type and searches for a registered handler for that id with the same callback. If it finds it, it'll complain (though this is mostly for testing) and refuse to connect it.

My test case included a duplication of the demo.glade in tests, an extra callback for the about dialog (which is connected only to the about action in the duplicated .glade) in builderdemo.c as well as duplicating the loading portion there (except the creation of the builder object of course).

So the duplicated about action should fire two handlers, and the original just one. Before the patch all three defined handlers are fired on activation of the duplicated about item, with the patch only the ones that are uniquely defined to that object (this also rejects double handlers if you specifically ask for it in the .glade).

Does this look/sound right?

(In reply to comment #93)
> > It is just (very) badly named, the only thing it does now is guessing the name
> > of the types corresponding *_get_type function if g_type_name returns
> > G_TYPE_INVALID. Perhaps this should be moved to glib itself, I definitely think
> > that there's a use case for this kind of function outside of the builder
> > itself.
> 
> I know that it does that, I just wonder if we can avoid exporting this function
> from gtk. If there is a use case for this function, then let it be reported
> separately and lets have the function added to gobject. If we don't export it
> from gtk at this point, we can then later simply switch to using the glib
> function.
> If we export it from gtk, we're stuck with it forever, and the best we can do
> is deprecate it. 

Right, I see your point. I'll open up a separate bug against glib, make it private and move it to gtk_builder_get_type_from_name, the only call site left.

> > gtk_builder_get_type_from_name is used to be able to support overloading of
> > types which apparently is required by gtkmm, I replaced all g_type_name with
> > that call for consistency.
> 
> Well, look at the code. You have an object, and you want a type. 
> What is wrong with simply using G_OBJECT_TYPE (object) there ?

Oh sorry, I misunderstood, I thought you were pointing at similar code at another place. You're right of course, it should just use G_OBJECT_TYPE.

(In reply to comment #94)
[..]
> My test case included a duplication of the demo.glade in tests, an extra
> callback for the about dialog (which is connected only to the about action in
> the duplicated .glade) in builderdemo.c as well as duplicating the loading
> portion there (except the creation of the builder object of course).

Is that really a valid use case, why would you want to parse an identical file twice?

(In reply to comment #92)
> (In reply to comment #91)
> > > * Change custom_tag_start/end user_data argument to bea gpointer *, instead of
> > > gpointer.
> > 
> > Hmm, how is this supposed to be used, and how does the memory management work ?
> > E.g. looking at 
> > 
> > +gtk_list_store_buildable_custom_tag_start
> > +gtk_list_store_buildable_custom_tag_end
> > 
> > I find it slightly confusing that the start function allocates the user_data,
> > but end does not free it.
> > 
> > The documentation for user_data is not really clarifying the situation:
> > + * @data: user data that will be passed in to parser functions
> 
> I can't remember of my head, but I think that's a leak in each case, the
> structures allocated in start should definitely be freed end.

Actually, after looking at the code I found out that the following is the case:
In most cases the data is not freed in custom_tag_end because it maybe reference other widgets not yet constructed, so it needs to wait until custom_finished until it can actually free all the data structures used.

In some cases it can already free them in custom_tag_end, like the liststore and it should probably be done there (instead of the GMarkupParsers element_end) for clarity.

(In reply to comment #96)
> (In reply to comment #94)
> [..]
> > My test case included a duplication of the demo.glade in tests, an extra
> > callback for the about dialog (which is connected only to the about action in
> > the duplicated .glade) in builderdemo.c as well as duplicating the loading
> > portion there (except the creation of the builder object of course).
> 
> Is that really a valid use case, why would you want to parse an identical file
> twice?
 
If you would do such outrageous thing like dynamically generating the UI description, or build multiple descriptions from an "outside source", ensuring that every widget is named uniquely would be tedious. As long as you don't want to look it up by name, you should be fine using callbacks even if it isn't uniquely named right?.

I could imagine wanting to load the same glade multiple times in multiwindowed applications too.

That said, all of this can of course be achieved by instantiating a builder object per file/string (without the drawback of not being able to look the objects up). I guess the builder object doesn't have such overhead that it would matter, does it?

The patch should also prevent multiple callback connecting within one file however, which I think would be worth it even if loading multiple non-unique descriptions isn't going to be supported.

I don't think the case of multiple files with conflicting ids is worth supporting.
Nonconflicting ids are only on sed call away...

I also don't think doing extra work to reject duplicate signal handlers is worth it. If this was a problem, GUI builders can easily enforce the "only one signal handler" rule.

(In reply to comment #99)
> I don't think the case of multiple files with conflicting ids is worth
> supporting.
> Nonconflicting ids are only on sed call away...

Fair enough.
 
> I also don't think doing extra work to reject duplicate signal handlers is
> worth it. If this was a problem, GUI builders can easily enforce the "only one
> signal handler" rule.

I'm more concerned about the "only one invocation of signal_autoconnect() on one instance of gtkbuilder during it's lifetime" restriction this imposes (unless fixed elsewhere).

If you require that, there's no point in providing it as separate API call, it could be done implicitly after the build process. And if every UI description needs a GtkBuilder instance, maybe the API should relfect that and offer new_form_*() constructors instead of the parse API and autoconnect implicitly (though I guess this would be bad for bindings?).

It's not just about duplicate ID's you see, if you load your main UI and reuse the builder for a dialog .glade it will reconnect the signals in the main UI. Now, either this could be fixed by flagging the signal info with "already did that" or by the bit more generic way that my patch does, check for the handler in the object instance.

In my opinion, limiting the builder instances to one description sounds like more work for the developer since he has to manage builders for each .glade he/she wants to use. Keeping all dialogs etc in the same .glade sounds like a big startup hit for the embedded case.

Hmm, good point. 

Is there any argument for having signal_autoconnect() as explicit api, Johan ?
It sounds nice to me to implicitly do the autoconnect when the parser finishes, 
and then only do the signals that were set up during the current parse. 

Would that work ?

(In reply to comment #101)
> Hmm, good point. 
> 
> Is there any argument for having signal_autoconnect() as explicit api, Johan ?
> It sounds nice to me to implicitly do the autoconnect when the parser finishes, 
> and then only do the signals that were set up during the current parse. 
> 
> Would that work ?
> 

It's easier to change signal_autoconnect to behave in the following way:
Connect all knowns signals free them afterwards, that would make signal_autoconnect() "destructive", you won't get the same behavior the second time you call it, that's probably not a big deal, since you normally want to connect a signal only once, as Kalle pointed out. But it would solve the problem he pointed out and make the following work:

  add_from_string(xml)
  add_from_string(additional_xml)
  signal_autoconnect()
  add_from_string(some_more_xml)
  signal_autoconnect()

In this example the first call to signal_autoconncet() would connect all the signals in the xml and additional_xml definitions and the second call only the ones found in some_more_xml.

That seems like the expected behavior, right?

Created attachment 89758 [details] [review]
v10: all in one

This fixes the signal_autoconnect issue in the way mentioned in comment 102, it also adds a test for this.

In what sense is it all-in-one ? it seems to miss at least gtkbuildable.[hc] ?

(In reply to comment #69)
> Small note on this patch: maybe it's better to move GtkBuilderPrivate *priv to
> the GtkBuilder structure and call GTK_BUILDER_GET_PRIVATE only once in the
> gtk_builder_init function, just like most of the widgets in gtk do.
> 

There seems to be different, I basically copied what gtkbutton.c did.
Perhaps it's just because the GtkButton structure is already filled that they didn't put the priv inside the structure.

(In reply to comment #104)
> In what sense is it all-in-one ? it seems to miss at least gtkbuildable.[hc] ?
> 

They are present, however towards the end of the diff, svn diff does not sort the output by filename.

Wrt. to priv pointers, for gtkbutton.c there was indeed no room for a priv pointer, but for a new structure, we should add one.

Small stylistic thing:

+  g_return_val_if_fail (GTK_IS_BUILDABLE (buildable), NULL);
+  g_return_val_if_fail (GTK_IS_BUILDER (builder), NULL);
+  g_return_val_if_fail (childname != NULL, NULL);
+
+  iface = GTK_BUILDABLE_GET_IFACE (buildable);
+
+  g_return_val_if_fail (iface != NULL, NULL);

since you already check GTK_IS_BUILDABLE, the iface != NULL check
seems redundant. We don't do that in other places in GTK+ that involve
interfaces.

gtk_buildable_custom_tag_start and
gtk_buildable_custom_tag_end handle this inconsistently. the start method has

+  g_return_val_if_fail (iface->custom_tag_start != NULL, FALSE);

the end method just silently does nothing.

(In reply to comment #109)
> gtk_buildable_custom_tag_start and
> gtk_buildable_custom_tag_end handle this inconsistently. the start method has
> 
> +  g_return_val_if_fail (iface->custom_tag_start != NULL, FALSE);
> 
> the end method just silently does nothing.
> 

They are actually different, a start should always be implemented, end doesn't need to since, freeing the data structure in custom_finished is also a valid use case. SizeGroup does this for example.

(In reply to comment #108)
> Small stylistic thing:
> 
> +  g_return_val_if_fail (GTK_IS_BUILDABLE (buildable), NULL);
> +  g_return_val_if_fail (GTK_IS_BUILDER (builder), NULL);
> +  g_return_val_if_fail (childname != NULL, NULL);
> +
> +  iface = GTK_BUILDABLE_GET_IFACE (buildable);
> +
> +  g_return_val_if_fail (iface != NULL, NULL);
> 
> since you already check GTK_IS_BUILDABLE, the iface != NULL check
> seems redundant. We don't do that in other places in GTK+ that involve
> interfaces.

Good point, this has been fixed. I also found out that I missed g_return_if_fail in a couple of public APIs.

(In reply to comment #107)
> Wrt. to priv pointers, for gtkbutton.c there was indeed no room for a priv
> pointer, but for a new structure, we should add one.
> 

Right, I updated this, it's actually much nicer.

Wrt. to signal autoconnection:

> That seems like the expected behavior, right?

Yes, sounds fine. I don't think the destructive nature of autoconnect
is a big deal, it just needs to be documented clearly.

Created attachment 89769 [details] [review]
v11: all in one

Sorted patch this time, gtk/gtkbuild* are in the beginning.
Changes:
* Make sure that the buildertest runs without leaks, free the list of construct only properties, free the domain, unref all builders & free all lists in buildertest.
* Rename root element from glade-interface to interface
* Remove g_return_if_fail(iface) calls inside GtkBuildable interface
* Implement signal_autoconnect using signal_autoconnect_full to avoid code duplication
* Add private structure to GtkBuilder
* Remove reference to Gazpacho DTD in example

Created attachment 89782 [details] [review]
v12: all in one

Changes:
* Rewrote most docstrings, cross references, examples. Still missing a complete DTD/RelaxNG definition + description of the XML format.
* ensure gtypes of flags/enum functions are correct
* Rename signal_autoconnect* -> connect_signals*
* Avoid an g_strdup() of the translation domain.

Thanks, starting to look good; although the documentation needs a lot more work.

What is the status of comment 68 ? That still needs doing, right ?

Error handling in general needs a careful look. It looks like there are a
lot of places where you simply use g_warning() for things which should better
be passed back up.


Cosmetics:

- Please use GTK_PARAM_READWRITE instead of G_PARAM_READWRITE

- GTK+ codingstyle asks for parameters to be aligned in functions like

static void
+gtk_builder_get_parameters (GtkBuilder *builder,
+                            GType object_type,
+                            const gchar *object_name,
+                            GSList *properties,
+                            GArray **parameters,
+                            GArray **construct_parameters)



+          g_print ("Unknown property: %s.%s\n",
+                   g_type_name (object_type), prop->name);

You probably want to use g_warning here, like the rest of the function.


+      if (gtk_debug_flags & GTK_DEBUG_BUILDER)
+        {
+          gchar *str = g_strdup_value_contents ((const GValue*)&param->value);
+          g_print ("set %s: %s = %s\n", info->id, param->name, str);
+          g_free (str);
+        }

Please include debug sections like this in 
#ifdef G_ENABLE_DEBUG
#endif


+  if (builder->priv->delayed_properties)
+    {
+      g_hash_table_foreach (builder->priv->delayed_properties,
+                            (GHFunc)apply_delayed_properties, builder);
+    }

GTK+ coding style calls for no {} here.


 * Returns: 1 on success, 0 if an error occurred

Please make gtk_builder_add_from_file/string return gboolean


+  if (tmp_error != NULL) {

Misplaced {


+  *list = g_slist_append (*list, object);

Might want to do the usual prepend/reverse thing here.


+void
+gtk_builder_set_translation_domain (GtkBuilder   *builder,
+                                    const gchar  *domain)

I wonder if we want to do the same set_translation_domain/set_translate_func
thing we do for action groups. Probably not.


+  if (!g_module_close (module))
+    g_warning ("failed to close GModule.");

That warning seems pointless; we don't check the return value of
any other module_close call in GTK+


+      else if (!strcmp (names[i], "translatable"))

I prefer "== 0" over "!" for string comparisons


+  g_assert (object != NULL);
+  g_assert (G_IS_OBJECT (object));

The first assertion is redundant, since G_IS_OBJECT checks for NULL


+#ifdef GTK_ENABLE_DEBUG

I believe this needs to be G_ENABLE_DEBUG


+        data->subparser->start = element_name;

Do you need a strdup() here ?


+    g_warning ("Invalid type specified: `%s'", type);

We could add a macro for this, similar to GTK_CONTAINER_WARN_INVALID_CHILD_PROPERTY_ID

I see 3 major things that still need work:

- documentation
- error handling
- reference handling (comment 68)

(In reply to comment #115)
> I see 3 major things that still need work:

Agreed;

> 
> - documentation

This ncluding XML spec,
I could probably use some help/direction here, I'm quite blinded by the API by this point.

> - error handling

GError-ify most g_warnings inside gtkbuilderparser.c

> - reference handling (comment 68)

Right, still needs to be done.
Windows and the normal Widget hierarchy are pretty much working here afaik, the problem is GObjects and objects referencing them. 

I'd also like to add this;

- pixbuf and path manipulation, eg, load pixbufs from this path(s), similar to icon theme, can probably just copy a chunk of code from there.

(In reply to comment #114)
> Thanks, starting to look good; although the documentation needs a lot more
> work.
> 
> What is the status of comment 68 ? That still needs doing, right ?
> 
> Error handling in general needs a careful look. It looks like there are a
> lot of places where you simply use g_warning() for things which should better
> be passed back up.

These two items are on the TODO list, see comment 116

> Cosmetics:

>  * Returns: 1 on success, 0 if an error occurred
> 
> Please make gtk_builder_add_from_file/string return gboolean

I changed this to allow the functions to return a id which could be used to unmerge/remove the ui in the future. I will change the docstring to say that a positive value is considered success.

> +void
> +gtk_builder_set_translation_domain (GtkBuilder   *builder,
> +                                    const gchar  *domain)
> 
> I wonder if we want to do the same set_translation_domain/set_translate_func
> thing we do for action groups. Probably not.

I'm not so sure about the use case for set_translate_func, but it's possible to add this later on if someone asks for it.

> +        data->subparser->start = element_name;
> 
> Do you need a strdup() here ?

Not really, the start element name is only used during the lifetime of the GMarkupContext and is not exported, so it should be fine.

Created attachment 89815 [details] [review]
v13: all in one

Changes:
* !strmp() -> strcmp() == 0
* indentation, if/for {}, some print -> warnings, remove asserts
* Add G_ENABLE_DEBUG around GTK_DEBUG_BUILDER sections
* Small add_from_string/file documentation changes
* GTK_BUILDER_WARN_INVALID_CHILD_TYPE macro

Created attachment 89830 [details] [review]
v14: all in one

Changes:
* Refactored parser into a few more functions.
* Much improved parser error handling, including tests

Created attachment 89950 [details] [review]
v15: all in one

Changes:
* Move builderdemo into gtk-demo
* Add user_data to connect_signals after suggestion by Christian Perch
* Add swapped to <signal> tag
* Remove after arugment in GtkSignalConnectFunc
* Fix typos in documentation and enum
* Remove noop buildable implementation for cell renderer

Commited to trunk!

Sending       ChangeLog
Sending       demos/gtk-demo/Makefile.am
Adding    demos/gtk-demo/builder.c
Adding    demos/gtk-demo/demo.ui
Sending       docs/reference/gtk/gtk-docs.sgml
Sending       docs/reference/gtk/gtk-sections.txt
Sending       docs/reference/gtk/gtk.types
Adding    docs/reference/gtk/tmpl/gtkbuildable.sgml
Adding    docs/reference/gtk/tmpl/gtkbuilder.sgml
Sending       gtk/Makefile.am
Sending       gtk/gtk.h
Sending       gtk/gtk.symbols
Sending       gtk/gtkaction.c
Sending       gtk/gtkactiongroup.c
Adding    gtk/gtkbuildable.c
Adding    gtk/gtkbuildable.h
Adding    gtk/gtkbuilder.c
Adding    gtk/gtkbuilder.h
Adding    gtk/gtkbuilderparser.c
Adding    gtk/gtkbuilderprivate.h
Sending       gtk/gtkcelllayout.c
Sending       gtk/gtkcelllayout.h
Sending       gtk/gtkcellview.c
Sending       gtk/gtkcolorseldialog.c
Sending       gtk/gtkcombobox.c
Sending       gtk/gtkcomboboxentry.c
Sending       gtk/gtkcontainer.c
Sending       gtk/gtkdebug.h
Sending       gtk/gtkdialog.c
Sending       gtk/gtkentrycompletion.c
Sending       gtk/gtkexpander.c
Sending       gtk/gtkfontsel.c
Sending       gtk/gtkframe.c
Sending       gtk/gtkiconview.c
Sending       gtk/gtkliststore.c
Sending       gtk/gtkmain.c
Sending       gtk/gtknotebook.c
Sending       gtk/gtksizegroup.c
Sending       gtk/gtktreestore.c
Sending       gtk/gtktreeview.c
Sending       gtk/gtktreeviewcolumn.c
Sending       gtk/gtkuimanager.c
Sending       gtk/gtkwidget.c
Sending       gtk/gtkwindow.c
Sending       tests
Sending       tests/Makefile.am
Adding    tests/buildertest.c
Transmitting data ..............................................
Committed revision 18141.

Thanks everybody who helped reviewing!