GNOME Bugzilla – Bug 696307
Button “frequent” doesn't scale to length of text
Last modified: 2013-06-04 11:54:55 UTC
In “Show applications” mode, the bottom “Frequent”/“All” buttons should scale with length of the text. For example, in Latvian, “Frequent” translates as “Biežāk izmantotās”, which gets truncated. Steps to reproduce: 1) Change system language to Latviešu (Latvian); 2) Log out/log in; 3) Open Overview mode and open “Show applications”
*** Bug 697439 has been marked as a duplicate of this bug. ***
See for another exemple : https://bug697439.bugzilla-attachments.gnome.org/attachment.cgi?id=240845 (in french "Frequent" becomes "Fréquemment utilisées" and, as it doesn't fit within the Tab Buttons fixed size, the label is shortened to "Fréquemme...")
this sounds like a possibly nice thing to fix for 3.8.1
Thanks for the bug report. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 664411 ***
Not fixed, reopening.
What we want is a CSS property like -st-natural-width greater than we have now(as width property), as we did at https://bugzilla.gnome.org/show_bug.cgi?id=664411 ??
No, -st-natural-width sets the preferred width while still allowing the widget to shrink when space is limited (up to the widget's minimum size). I'd say here we want each button to have the maximum natural width of all buttons.
Created attachment 245220 [details] [review] appDisplay: Scale control buttons to text length Before, the text of those buttons were truncated when the text exceeded the fixed width we had at the CSS. Now, the buttons are scaled to the maximum text length of all buttons.
Created attachment 245224 [details] Patch working with a custom loong label
Review of attachment 245220 [details] [review]: ::: js/ui/appDisplay.js @@ +344,3 @@ + * To preserve homogeneous size buttons with none of the texts of the + * buttons truncated we have to override the BoxLayout behaviour to use as + * button width the maximum width of the bigger button. The last part reads odd - how about "override the BoxLayout behavior to use the maximum preferred width of all buttons for each child"? @@ +353,3 @@ + child; + child = child.get_next_sibling()) { + global.log(child.get_preferred_width(forHeight)); Left-over debug spew @@ +357,3 @@ + maxMinWidth = Math.max(maxMinWidth, preferredWidth[0]); + maxNaturalWidth = Math.max(maxNaturalWidth, preferredWidth[1]); + global.log([maxMinWidth, maxNaturalWidth]); Dto. @@ +360,3 @@ + } + // Now, multiply the maximum width with the number of childs to get + // the container total needed width The comment just describes what the code already obviously does - remove it. @@ +362,3 @@ + // the container total needed width + let childrenCount = container.get_n_children(); + return [maxMinWidth * childrenCount, maxNaturalWidth * childrenCount]; We currently don't set ClutterBoxLayout:spacing, but it would be good to take it into account just in case. (If you want to get fancy, you could implement set_container(), connect to the container's style-changed signal and hook up the layout's spacing property to a 'spacing' css property) Also it would be nifty to take the container's standard CSS properties into account (see st_theme_node_adjust_for_height() and st_theme_node_adjust_preferred_width()) @@ -385,3 @@ this.actor.add(new St.Bin({ child: this._controls })); - Unnecessary whitespace change.
(In reply to comment #9) > Created an attachment (id=245224) [details] > Patch working with a custom loong label The button could use some padding :-)
Created attachment 245472 [details] [review] appDisplay: Scale control buttons to text length Before, the text of those buttons were truncated when the text exceeded the fixed width we had at the CSS. Now, the buttons are scaled to the maximum text length of all buttons.
Review of attachment 245472 [details] [review]: ::: js/ui/appDisplay.js @@ +353,3 @@ + let preferredWidth = child.get_preferred_width(forHeight); + maxMinWidth = Math.max(maxMinWidth, preferredWidth[0]); + maxNaturalWidth = Math.max(maxNaturalWidth, preferredWidth[1]); More elegant as let [minWidth, natWidth] = child.get_preferred_width(forHeight); maxMinWidth = Math.max(maxMinWidth, minWidth); @@ +357,3 @@ + let childrenCount = container.get_n_children(); + let totalSpacing = 0; + if(this.spacing != null) { ClutterBoxLayout:spacing defaults to 0, so you can just use let totalSpacing = this.spacing * (childrenCount - 1); (e.g. if spacing is unset, totalSpacing will end up correctly as 0) @@ +360,3 @@ + totalSpacing = this.spacing * (childrenCount - 1); + } + return [maxMinWidth * childrenCount + totalSpacing, maxNaturalWidth * childrenCount + totalSpacing]; It would be nice to take the container's padding and borders into account - see st_theme_node_adjust_for_height() and st_theme_node_adjust_preferred_width(). @@ +368,3 @@ + this._container = container; + } else { + this._container= null; This is basically if (foo == null) bar = null; else bar = foo; which you can write more easily as bar = foo; :-) However while I don't see us swapping out containers any time soon, for correctness you should save the signal id and disconnect it as necessary. @@ +373,3 @@ + + _onStyleChanged: function() { + if(this._container != null){ This is a handler for the StWidget::style-changed signal, which you should *only* ever receive for this._container (see comment above about disconnecting the handler for "old" containers). In other words, you don't need this check. @@ +426,3 @@ + this._controls = new St.Widget({ style_class: 'app-view-controls' }); + //Hack: since we overrides set_container from BoxLayout and we don't associate + //the container to the manager in that function we need to associate it later. I don't understand that comment. set_container() is called by clutter when the layout manager is assigned to an actor. This is a problem when using a construct property, as the container is still constructing when set_container() is called, so the introspection layer doesn't get what it expects.
Created attachment 245598 [details] [review] appDisplay: Scale control buttons to text length Before, the text of those buttons were truncated when the text exceeded the fixed width we had at the CSS. Now, the buttons are scaled to the maximum text length of all buttons.
In the last patch, as I talk with Florian, we have to set the layout manager after the contrusctor. Also, there's not need to take into account padding and borders, since it is already taked into account. The only thing we take into account is spacing.
Review of attachment 245598 [details] [review]: Marking needs-work because it doesn't work (typo!), it's actually fairly close (some nitpicks apart). The commit message isn't quite right either - "scale" has syntactic meaning in Clutter (think "zoom"), so using it differently is confusing. ::: js/ui/appDisplay.js @@ +359,3 @@ + let [minWidthContainer, maxNaturalWidthContainer] = [maxMinWidth * childrenCount + totalSpacing, + maxNaturalWidth * childrenCount + totalSpacing]; + return [minWidthContainer, maxNaturalWidthContainer]; The intermediate variables are a bit pointless, just return [maxMinWidth * childrenCount + totalSpacing, maxNaturalWidth * childrenCount + totalSpacing]; @@ +363,3 @@ + + vfunc_set_container: function(container) { + if(this._singalId != null) { The joy of dynamically typed languages - the typo makes this "if (false)" @@ +365,3 @@ + if(this._singalId != null) { + this._container.disconnect(this._signalId); + this._signalId = null; "signalId" is a slightly less generic name than "variable" - I'd prefer something a bit more specific ... (Also another nitpick: signal ids are integers, so 0 (which is not a valid signal id) makes more sense then null (though javascript doesn't differentiate between the two unless you compare with ===/!==)) @@ +369,3 @@ + if(container != null) { + this._signalId = container.connect('style-changed', Lang.bind(this, this._onStyleChanged)); + } Style nit: no braces. @@ +370,3 @@ + this._signalId = container.connect('style-changed', Lang.bind(this, this._onStyleChanged)); + } + this._container= container; Style nit: missing space before = @@ +374,3 @@ + + _onStyleChanged: function() { + this.spacing = this._container.get_theme_node().get_length('spacing'); You may consider an anonymous function for a one-liner like this, though this is obviously not wrong - fine either way, your call.
Also the buttons look a bit narrow to me with the patch applied - Jakub, can you take a look and comment?
(In reply to comment #16) > (Also another nitpick: signal ids are integers, so 0 (which is not a valid > signal id) makes more sense then null (though javascript doesn't differentiate > between the two unless you compare with ===/!==)) ??? js> 0 == null false js> null == 0 false js> 0 != null true js> null != 0 true
(In reply to comment #18) > ??? Gah, I blame the time on this side of the pond!
(In reply to comment #16) > Review of attachment 245598 [details] [review]: > > Marking needs-work because it doesn't work (typo!), it's actually fairly close > (some nitpicks apart). > > The commit message isn't quite right either - "scale" has syntactic meaning in > Clutter (think "zoom"), so using it differently is confusing. > How about "Now, the buttons are resized to the maximum text length of all buttons." ?? > ::: js/ui/appDisplay.js > @@ +359,3 @@ > + let [minWidthContainer, maxNaturalWidthContainer] = [maxMinWidth * > childrenCount + totalSpacing, > + maxNaturalWidth * > childrenCount + totalSpacing]; > + return [minWidthContainer, maxNaturalWidthContainer]; > > The intermediate variables are a bit pointless, just > return [maxMinWidth * childrenCount + totalSpacing, > maxNaturalWidth * childrenCount + totalSpacing]; > "Refactoring: Improving the Design of Existing Code" book fault hehe. Done. > @@ +363,3 @@ > + > + vfunc_set_container: function(container) { > + if(this._singalId != null) { > > The joy of dynamically typed languages - the typo makes this "if (false)" > ouch! good eyes...Done. > @@ +365,3 @@ > + if(this._singalId != null) { > + this._container.disconnect(this._signalId); > + this._signalId = null; > > "signalId" is a slightly less generic name than "variable" - I'd prefer > something a bit more specific ... > (Also another nitpick: signal ids are integers, so 0 (which is not a valid > signal id) makes more sense then null (though javascript doesn't differentiate > between the two unless you compare with ===/!==)) > It's true, named to styleChangedSignalId now. Also, due to assign signalId a 0 (and last comment from Jasper), now I have to take into account singalId!= null and singalId != 0 in the if condition (it is ok?). Done. > @@ +369,3 @@ > + if(container != null) { > + this._signalId = container.connect('style-changed', > Lang.bind(this, this._onStyleChanged)); > + } > > Style nit: no braces. > Done > @@ +370,3 @@ > + this._signalId = container.connect('style-changed', > Lang.bind(this, this._onStyleChanged)); > + } > + this._container= container; > > Style nit: missing space before = > Done > @@ +374,3 @@ > + > + _onStyleChanged: function() { > + this.spacing = this._container.get_theme_node().get_length('spacing'); > > You may consider an anonymous function for a one-liner like this, though this > is obviously not wrong - fine either way, your call. I don't like the mess of anonymous functions inside calls, but in this case, as you said, it's one line, so ok, I did an anonymous function. Done. Thanks for the well explained review Florian!
Created attachment 245815 [details] [review] appDisplay: Resize control buttons to text length Before, the text of those buttons were truncated when the text exceeded the fixed width we had at the CSS. Now, the buttons are resized to maximum text length of all buttons.
Review of attachment 245815 [details] [review]: (In reply to comment #20) > How about "Now, the buttons are resized to the maximum text length of all > buttons." ?? Better, though technically not quite correct (the buttons are not resized in the sense of changing their width property, they are given move horizontal space than they asked for in their size requests). > It's true, named to styleChangedSignalId now. > Also, due to assign signalId a 0 (and last comment from Jasper), now I have to > take into account singalId!= null and singalId != 0 in the if condition (it is > ok?). Done. The name is ok, though our common practice is to just use signalName + Id (e.g. styleChangedId). Regarding the condition, you can just use if (this._styleChangedId) this._container.disconnect(this._styleChangedId); this._styleChangedId = 0; It is also good practice to initialize the property to 0 in _init(), but overriding the parent's _init() just for this purpose as would be the case here would be overkill.
(In reply to comment #22) > Review of attachment 245815 [details] [review]: > > (In reply to comment #20) > > How about "Now, the buttons are resized to the maximum text length of all > > buttons." ?? > > Better, though technically not quite correct (the buttons are not resized in > the sense of changing their width property, they are given move horizontal > space than they asked for in their size requests). > oh ok (my faulty English), see the next commit message, tell me if it is good for you. > > It's true, named to styleChangedSignalId now. > > Also, due to assign signalId a 0 (and last comment from Jasper), now I have to > > take into account singalId!= null and singalId != 0 in the if condition (it is > > ok?). Done. > > The name is ok, though our common practice is to just use signalName + Id (e.g. > styleChangedId). Regarding the condition, you can just use > > if (this._styleChangedId) > this._container.disconnect(this._styleChangedId); > this._styleChangedId = 0; > > It is also good practice to initialize the property to 0 in _init(), but > overriding the parent's _init() just for this purpose as would be the case here > would be overkill. The name of the variable was taken from examples of other files on gnome-shell, concretly I saw Background.js(the first alphabetically ordered file I found with "disconnect()" function), which is plenty of fooSignalId's. But as you said, seeing other gnome-shell files the common practice is fooId, so I changed it :) I thougth the same as you about the rewrite of _init, that's why I didn't do it.
Created attachment 245825 [details] [review] appDisplay: Give more horizontal space to control buttons Before, the text of those buttons were truncated when the text exceeded the fixed width we had at the CSS. Now, we give more horizontal space to the control buttons to match the maximum text length of all buttons, avoiding truncate text.
Review of attachment 245825 [details] [review]: Looks good, thanks! Minor nits in the commit message: "we had at the CSS" => "we had in the CSS" "avoiding truncate text" sounds wrong, not sure what would be better - "avoiding ellipsization"? "to avoid truncating text"? Just finish the sentence after "all buttons"? Also comment #17 still applies - if we want to bump padding on the buttons, that should be a separate patch, but if we want to use a min-width instead (where we currently use width), we can squash this into this patch. So I'd still like some design feedback before pushing.
(In reply to comment #25) > Review of attachment 245825 [details] [review]: > > Looks good, thanks! > Cool! Hope I don't give you so much work and iterations in the next patch. (But I can't assure it to you hehe) > Minor nits in the commit message: > "we had at the CSS" => "we had in the CSS" > "avoiding truncate text" sounds wrong, not sure what would be better - > "avoiding ellipsization"? "to avoid truncating text"? Just finish the sentence > after "all buttons"? > Sorry, my poor English. Done. > Also comment #17 still applies - if we want to bump padding on the buttons, > that should be a separate patch, but if we want to use a min-width instead > (where we currently use width), we can squash this into this patch. So I'd > still like some design feedback before pushing. Ok, we wait. Thanks florian!
Created attachment 245827 [details] [review] appDisplay: Give more horizontal space to control buttons Before, the text of those buttons were truncated when the text exceeded the fixed width we had in the CSS. Now, we give more horizontal space to the control buttons to match the maximum text length of all buttons.
Comment on attachment 245827 [details] [review] appDisplay: Give more horizontal space to control buttons Assuming no code changes, so marking a-c-n again.
Carlos, do you have commit rights?
Jasper, Yes I do. Now we are waiting for the design team now.
(In reply to comment #9) > Created an attachment (id=245224) [details] > Patch working with a custom loong label Yeah, I'd add some more padding to either side of the string. Try 6px or 12px and see what's best.
Carlos, If you need further feedback, please attach screenshots, not another patch version (it's ready!); if you are happy with the padding and push, please do so to both matter and gnome-3-8.
Created attachment 245863 [details] patch working Hi Allan, Trying to be consistent with other shell buttons, I search for those buttons paddings (i.e. modal-dialog-button), an it is 32px. In fact, with this padding, the result is very close to that we had before the patch. So I go with that. See screenshot attached to see the result =) Now, I have to wait to accounts@gnome.org that give me permission to another SHA key to commit, since I erased the last one by accident... Then I'll commit the patch. Thanks everybody for the help!
Attachment 245827 [details] pushed as f9e3467 - appDisplay: Give more horizontal space to control buttons