GNOME Bugzilla – Bug 679930
gtk_builder_add_* should not use GError
Last modified: 2013-06-18 14:22:10 UTC
GtkBuilder uses GError to report errors. Some number of years ago I tried to make that actually work properly (ie: I tried to make it so that parsing some invalid XML with GtkBuilder would result in you getting an GError output and able to continue along your way). I had some success, but really it was a total waste of time. GtkBuilder is still leaking memory in some of the failure cases and it's an extremely difficult problem to fix. Meanwhile, if a program fails to parse its user interface, the only reasonable thing to do is exit. Having these functions return GError is confusing to programmers who think that they should attempt to handle these failures. I think we need to do two things: - modify the API doc for these functions to instruct the user that the only reasonable thing to do on failure is to immediately g_error() - add a new API that does this for them and make that the preferred one I've always wondered why we don't have gtk_builder_new_from_file()/_new_from_resource() so perhaps it is time to introduce those, with abort-on-fail.
Created attachment 218814 [details] [review] docs: GtkBuilder: warn about unusual GError use GtkBuilder returns GError for _add_from_{file,resource,string}(), implying that the user should be able to recover from these errors. Mention in the docs that it's unreasonable to try to do this.
Created attachment 218824 [details] [review] GtkBuilder: add new constructor APIs Add new APIs gtk_builder_new_from_{file,resource,string}() and encourage their use over the older _add_from_...() APIs.
review?
(In reply to comment #0) > GtkBuilder uses GError to report errors. > > Some number of years ago I tried to make that actually work properly (ie: I > tried to make it so that parsing some invalid XML with GtkBuilder would result > in you getting an GError output and able to continue along your way). > > I had some success, but really it was a total waste of time. GtkBuilder is > still leaking memory in some of the failure cases and it's an extremely > difficult problem to fix. Meanwhile, if a program fails to parse its user > interface, the only reasonable thing to do is exit. > FWIW, I agree with this. Note that while using these apis, it becomes impossible for one to add multiple xml fragments to the same GtkBuilder object... however that's not a practice that I would like to encourage anyway (IMO, a GtkBuilder object should be used at interface initialisation time and discarded as soon as possible, ideally). In other words, I think its a good thing to also force this paradigm by promoting the newer apis.
Review of attachment 218824 [details] [review]: Overall the patch is straightforward and functional. The only thing I wonder is if we should more explicitly discourage the merging of multiple xml fragments into the same GtkBuilder object (for instance, in the documentation changes you made to gtk_builder_new() ). Doing so generally leads to: o Risk of name collisions o Lazy Programming practices where one "holds" the GtkBuilder object, incurring a hash-table lookup to access any of the built objects and some extra memory that nobody needs to allocate really. In any case, GtkBuilder objects are not 'pathed' like GtkUIManager objects were, so the merging of user interfaces was never really sophisticated or useful as the feature was with GtkUIManager. Further, I hope to remedy the latter issue with the composite branch which Juan has been working on (hopefully I'll have time this week to finalise those patches since the topic is still fresh from GUADEC, in which case we could have the option of discouraging direct usage of GtkBuilder apis altogether).
Looks reasonable to me, fwiw. Just add AVAILABLE_IN decorations, and update the Since: tags.
Attachment 218814 [details] pushed as 1bc7359 - docs: GtkBuilder: warn about unusual GError use Attachment 218824 [details] pushed as 7554c15 - GtkBuilder: add new constructor APIs