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

Karthik P K kpk at openjdk.org
Wed Feb 8 12:59:01 UTC 2023


On Tue, 7 Feb 2023 19:57:00 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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 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.

In the case of `computePrefHeight` as well calculation of `graphicHeight` is moved to `else` block to avoid one unnecessary calculation.
In this method, by checking for ignore text condition before checking for ignore graphic, we can move `cleanText`, `textWidth` and `textHeight` calculation to `else` block instead of only `graphicHeight`. But this again leads to unexpected result because of the checks present in `isIgnoreText()` method. Hence did not re-order `if-else` statements now.

Added boolean for both `isIgnoreGraphic` and `isIgnoreText`.

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

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


More information about the openjfx-dev mailing list