Re: GtkBuilder report

[Johan Dahlin <jdahlin async com br>]

Hi Tristan

Tristan Van Berkom wrote:
> Hi all,
>    As we discussed in the last meeting - here is my evaluation the 
> state of affairs on the gtkbuilder front, it mostly includes all the
> stuff I think is essentially needed before the code is production ready,
> and a few unclear points I think need to be discussed before 
> finishing up.

Sorry for not being able to reply earlier or attend the meeting.

> Showstoppers:
> =============
> 
>   - Documentation, not all the gtk-doc statements are complete - and I
>     think in an depth description will be in order for the GtkBuildable
>     interface

Which other parts are incomplete, could you provide a list of interfaces
you'd like to have documented.
GtkBuildable is mainly interesting for third party application developers
which would like to make their widgets supported by the GtkBuilder.
What kind of documentation would you like to see here?
Can you help out and provide a hand to make this effort going?

>   - Parse errors should be reported nicely; currently if your glade file
>     is malformed - you'll get some assertion failure like "assertion
>     object_info != NULL failed".
> 
>     Some work has been done already to improve this - by looking at the
>     error_missing_attribute() function in gtkbuilderparser.c - looks
>     like these assertions can at least be replaced with some more
>     informative output.

Not really a blocker IMO.
It's easier to add error handling code when you find broken glade files or
are adopting an existing program to output something GtkBuilder can parse.
libglade does well here, so we should definitely improve this at some point.

>   - The GtkBuilder::finish signal is flawed, the builder supports
>     multiple parses and buildable objects connect to this signal to know
>     when the parse is over (more elegant than using an idle handler).
>     The bug here is that GtkBuildable objects that were parsed by the
>     builder do not disconnect thier handlers after running the signal
>     callback once - those handlers will be called again when the same
>     GtkBuilder parses a separate hierarchy - causing random side 
>     effects and; if the previous hierarchy is destroyed at this point -
>     segfaults.
> 
>     I'd propose to add a ->parse_finished vfunc to the GtkBuildable
>     interface that would be called on all objects after the parse (in
>     the builder "finish" class handler) - this would eliminate the 
>     bug and provide a nice api to the buildable objects (i.e. no
>     connecting and disconnecting and recording signal ids).

Good point.
That'd probably make it a bit easier to do this kind of things and avoid the
disconnect() as you pointed out.

>   - Atk properties
>     These are currently completely unsupported - supporting atk
>     properties for me personally would represent an evening of serious
>     work (maybe more because I always get lazy when it comes to the 
>     atk stuff - just seems like I'm writing code that noone will
>     ever use)... thoughts ? is this really a showstopper ?

Right.
The main reason I didn't implement the atk tags (atkproperty, atkaction,
atkrelation ad accessibility) was that I couldn't find any example glade
files to use as a reference from GNOME cvs.

A quick web search reveals that there are quite a few that uses them.
However, I'm not sure the DTD used by libglade is good enough, someone with
more knowledge of accessibility would have to help me here.

>   - Accelerators
>     Accelerators built directly from the glade file are in conflict with
>     those build by GtkUIManager & GtkActions - some code is needed to
>     make them share the GtkAccelGroup on the toplevel concerned... is
>     this really a showstopper ?

I don't see any conflicts there.
GtkBuilder currently only supports accelerators on GtkWidgets.
There's currently no way to set the accelerators on GtkAction's through
GtkBuilder since Gtk+ would need to support the GtkAction::accelerator
property first, see http://bugzilla.gnome.org/show_bug.cgi?id=172536

> Unclear:
> ========
> 
>   - abort()ing on bad glade files
> 
>     In my opinion, we shouldnt abort just because a glade file was
>     malformed (we dont abort on corrupt jpegs afaik) - so what is 
>     the alternative ?
> 
>     How about printing an error message about the parse failure/reason
>     and then destroying the hierarchy that was "created thus far" ? 

Isn't GError prefered?

>   - Depricated widgets
> 
>     If people think that we should explicitly discourage the use of
>     depricated widgets by unsupporting them - I wont disagree (not going
>     to argument on that one) - I would still like to propose that we
>     support the old fashioned libglade style of glade file for the
>     widgets that we do decide to support.

I'd be happier if we just didn't support them at all.
We should not encourage the use of unmaintained parts of a library for
newly written code.

>   - Object References & parsing of subhierarchies
> 
>     Libglade allowed the user to provide a "root" argument to
>     glade_xml_new(), the builder doesnt do anything like that.
> 
>     Is this a requirement ? if so - we'll need to make that argument a
>     list or an array, since some widgets will now be makeing references
>     to objects that stand outside the widget hierarchy - thos referenced
>     objects would also have to be parsed and available (such as
>     GdkPixbuf, GtkSizeGroup, GtkAction, GtkUIManager etc).
> 
>     We could easily steer clear of this feature at least for the initial
>     release...

This is probably not too difficult to implement. I'm not sure how widely it
is used by application developers.

>   - GdkPixbuf cant be buildable
> 
>     I remember discussing this with Johan before gtk+ 2.10 came out...
>     Some hack in the gtkbuilder code will have to exist in order to load
>     pixbuf objects properly (since they need some custom code for
>     loading purposes) - this can be avoided by moving GtkBuildable -->
>     GBuildable and implementing this code on the pixbuf object itself.

Ideally I'd like to support the filename property on GdkPixbuf, but it seems
that would be rather difficult to do because of the design of the GdkPixbuf
library and its loaders.
We'd need to make g_object_new(GDK_TYPE_BUILDABLE, "filename", ...) work.

There's also the issue that the buildable interface is present in the libgtk
library which libgdk_pixbuf is not linked against.
It should ideally be moved into gobject itself to avoid this issue.

>   - Packing properties are only introspected on GtkContainer objects
> 
>     Currently only GtkContainer derived objects can have thier packing
>     properties recognized and introspected - any GObject that parents
>     another GObject delagate must explicitly implement GtkBuildable and
>     handle packing properties manually.
> 
>     I'd propose this (yeah it sounds wild but its really simple):
>       - Create GContainerIface with add/remove get/set_child_property
>       - Implement GContainerIface on GtkContainer itself
>       - use the GContainerIface api from the builder

The current implementation is correct here.
Packing properties as you refer to is something which is specific to the
GtkContainer widget. If you want to support similar properties for your own
type you'd need to end up duplicating large chunks of GtkContainer anyway,
(child_set_property, child_get_property etc), adding support for
GtkBuildable on top of that is a small task.

Thanks for the comments.

Johan