After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 447998 - GtkBuilder does not support building parts of the xml tree
GtkBuilder does not support building parts of the xml tree
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkBuilder
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: GtkBuilder maintainers
GtkBuilder maintainers
: 531415 535971 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-06-15 19:54 UTC by Kalle Vahlman
Modified: 2009-03-17 15:51 UTC
See Also:
GNOME target: ---
GNOME version: 2.19/2.20


Attachments
A preliminary implementation of constructing only a part of the xml tree (5.23 KB, patch)
2007-06-17 12:19 UTC, Kalle Vahlman
none Details | Review
Few changes to gtk-demo for testing subtree building (2.72 KB, patch)
2007-06-17 12:25 UTC, Kalle Vahlman
none Details | Review
patch (17.30 KB, patch)
2008-06-02 10:34 UTC, Paolo Borelli
none Details | Review
patch (16.42 KB, patch)
2008-07-16 08:11 UTC, Paolo Borelli
reviewed Details | Review

Description Kalle Vahlman 2007-06-15 19:54:30 UTC
Libglade has a root argument in the constructor of GladeXML objects, which can be used to build only a certain part of the widget tree from the given UI description.

GtkBuilder currently lacks API to do this.

Choices that seem obvious are:

 - Adding root parameter to _add_from_*()
 - Adding variants: _add_from_*_with_root() or _add_subtree_from_*()

The former would be consistent with libglade, but would need to be implemented ASAP to avoid API breakage. The latter options could be added later (though this is pretty heavily used feature so it would need to be solved soon either way).
Comment 1 Johan (not receiving bugmail) Dahlin 2007-06-15 19:57:49 UTC
I wonder if a simple setter would be enough;

- set_root(const char* root) 

Which would restrict construction of all interfaces to that object name?
Comment 2 Kalle Vahlman 2007-06-17 12:19:11 UTC
Created attachment 90136 [details] [review]
A preliminary implementation of constructing only a part of the xml tree

So, after reviewing the parsing process a bit it seems that due to the extensibility of the XML and object references this isn't going to happen without sacrifices.

What the patch does is simply carry a flag indicating if the current tag is inside the intended build tree and skipping object construction when not. Parsing of signals, properties and (obviously) children are not skipped (though I guess the former two could be). The patch doesn't introduce an API yet, but instead looks for GTK_BUILDER_BUILD_ROOT environment variable to allow testing.

This means that since we don't instantiate the objects, we cannot call the custom tag parsing methods either. This is the first limitation with building just a subtree:
  You cannot build a subtree contained inside a custom tag. Also as a side-effect, the custom tags won't be validated.


The second, perhaps more troublesome but I guess expected limitation is object references:
  You cannot build a subtree that references objects outside the subtree you are building. Most notably this means no UIManager-constructed widgets in the subtree.

But other than those two limitations, it seems to be working.
Comment 3 Kalle Vahlman 2007-06-17 12:25:20 UTC
Created attachment 90137 [details] [review]
Few changes to gtk-demo for testing subtree building

This patch adds two extra windows to the demo.ui and code to search for a valid window in builder.c.

With the this and the earlier parser patch you can run gtk-demo like this:

  GTK_BUILDER_BUILD_ROOT=<id> ./gtk-demo

where <id> is one of window2, window3 or button1 to get only the window subtrees or just one button to be constructed and shown (note that due to the limitations mentioned, using window1 as build root won't work as it references the UIManager object for the menubar and toolbar).
Comment 4 Kalle Vahlman 2007-06-17 12:43:58 UTC
Now, as for the actual API, I'm leaning towards _add_subtree_from_*():

 - It doesn't introduce a parameter which will most of the time be NULL
 - It separates the full parsing a bit from the subtree parsing, as these have different set of rules (the limitations of subtree parsing mentioned earlier)
 - I don't like the "global" option (set_root()) as it is conceptually a setting for the parsing, not the builder itself. And it would require developers to re-set the root between loading multiple files (yes, I'm still keen on supporting that ;) and thus would be easy to produce confusion if it accidentally isn't reset (and suddenly you just don't find your widgets).

So I propose adding the following API:

guint        gtk_builder_add_subtree_from_file   (GtkBuilder    *builder,
                                                  const gchar   *filename,
                                                  const gchar   *root_id,
                                                  GError       **error);
