Re: GtkBuilder report

[Tristan Van Berkom <tvb gnome org>]

On Tue, 2007-01-23 at 13:21 -0200, Johan Dahlin wrote:
> 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?

specifically, I would like to see the meanings of each vfunc described
in the buildable iface, so that buildable implementors have an easy
time understanding the design.

> Can you help out and provide a hand to make this effort going?

Ofcourse, I'll do anything to get builder into 2.12 ;-)

> 
> >   - 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.

I've worked with crappy parser errors before, hand-editing ~2000 ui
description files and saving and testing one line at a time to be sure
what part of the edit broke the build because the parser wouldnt tell
me - people will find new and interesting ways to create glade files,
people will edit them by hand, and they will have a right to complain
when the application says "sorry bad glade file" instead of something
usefull, definitly consider this a blocker.

[...]
> >   - 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

Maybe I'm wrong, but I've been hearing bug reports of people
using UIManager & glade generated accelerators inside the same
toplevel, usually the glade accelerators arent working, again,
sorry if my information is inacurate on this front, I'll dig a
little deeper for this one.

> 
> > 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?

Good point.

[...]
> >   - 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.

Check - shouldnt be too hard to do.

> 
> >   - 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.

Wholeheartedly disagree ;-) - IMO this is quite incorrect.

GtkContainer is not the only object able to parent another object, and
forcing non-container objects that support children to use 50 lines
of GMarkup for the purpose, is just not practicle (not to mention
handling the set_child_property() and then chaining up to the
container buildable's set_child_property() is very very difficult
to do)

If you look into my patches you'll also see a big fat comment about
introspected packing properties, currently only GtkContainer packing
properties are introspected - I suggest we proxy the GtkContainer api
through a more generic GContainer interface to fix this (this would
also be trivial to effect as a change - it would only require
a larger consensus).

Cheers,
                     -Tristan