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

[Andy Goryachev]

On Tue, 7 Feb 2023 05:25:08 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:
> 
>   Addressing review comments

modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 328:

> 326:         }
> 327: 
> 328:         double textWidth = emptyText ? 0.0 : Utils.computeTextWidth(font, cleanText, 0);

textWidth is being computed even if isIgnoreText() is true.  I wonder if we could re-structure the if-else statements below to avoid unnecessary computations?

There is another suggestion - isIgnoreText() is not a simple getter (does a bit of computation).  We probably should use a boolean early in this method to avoid calling it repeatedly.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 382:

> 380:                 labeled.getLineSpacing(), text.getBoundsType());
> 381: 
> 382:         double height;

same comment here - textHeight is computed regardless of whether its needed (isIgnoreText() == true),

and use a boolean to avoid multiple calls to isIgnoreText() in the same method.

isIgnoreGraphic, on the other hand, is very light weight, so it's up to you whether to use a local boolean variable in this and the other method or not.

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

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