On Thu, 2 Feb 2023 16:54:28 GMT, Andy Goryachev <ango...@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 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 ? Not sure if I didn't understand your comments properly but I'm guessing you are referring to `textHeight` and `graphicHeight` calculation in line 375 and 380 because the line numbers you mentioned are in `computePrefWidth` method and changes are made to height calculation in `computePrefHeight` only. Since `textHeight` and `graphicHeight` each are used in 3 conditions from line no 386 to 394 I have kept the calculation in the beginning itself. On the other hand `textWidth` declared in line 368 might change in line 371, so I can't use `width` directly instead of defining `textWidth` as `width` need to be used while calling other graphic related methods. ------------- PR: https://git.openjdk.org/jfx/pull/996