guint        gtk_builder_add_subtree_from_string (GtkBuilder    *builder,
                                                  const gchar   *buffer,
                                                  gsize          length,
                                                  const gchar   *root_id,
                                                  GError       **error);

This would separate the subtree parsing while still being familiar from the glade API. Thoughts? Do we want to take this to devel-list for discussion?
Comment 5 Johan (not receiving bugmail) Dahlin 2007-06-17 12:55:07 UTC
(In reply to comment #2)
> Created an attachment (id=90136) [edit]
> A preliminary implementation of constructing only a part of the xml tree

Thanks for the patch, in general it looks good but;

> What the patch does is simply carry a flag indicating if the current tag is
> inside the intended build tree and skipping object construction when not.
> Parsing of signals, properties and (obviously) children are not skipped 

I don't like the amount of changes this patch makes, is it not possible to modify start/end_element a little bit less?

> The second, perhaps more troublesome but I guess expected limitation is object
> references:
>   You cannot build a subtree that references objects outside the subtree you
> are building. Most notably this means no UIManager-constructed widgets in the
> subtree.

Thats unfortunate and makes this feature less useful now when we support object references, which libglade didn't.

I'm almost leaning towards not supporting this feature because of that limitation, If it won't be possible to create menubars, toolbars, treemodels, sizegroups why should it be supported at all?

(In reply to comment #3)
> Created an attachment (id=90137) [edit]
> Few changes to gtk-demo for testing subtree building

This should really go into tests/buildertest.c, but I guess we're not quite at that point yet.

(In reply to comment #4)
> Now, as for the actual API, I'm leaning towards _add_subtree_from_*():
> 
>  - It doesn't introduce a parameter which will most of the time be NULL

> This would separate the subtree parsing while still being familiar from the
> glade API. Thoughts? Do we want to take this to devel-list for discussion?

It's not ideal to have 2 APIs which are essentially identical. If the problem mentioned above can be solved I'd much rather introduce a new argument to the add_from_* methods.

Comment 6 Kalle Vahlman 2007-06-17 14:37:58 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > Created an attachment (id=90136) [edit]
> > A preliminary implementation of constructing only a part of the xml tree
> 
> Thanks for the patch, in general it looks good but;
> 
> > What the patch does is simply carry a flag indicating if the current tag is
> > inside the intended build tree and skipping object construction when not.
> > Parsing of signals, properties and (obviously) children are not skipped 
> 
> I don't like the amount of changes this patch makes, is it not possible to
> modify start/end_element a little bit less?

Probably could, but when I tried to just skip the parsing of the elements I kept hitting the g_assert_not_reached()s and other state inconsistencies in end_element(). Gathering the object info and just omitting the construction seemed simpler. Could be that I just did it wrong of course. 

> > The second, perhaps more troublesome but I guess expected limitation is object
> > references:
> >   You cannot build a subtree that references objects outside the subtree you
> > are building. Most notably this means no UIManager-constructed widgets in 
> > the subtree.
> 
> Thats unfortunate and makes this feature less useful now when we support
> object references, which libglade didn't.

Yes, indeed.

> I'm almost leaning towards not supporting this feature because of that
> limitation, If it won't be possible to create menubars, toolbars, treemodels,
> sizegroups why should it be supported at all?

That might be a good idea. I think the only (common) use-cases for this are

 - Keeping in the same glade multiple windows of which you don't want to construct all on startup (main window and dialogs for example)
 - Re-using common widget trees in different parts of the UI

and both of these can be achieved simply by putting the descriptions into multiple files (which should be more efficient in any case since you don't need to parse the whole tree every time).

I'm just worried it might be a commonly used feature from which developers are not keen to move out (even if it means losing functionality) and thus make them stick with libglade...

One solution to the object reference problem would be to make the root parameter a list of objects to build. That way you could say:

  object_list[] = { "uimanager1", "sizegroup2½",
                    "treemodel3⅓", "window4¼",
                    NULL
                  };
  _add_from_file(..., object_list, ...);

Wouldn't that work?

> (In reply to comment #3)
> > Created an attachment (id=90137) [edit]
> > Few changes to gtk-demo for testing subtree building
> 
> This should really go into tests/buildertest.c, but I guess we're not quite at
> that point yet.

Yeah, that's just for demonstrative purposes :)
 
> (In reply to comment #4)
> > Now, as for the actual API, I'm leaning towards _add_subtree_from_*():
> > 
> >  - It doesn't introduce a parameter which will most of the time be NULL
> 
> > This would separate the subtree parsing while still being familiar from the
> > glade API. Thoughts? Do we want to take this to devel-list for discussion?
> 
> It's not ideal to have 2 APIs which are essentially identical. If the problem
> mentioned above can be solved I'd much rather introduce a new argument to the
> add_from_* methods.
 
Matter of tastes no doubt ;) but I guess first it should be decided if this is worth it at all.
Comment 7 Johan (not receiving bugmail) Dahlin 2007-06-17 22:17:11 UTC
(In reply to comment #6)
> One solution to the object reference problem would be to make the root
> parameter a list of objects to build. That way you could say:
> 
>   object_list[] = { "uimanager1", "sizegroup2½",
>                     "treemodel3⅓", "window4¼",
>                     NULL
>                   };
>   _add_from_file(..., object_list, ...);
> 
> Wouldn't that work?

I'd much rather not duplicate information which is the xml file. We can find out the dependencies by looking at the xml itself. That's non-trival however, especially with the current implementation of the parser.
Comment 8 Kalle Vahlman 2007-06-18 09:52:59 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > One solution to the object reference problem would be to make the root
> > parameter a list of objects to build. That way you could say:
> > 
> >   object_list[] = { "uimanager1", "sizegroup2½",
> >                     "treemodel3⅓", "window4¼",
> >                     NULL
> >                   };
> >   _add_from_file(..., object_list, ...);
> > 
> > Wouldn't that work?
> 
> I'd much rather not duplicate information which is the xml file. We can find
> out the dependencies by looking at the xml itself.

That's true, though the above was just an example of usage. You could choose to build two dialogs out of five or just build toolbar and view widget for a stripped-down preview window or something.

> That's non-trival however, especially with the current implementation of
> the parser.

Yes, it would really need to be two-pass operation, first parsing the objects and recording dependancies for the build phase. That's of course totally opposite of how the build process is now implemented and non-trivial wrt the custom tag parsing (you would need to accept unknown tags in the first pass and then reparse the subtree when the object is instantiated).

Since solving the object reference problem would require rewrite of the parsing engine, and making the user manage the dependancies with a list of buildable objects goes against the idea of easily modifyable UI's (add a sizegroup => oops you need to change code), I'm really beginning to feel strongly in favour of dumping the whole notion of subtree building. It can't be justified with the amount of work and complexity it would bring, since the same effect is trivially implemented by using separate descriptions.

If someone wants to make an addon that parses the XML description and strips out unwanted parts and *then* feeds it to the Builder, that's totally doable without it being supported by the GtkBuilder API in 2.12.
Comment 9 Matthias Clasen 2007-07-04 05:17:42 UTC
I'm in favour of dropping this feature, rather than shoe-horning it in with odd restrictions or corner-cases.

It might be useful to add some file-splitting feature to gtk-builder-convert,
though. Having a way to create separate xml files for 40 dialogs contained in a
single glade file might ease the transition for people who relied on the partial construction feature of libglade.
Comment 10 Johan (not receiving bugmail) Dahlin 2007-07-04 11:27:44 UTC
(In reply to comment #9)
> I'm in favour of dropping this feature, rather than shoe-horning it in with odd
> restrictions or corner-cases.

For Gtk+ 2.12, definitely. But we might want to support it in the future, it would involve rewriting most of the parser although...

> It might be useful to add some file-splitting feature to gtk-builder-convert,
> though. Having a way to create separate xml files for 40 dialogs contained in a
> single glade file might ease the transition for people who relied on the
> partial construction feature of libglade.
> 

I added --root option to gtk-builder-convert which skips everything but root and its children. But it would be nice to have something which automates the process by doing it on all dialogs/windows. 
Comment 11 Ivan Baldo 2007-08-08 00:22:17 UTC
Ouch! I use that functionality of libglade extensively.
I have for example a properties panel built dynamically when new elements get added by the user on the application.
I like to design the properties of every element for this panel on Glade seeing how they look together (to make them more consistent).
So, on the application when element of type X gets added, I build the properties for the X element and append them to a vbox; of course there could be multiple elements of type X added by the user.
My personal opinion: having that functionality even with those limitations is very useful on some situations.
Otherwise I must split the files manually or use the convert script and do that on Linux to not add a dependency on Python for building on Windows...
Could you reconsider this? Maybe asking on the gtk-app-devel list could help find more scenarios of this usage and see other opinions.
Of course maybe I am not doing things as I am supposed to do, though I couldn't imagine other way of doing this except by doing it by code instead of comfortably using Glade.
Thank you.
Comment 12 Johan (not receiving bugmail) Dahlin 2008-05-05 11:36:15 UTC
*** Bug 531415 has been marked as a duplicate of this bug. ***
Comment 13 Johan (not receiving bugmail) Dahlin 2008-06-01 13:43:57 UTC
*** Bug 535971 has been marked as a duplicate of this bug. ***
Comment 14 Johan (not receiving bugmail) Dahlin 2008-06-01 13:52:29 UTC
For new readers, read the whole discussion in bug 535971

(In reply to comment #5)
> Well, I didn't add a method fo adding a buffer with a root since I think that's
> a pretty remote use case: if you create the string programmatically you surely
> can create it with the proper root, while the main use case for the root_object
> argument when adding from a file is that you want to still edit the file in a
> GUI builder with the proper toplevel etc.

add_from_buffer is not only for creating strings programatically, imagine loading compressed data or data sent over the network. Sure you can create temporary files, but that's rather awkward.
 
> That said, since adding more than one file is not that common, as long as the
> functionality is there one way or the other I am willing to rework the 
> patch to add the set_root() method.

In hindsight, it might have made sense to add a root paremeter to add_from_file/add_from_buffer, but it is too late today, and as we cannot break the API/ABI we have to do something different.
I really prefer to have a API small API as possible and adding one method instead of two is usually a win. I also don't want to encourage the use of set_root(), since it's not as efficient as using separate files.

It would make more sense to make it easier to create a builder file with a root by using a easier api, as described in bug 447969.

Comment 15 Johan (not receiving bugmail) Dahlin 2008-06-01 14:12:04 UTC
Okay, I've changed my mind after discussion this here in the office. 
It makes more sense to add two apis, one for file and one for string, so please ignore these remarks in comment 14.

However, if I understand correctly, we still have the restrictions mentioned by Kalle in comment 2, right Paolo?
Comment 16 Paolo Borelli 2008-06-01 15:42:00 UTC
> However, if I understand correctly, we still have the restrictions mentioned by
> Kalle in comment 2, right Paolo?

As per discussion on irc, yes that's still the case, however it's not so bad since one can call add_with_root() again to also fetch the treemodel etc. Sure, it would mean trasversing the xml more than once, but I don't think that's a big issue.

An alternative is add an api to explicitely list a collection of objects to build from the xml (where the default is build all the toplevel objetcts)
Comment 17 Paolo Borelli 2008-06-01 15:57:34 UTC
The api could look like this:

gint gtk_builder_add_objects_from_[file|string] (GtkBuilder builder,
                                                 [filename | string, len],
                                                 GError err,
                                                 ...)

where in the vararg is a null terminated list of ids of objects you are interested in.

To get the contents of a dialog containing a treeview, you would list both the "mainvbox" object and the "treemodel" object.

The api would also allow for instance to store more than one toplevel in the same file but just build one (I know in general it's not good practice, but sometimes it could make sense, e.g. both toplevels share a treemodel etc).
Comment 18 Paolo Borelli 2008-06-02 10:34:42 UTC
Created attachment 111944 [details] [review]
patch

This patch implements gtk_builder_add_objects_from_[file|string]() so that you can build just a selection of objects from a ui description.

Instead of using a vararg like described above I opted for a simple array of strings since:
1 - vararg would have meant 4 new methods since also non-vararg version are needed
2 - varargs and optional GError do not mix well since usually they both need to be the last argument
3 - 99% of the times the list of objects you want are known at compile time so, something like gchar *objs[3] = {"mainbox", "treemodel1", NULL} is a reasonable way to specify them

Also, the method names are way nicer than set_root()/_with_root() :-)
Comment 19 Matthias Clasen 2008-06-08 03:52:53 UTC
As long as all the object references are forward references, it should be possible to just add to the list of requested objects on the fly. 
Backreferences would still be broken, but at least it would be possible to detect this once the parsing is complete, and issue an error.
Comment 20 Paolo Borelli 2008-06-08 13:10:00 UTC
Yes, forward references can be easily added to the list of requested objects, however I do not think we should issue an error at the end of the parsing: the dependency can be resolved with a separate call to add_from_file/string (e.g. a treemodel defined in a separate file). Such and error will be immediately evident when the actual widget is used so I'd not be too worried about it.

In fact I am not even sure that resolving deps is what we want: if I am cherry picking objects from a file it seems fair enough that I have to explicitly list all objects I need: for instance I may even want to get a treeview from a builder file without getting the model because I have two variants of the same dialog and in a case both the view and the model are in the builder file, while in the second case I just get the view and fill the model separately. No strong opinion on this though.
Comment 21 Matthias Clasen 2008-06-13 05:26:17 UTC
Hmm, I can probably live with the 'dangling references' problem if it is clearly spelled out in the docs, but the 'no subtrees in custom tag' seems just very hard to explain in the docs without just saying 'we suck'...
Comment 22 Paolo Borelli 2008-06-13 08:18:12 UTC
Well, I must admit that there are probably usecases of custom tags that I am not aware of, but at least as far as GtkUIManager is concerned (which is the custom tag example I looked at) I do not see the problem: the api I propose doesn't allow to cherry pick any subtree, it allows to cherry pick *objetcs*.
AFAIK custom tags can only be inside <child>, so if you cherry pick the  GtkUIManager object you also get the <ui>, if you just get a GtkAction you do not get the <ui> with sounds fair enough to me.

I guess there is a problem if there is something like <child><foo><objetc> and you try to cherry pick the object, but I am not even sure if that is considered legal.



Comment 23 Matthias Clasen 2008-07-12 01:35:51 UTC
Since there seems to be strong interest in using subtrees, I'm willing to go 
with the proposed api:

gint gtk_builder_add_objects_from_[file|string] (GtkBuilder builder,
                                                 [filename | string, len],
                                                 const gchar **names,
                                                 GError err)

But we need to spell out the restrictions about dangling references and objects in custom tags in the docs. 


Comment 24 Paolo Borelli 2008-07-15 08:07:19 UTC
In order to provide a patch with the updated docs, can someone clear my doubt in comment 22? Is having objtects inside custom tags ("<child><foo><objetc>") legal?

If that is not legal, we just need to warn about dangling references

Comment 25 Matthias Clasen 2008-07-15 15:09:38 UTC
No idea, honestly. 

Johan ?
Comment 26 Paolo Borelli 2008-07-16 08:11:38 UTC
Created attachment 114646 [details] [review]
patch

Here is a patch that adds a <note> in the docs about having to explicitely pick all the objects needed to satisfy dependencies.

Johan confirmed on irc that having objects inside custom tags is not allowed so there is no need to warn about that.
Comment 27 Johan (not receiving bugmail) Dahlin 2008-07-16 08:46:30 UTC
Comment on attachment 114646 [details] [review]
patch

Since Matthias has okayed this, I think it should go in.

The added API looks good, the implementation is reasonable, documentation is good. Lacking a few tests, for the error cases and for multiple objects (eg dependencies)
Comment 28 Paolo Borelli 2008-07-16 15:39:59 UTC
I added some more tests and committed, in particular a test showind how a uimanager is cherry picked and a menubar built with it.
I did not add test cases for error cases, since the only possible errors that can occur are on the same codepath of the plain add_file/add_string so they are already tested.
Comment 29 Murray Cumming 2008-07-16 15:59:16 UTC
Woot. Thank you all.
Comment 30 Murray Cumming 2008-07-23 09:31:45 UTC
Shouldn't those gchar** object_ids parameters be const?

Maybe gchar* const * const

However, GTK+ seems to generally use const gchar** so you might prefer that. For instance:
http://library.gnome.org/devel/gtk/unstable/GtkAboutDialog.html#gtk-about-dialog-set-documenters