On Wed, 8 Feb 2023 19:57:46 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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.
>
> no, what I meant is try to make sure that any operations with text (and some
> other calls) are made only when they are needed.
>
> For example, this code avoids calling anything to do with text and font when
> isIgnoreText == true.
>
> I also noticed that there is a field called `textWidth`; to avoid confusion I
> renamed the local variable from textWidth to `txWidth`.
>
>
> /** {@inheritDoc} */
> @Override
> protected double computePrefWidth(double height, double topInset, double
> rightInset, double bottomInset,
> double leftInset) {
> // Get the preferred width of the text
> final Labeled labeled = getSkinnable();
>
> boolean isIgnoreText = isIgnoreText();
> double widthPadding = leftInset + rightInset;
>
> double txWidth;
> if (isIgnoreText) {
> txWidth = 0.0;
> } else {
> widthPadding += (leftLabelPadding() + rightLabelPadding());
>
> String cleanText = getCleanText();
> boolean emptyText = cleanText == null || cleanText.isEmpty();
> if (emptyText) {
> txWidth = 0.0;
> } else {
> Font font = text.getFont();
> txWidth = Utils.computeTextWidth(font, cleanText, 0);
> }
> }
>
> double width;
> if (isIgnoreGraphic()) {
> width = txWidth;
> } else {
> // Fix for RT-39889
> double grWidth = graphic == null ? 0.0
> : Utils.boundedSize(graphic.prefWidth(-1),
> graphic.minWidth(-1), graphic.maxWidth(-1));
>
> if (isIgnoreText) {
> width = grWidth;
> } else {
> ContentDisplay cd = labeled.getContentDisplay();
> if (cd == ContentDisplay.LEFT || cd == ContentDisplay.RIGHT) {
> width = txWidth + labeled.getGraphicTextGap() + grWidth;
> } else {
> width = Math.max(txWidth, grWidth);
> }
> }
> }
>
> return width + widthPadding;
> }
>
>
> I suggest we apply a similar treatment to the second method being modified.
Updated `computePrefWidth()` method.
Please refer to my comment at the end regarding `computePrefHeight()` method
optimization observations.
>> Since `clenText` is used in `textHeight` calculation, this cannot be avoided
>> before the `if-else` block as `if-else` block is not re-ordered now. Please
>> refer to the previous comment for the details on `computePrefHeight()`
>> method.
>
> please see my earlier code sample, it is possible.
> cleanText is only needed to get the textWidth, and that is only needed when
> isIgnoreText == false.
> Also note that `textWidth` here is a local and hides an instance field with
> the same name. Rename the local?
Changed local variable name.
Please refer to my comment at the end regarding `computePrefHeight()` method
optimization.
-------------
PR: https://git.openjdk.org/jfx/pull/996
