RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v4]

[Andy Goryachev]

On Wed, 8 Feb 2023 12:46:15 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> 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.

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.

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 365:
>> 
>>> 363:         }
>>> 364: 
>>> 365:         String cleanText = getCleanText();
>> 
>> cleanText code block is unnecessary  when isIgnoreText() == true
>
> 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?

-------------

PR: https://git.openjdk.org/jfx/pull/996