GNOME Bugzilla – Bug 353088
gtk_expander_get_label should return the full label text
Last modified: 2009-04-07 16:57:28 UTC
gtk_expander_get_label should return the label set by gtk_expander_set_label but currently it retuns the label text stripped of any Pango markup or underscore character. The problem is that gtk_expander_get_label uses gtk_label_get_text instead of gtk_label_get_label.
Created attachment 87247 [details] [review] Trivial patch. Not sure if this counts as API change though (someone might already depend on the current behavior)
Looks right to me.
committed to trunk. 2008-03-22 Björn Lindqvist <bjourne@gmail.com> * gtk/gtkexpander.c (gtk_expander_get_label): gtk_label_get_label() should be used instead of gtk_label_get_text(). (#353088, Xan Lopez)
Unfortunately, I've got to re-open this one. Xan's comment on "someone might already depend on the current behavior" turns out to have been true. From modules/other/gail/gailexpander.c:gail_expander_get_name it ends up calling gtk_expander_get_label. This previously use to return the label text, with the underscore's stripped. With this change to gtkexpander.c, the accessible layer is seeing underscores in expander names (accersicer, orca, etc). I see two potential fixes: 1) Introduce a gtk_expander_get_text function, which would ultimately end up calling gtk_label_get_text. This of course introduces new API just to fix a bug. Not sure the policy on this. 2) Enhance gailexpander.c to use gtk_expander_get_label_widget, and then just doing a gtk_label_get_text on that. This would restore the previous functionality without new API. Personally, I'd say #2 is probably the better bet, but I'm certainly not the exper on this. Thoughts?
I say lets: - Revert the patch, - Do #2, - Deprecate gtk_expander_get_label How does that sound?
Behdad, that would go beyond fixing the bug introduced. I can't really comment on the deprecation issue, but I just cooked a patch to make gailexpander.c use gtk_expander_get_label_widget instead, and am compiling now to test it.
Created attachment 121908 [details] [review] Fix to make gailexpander.c not use gtk_expander_get_label This implements "Option #2" and seems to work as expected. Accersicer now shows the text as it did with gtk+ 2.12 in all places. Would probably be good to have a "gail person" look at this, as I'm ceratinly not well versed in the gail code.
Unless there's some reason for the differences in behavior of the additions (basically using g_return_if_fail sometimes and not others) I think I'd make a small function to get the label text from the expander, as you are repeating it 4 times. Other than that looks good to me.
In one of the cases, it's sufficient to return NULL, but the others have further logic I was scared to bypass, so I attempted to continue, but using NULL for the label text. I certainly could make a small function for returning the text, and use it in all 4 spots though.
Created attachment 121922 [details] [review] Cleaner fix for gailexpander.c Ok, this is a cleaner (IMHO) patch, that also prevents a Gtk-CRITICAL from being printed, by avoiding using g_return_val_if_fail.
Well, we now have a gtk_expander_get_label symbol that has one semantic in most releases and another in the latest one. Granted, it seems like no one is using it (quick Google Code check), but if we're not ok adding a _get_text equivalent on the basis that it can be done equivalently already, then the least-confusion approach may be to simply deprecate the current problematic _get_label symbol too.
If get_label is deprecated, then set_label should too be deprecated... In fact, why not deprecate the other label-setter attributes too, use_underline and use_markup? They are just convenience functions for C programmers. I also think that gail is the one being bad here. The gtk_expander_get_label documentation clearly states "Fetches the text from a label widget including any embedded underlines indicating mnemonics and Pango markup." So if we go with the old get_text behaviour, then the docs should be updated too.
Regardless of this API debate, can we at least get the attached fix commited, which works as expected?
The gail patch looks fine. Please commit. To both branches, if you don't mind. Wrt. to the api debate, I'm not sure deprecating it all is a good idea. Reverting to the old behaviour doesn't strike me as very convincing either, since the old behaviour _was_ wrong, and we've only found a single user, which we are fixing with the above patch. I'd propose to leave the behaviour as it is now, and add a note to the docs pointing out that the function behaved differently in versions < 2.14.
Created attachment 123601 [details] [review] Note change in docs
Might want to add that this problem can be avoided by getting the label widget and using its api.
Created attachment 123824 [details] [review] Better note in docs
What's the status of this? Can we commit the documentation improvements and close this bug?