On Tue, 7 Feb 2023 19:47:50 GMT, Andy Goryachev <ango...@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 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. Previously both `textWidth` and `graphicWidth` were calculated regardless of the condition. In the updated code calculation of `graphicWidth` is moved to `else` block so that one unnecessary calculation is avoided. Since `textWidth` is used in 2 more conditions in `else` block, calculated it before the start of `if` block. Since `isIgnoreText()` does more than just checking for `ContentDisplay` value, further reordering `if-else` statements generates different result than expected. So this further leads to question previously raised by @hjohn, if `isIgnoreText()` and `isIgnoreGraphic()` should only check for ContetDisplay type or check for conditions like null/empty graphic or text as well. If this needs to be separated, again it gives raise to more conditions in the `computePrefWidth()` method. Added boolean in the beginning as suggested. ------------- PR: https://git.openjdk.org/jfx/pull/996
