RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]

[Andy Goryachev]

On Fri, 3 Feb 2023 05:56:32 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 402:
>> 
>>> 400:         }
>>> 401: 
>>> 402:         return  height + padding;
>> 
>> My only comment is possibly reorganize the logic to avoid unnecessary computation.  
>> For example, there is no need to compute text width (line 329) if isIgnoreText() is true; 
>> Similarly, no need to compute graphicWidth on line 333 if isIgnoreGraphic() is true.
>> 
>> (basically, same comment as @hjohn has made earlier)
>> 
>> What do you think, @karthikpandelu ?
>
> Not sure if I didn't understand your comments properly but I'm guessing you are referring to `textHeight` and `graphicHeight` calculation in line 375 and 380 because the line numbers you mentioned are in `computePrefWidth` method and changes are made to height calculation in `computePrefHeight` only.
> Since `textHeight` and `graphicHeight` each are used in 3 conditions from line no 386 to 394 I have kept the calculation in the beginning itself. 
> 
> On the other hand `textWidth` declared in line 368 might change in line 371, so I can't use `width` directly instead of defining `textWidth` as `width` need to be used while calling other graphic related methods.

Both `computePrefHeight()` and `computePrefWidth` can be restructured to avoid unnecessary computation (especially since these are very popular objects).

My point is that, for example, textWidth on line 375 (used to compute textHeight:375) is not used if isIgnoreText() == true.  

Similarly, graphicHeight:380 is not used if isIgnoreGraphic() == true, line 386.

Same optimization can be applied to computePrefWidth():315

Just a suggestion, really.

-------------

PR: https://git.openjdk.org/jfx/pull/996