On Tue, 17 Jan 2023 11:40:47 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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 393:
>
>> 391: height = graphic.prefHeight(-1) + gap + textHeight;
>> 392: } else {
>> 393: height = Math.max(textHeight, graphic.prefHeight(-1));
>
> The logic of this code seems to have changed more than just the fixing bug.
> Why are the `prefHeight` functions now called with `-1`? Normally these
> should respect the content bias of the node involved.
>
> Also, I notice that you check `graphic == null`, while the `isIgnoreGraphic`
> function checks quite a bit more:
>
> boolean isIgnoreGraphic() {
> return (graphic == null ||
> !graphic.isManaged() ||
> getSkinnable().getContentDisplay() ==
> ContentDisplay.TEXT_ONLY);
> }
>
> Doing all those operations on `graphic` when for example the `ContentDisplay`
> is `TEXT_ONLY` seems unnecessary. Looking at the rest of the code,
> `graphicHeight` is only used in one branch in your if/elseif/elseif/else.
>
> I also wonder if a simple fix like this would have the same result:
>
>
> - final double textHeight = Utils.computeTextHeight(font, cleanText,
> + final double textHeight = isIgnoreText() ? 0.0 :
> Utils.computeTextHeight(font, cleanText,
> labeled.isWrapText() ? textWidth : 0,
> labeled.getLineSpacing(), text.getBoundsType());
@hjohn please take a look and review the changes made.
-------------
PR: https://git.openjdk.org/jfx/pull/996
