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 700629 - GtkComboBoxText <items> from GtkBuilder input is broken
GtkComboBoxText <items> from GtkBuilder input is broken
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkComboBox
3.9.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
: 701083 701933 (view as bug list)
Depends on:
Blocks: 549478
 
 
Reported: 2013-05-19 11:01 UTC by Tristan Van Berkom
Modified: 2013-06-23 20:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GtkBuilder file showing breakage (535 bytes, text/plain)
2013-05-19 11:01 UTC, Tristan Van Berkom
  Details
Memory allocation fixes for GtkBuilder translations (9.90 KB, patch)
2013-06-06 10:06 UTC, Kalev Lember
none Details | Review
Fix string allocation handling with GtkBuilder translations (4.39 KB, patch)
2013-06-07 13:24 UTC, Kalev Lember
committed Details | Review

Description Tristan Van Berkom 2013-05-19 11:01:18 UTC
Created attachment 244703 [details]
GtkBuilder file showing breakage

The attached GtkBuilder file shows a GtkComboBox which sets the <items>,
the combo seems to load the 2 items, and then fails to render them in
the drop down menu.

It would seem that the data is correctly in place, but for some reason
the cell renderer is missing.

To reproduce, fire up:
   glade-previewer -f combo-box-text-bug.ui (the attached file)
Comment 1 Matthias Clasen 2013-05-26 04:16:23 UTC
seems to work ok here ?
Comment 2 Tristan Van Berkom 2013-05-27 02:55:44 UTC
(In reply to comment #1)
> seems to work ok here ?

Strange.

I've updated GTK+ master and with today's master I still see no 'item1' or
'item2' rendered by the combo box (problem remains).

I'm half tempted to point at the theme setup, since I'm using stock GTK+
from a relocated jhbuild and no theme (and I suspect you are running
with Adwaita)... however it doesn't really make much sense that this
would be theme related since I use plenty of combo boxes which show up
fine, only that the items are added dynamically and not declared with
<items> in GtkBuilder.
Comment 3 Yanko Kaneti 2013-05-27 13:45:28 UTC
I am seeing what looks to be the same thing  with combos in evolution preferences on rawhide with a gtk snapshot from today. Can reproduce the supplied example breakage.
Comment 4 Yanko Kaneti 2013-05-27 14:43:43 UTC
The evolution issue which lead me here https://bugzilla.gnome.org/show_bug.cgi?id=700629
Comment 5 Yanko Kaneti 2013-05-27 17:07:59 UTC
Bisected to this 
commit e9f182e37a7f6e2dc339054841a3c9f930f573ed
Author: Matthias Clasen <mclasen@redhat.com>
Date:   Sun Apr 28 21:43:49 2013 -0400

    Fix a few memory leaks wrt to translations

Just reverting on top of current master fixes the rendering of the evolution composer preferences comboboxes
Comment 6 Yanko Kaneti 2013-05-27 17:15:24 UTC
*** Bug 701083 has been marked as a duplicate of this bug. ***
Comment 7 Kalev Lember 2013-06-05 19:01:42 UTC
I ran into the same problem today and debugged it further, and I believe I understand what's going on here.

The issue is that _gtk_builder_parser_translate() can now return the same pointer that is passed in, and that trips up the callers.

from gtk/gtkcomboboxtext.c:
          translated = _gtk_builder_parser_translate (data->domain,
                                                      data->context,
                                                      data->string->str);
          g_string_set_size (data->string, 0);
          g_string_append (data->string, translated);

Depending on the locale, in some cases the returned translated == data->string->str. In that case the code above does the following:

1) translated = a pointer to GString's internal character array

2) reset the size of GString to 0, which invalidates the internal character array

3) call append(), passing in the pointer to the now invalidated internal array

I think the best way here would be go back to returning a strdup'ed string from _gtk_builder_parser_translate() and just have callers free it. Less suprises that way.


I can do the patch later today if it makes sense.
Comment 8 Tristan Van Berkom 2013-06-06 09:12:02 UTC
(In reply to comment #7)
> I ran into the same problem today and debugged it further, and I believe I
> understand what's going on here.
> 
> The issue is that _gtk_builder_parser_translate() can now return the same
> pointer that is passed in, and that trips up the callers.
> 
> from gtk/gtkcomboboxtext.c:
>           translated = _gtk_builder_parser_translate (data->domain,
>                                                       data->context,
>                                                       data->string->str);
>           g_string_set_size (data->string, 0);
>           g_string_append (data->string, translated);
> 
[...]
> 
> I can do the patch later today if it makes sense.

Nice catch.

As _gtk_builder_parser_translate() is an internal function I guess
it doesn't matter if we change it's behaviour (and grep tells me there
are only a hand full of usages of that function).

I think I agree that reverting the behaviour would be safer (and more
readable), since the original change was to avoid memory leaks, the
patch should of course ensure that we don't have leaks around calls
to _gtk_builder_parser_translate() (bug 699016).
Comment 9 Kalev Lember 2013-06-06 10:06:32 UTC
Created attachment 246130 [details] [review]
Memory allocation fixes for GtkBuilder translations

This is another attempt to squash some memory leaks with GtkBuilder
translations and at the same time fix regressions from the previous
attempt.

Commit e9f182e37a7f6e2dc339054841a3c9f930f573ed changed
_gtk_builder_parser_translate() to avoid strdup() and just pass on the
gettext() return value. Depending on if the translation was found, the
return value would be either a pointer to a statically allocated string,
or just the msgid parameter passed back. That led to some confusion with
callers deallocating the msgid before using the return value --
sometimes it would work, sometimes it would also deallocate the returned
value; depending on the locale.

With LANG=C, GtkComboBoxText <items> from GtkBuilder started showing up
as empty; with some other locales that had translations it would work.

This commit goes back to the less suprising behaviour where
_gtk_builder_parser_translate dup's the string and callers are
responsible for freeing it.
Comment 10 Matthias Clasen 2013-06-07 11:21:21 UTC
What we came up with on irc is to replace 

 	  g_string_set_size (parser_data->string, 0);
 	  g_string_append (parser_data->string, translated);

with 

          g_string_assign (parser_data->string, translated);

instead.
g_string_assign is self-assignment-safe.
Comment 11 Kalev Lember 2013-06-07 13:24:36 UTC
Created attachment 246247 [details] [review]
Fix string allocation handling with GtkBuilder translations

Use g_string_assign to avoid issues with assigning GString's internal
buffer back to the same string. This can happen when no translations are
available and _gtk_builder_parser_translate returns back the same
pointer that was passed in.

This fixes a regression from commit e9f182e37a7f6e2dc339054841a3c9f930f
that caused GtkComboBoxText <items> from GtkBuilder to show up empty if
no translations are available.
Comment 12 Matthias Clasen 2013-06-08 00:32:22 UTC
Review of attachment 246247 [details] [review]:

thanks, looks good
Comment 13 Kalev Lember 2013-06-08 08:55:53 UTC
Thanks!

Attachment 246247 [details] pushed as a374501 - Fix string allocation handling with GtkBuilder translations
Comment 14 Matthew Barnes 2013-06-23 20:29:36 UTC
*** Bug 701933 has been marked as a duplicate of this bug. ***