RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]
Karthik P K
kpk at openjdk.org
Mon Feb 6 14:26:15 UTC 2023
On Fri, 3 Feb 2023 18:04:09 GMT, Andy Goryachev <angorya at openjdk.org> wrote:
>> 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.
>
> Both `computePrefHeight()` and `computePrefWidth` can be restructured to avoid unnecessary computation (especially since these are very popular objects).
>
> My point is that, for example, textWidth on line 375 (used to compute textHeight:375) is not used if isIgnoreText() == true.
>
> Similarly, graphicHeight:380 is not used if isIgnoreGraphic() == true, line 386.
>
> Same optimization can be applied to computePrefWidth():315
>
> Just a suggestion, really.
Understood.
I have optimized the code. Please take a look.
Did following optimizations in the code:
- Optimized `if else` conditions by moving the `graphicHeight` and `graphicWidth` value calculation inside the else statement in `computePrefHeight` and `computePrefWidth` methods respectively.
- Removed unused `graphic` variable.
- In `computePrefHeight` method, since a `if` condition was already present which was same as the one used for `padding` calculation, moved `padding` calculation code to the same location.
- In `computePrefWidth` method, replaced multiple return statements with single statement for better readability and did minor optimizations.
-------------
PR: https://git.openjdk.org/jfx/pull/996
More information about the openjfx-dev
mailing list