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 679930 - gtk_builder_add_* should not use GError
gtk_builder_add_* should not use GError
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkBuilder
unspecified
Other All
: Normal normal
: ---
Assigned To: GtkBuilder maintainers
GtkBuilder maintainers
Depends on:
Blocks:
 
 
Reported: 2012-07-14 14:53 UTC by Allison Karlitskaya (desrt)
Modified: 2013-06-18 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
docs: GtkBuilder: warn about unusual GError use (2.16 KB, patch)
2012-07-14 15:05 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GtkBuilder: add new constructor APIs (8.74 KB, patch)
2012-07-14 16:44 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2012-07-14 14:53:51 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.
Comment 1 Allison Karlitskaya (desrt) 2012-07-14 15:05:39 UTC
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.
Comment 2 Allison Karlitskaya (desrt) 2012-07-14 16:44:49 UTC
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.
Comment 3 Allison Karlitskaya (desrt) 2012-08-08 15:37:26 UTC
review?
Comment 4 Tristan Van Berkom 2012-08-13 13:53:57 UTC
(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.
Comment 5 Tristan Van Berkom 2012-08-13 14:02:46 UTC
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).
Comment 6 Matthias Clasen 2013-06-17 22:02:09 UTC
Looks reasonable to me, fwiw.
Just add AVAILABLE_IN decorations, and update the Since: tags.
Comment 7 Allison Karlitskaya (desrt) 2013-06-18 14:21:44 UTC
Attachment 218814 [details] pushed as 1bc7359 - docs: GtkBuilder: warn about unusual GError use
Attachment 218824 [details] pushed as 7554c15 - GtkBuilder: add new constructor APIs