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

Andy Goryachev angorya at openjdk.org
Thu Feb 2 16:58:36 UTC 2023


On Wed, 18 Jan 2023 06:56:57 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> Ignore text condition was not considered in `computePrefHeight` method while calculating the Label height.
>> 
>> Added `isIgnoreText` condition in `computePrefHeight` method while calculating Label height.
>> 
>> Tests are present for all the ContentDisplay types. Modified tests which were failing because of the new height value getting calculated.
>
> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use width while calling prefHeight method. Use graphicHeight instead of multiple calls to prefHeight

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 ?

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

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


More information about the openjfx-dev mailing list