GNOME Bugzilla – Bug 774652
[PATCH Label: Don't use misleading parameter names in constructor: currently x/y align actually set h/valign
Last modified: 2016-11-19 23:26:09 UTC
These arguments are forwarded to set_halign() and set_valign(), yet they were named xalign and yalign, which is especially confusing because Label already has completely separate xalign and yalign properties. This could lead to users getting confused[*] when they think they're setting one of these pairs of properties but actually setting the other. This might've been a holdover from the deprecated constructor that takes doubles and _did_ call set_alignment(xalign, valign) * see: me
Created attachment 340192 [details] [review] patch: rename x/yalign to h/valign to reflect what they actually do
Created attachment 340195 [details] [review] patch: improve h/valign constructor doc comment While we're here, I'd also suggest amending the doc comment to something like this. (I hope I didn't exceed some line width limit or something)
Created attachment 340196 [details] [review] patch: improve h/valign constructor doc comment oops, forgot the bz link
That looks useful. I wonder if the constructor is even useful now? Maybe xalign and yalign are what people are really interested in when creating the label. But we couldn't just change it to set x/y instead of h/v.
(In reply to Murray Cumming from comment #4) > I wonder if the constructor is even useful now? Maybe xalign and yalign are > what people are really interested in when creating the label. But we > couldn't just change it to set x/y instead of h/v. I initially thought the same thing, until I noticed that in 3.22, there's already a constructor taking doubles for x/yalign - but it's deprecated. Do we know why that is? It seems that someone already had the same idea as you but then changed their mind. https://git.gnome.org/browse/gtkmm/tree/gtk/src/label.hg?h=gtkmm-3-22#n47
That deprecated constructor is just the one that takes float. The one you've patched takes an Align enum. I believe it replaced the one that takes float.
So are you thinking it might be worth flipping these back around, and having a convenience constructor that (really) sets x/yalign instead of h/valign? It does seem like the more useful of the two, if we had to pick one. Not sure, but I don't think many other widgets have h/valign convenience constructors?
Thanks. I have pushed both patches to master and the gtkmm-3-22 branch. I think it would be best to remove this constructor in gtkmm 4 (git master). It might be more useful if it used xalign/yalign, but we cannot just change the behaviour of the existing constructor. I think Label had this because the lack of left text alignment by default was particularly annoying for this widget.
Thanks!