On Tue, 7 Feb 2023 05:25:08 GMT, Karthik P K <k...@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
