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

[Karthik P K]

On Wed, 8 Feb 2023 19:57:46 GMT, Andy Goryachev <angorya at 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