On Tue, 14 Apr 2026 18:07:18 GMT, Andy Goryachev <[email protected]> wrote:
>> This one took me actually some hours to fully understand.
>> I recommend reading the ticket description made by Phil as it already
>> explains most of it.
>>
>> What is the problem (in `Utils`):
>> - `Utils.computeTextWidth(..)` does not initialize the `boundsType`. As a
>> consequence, we do not cache the layout result
>> - For the text width, it makes sense to set it, so the result is cached.
>> Note that the `boundsType` does not matter for the width calculation
>> itself, but we really want it set so the result will be cached if possible
>> - At one point due to `Utils.computeTextHeight(..)` method call, the
>> `boundsType` is initialized. Now calls to `Utils.computeTextWidth(..)` do
>> cache the result
>> - So in order to fix that, `Utils.computeTextWidth(..)` does now set the
>> the property as well
>>
>> Ok, but why only cache this specific usecase (Phil was also wondering about
>> that) (in `PrismTextLayout`):
>> - First of all, the previous method name was confusing (me).
>> What it actually does:
>> - decide if the layout result should be cached
>> - when we have a cache hit, if we can reuse it completely or ONLY the text
>> runs
>> - So what `PrismTextLayout` does is to cache the layout result for simple
>> cases with the default setting.
>> - This is biased towards modena, which specifies `-fx-bounds-type:
>> logical_vertical_center;` for ever `Text` node
>> - If one of those setting or the bounds type do not match, we will not
>> cache the text layout or reuse it completely, but the text runs instead
>> - That means the logic is fine and there's a thought behind it: Only cache
>> if the settings match those we are most likely to receive from the
>> application
>> - In case we have already a cache result but for example the `wrapWidth`
>> or the `boundsType` is different, we can reuse the runs but not the whole
>> cached layout result
>> - So we should not cache results with `boundsType = 0`, as those will have
>> another height than with `boundsType = BOUNDS_CENTER`
>> - So the logic is unchanged there
>>
>> As `StubTextLayout` extends `PrismTextLayout`, we can actually write
>> meaningful tests:
>> - So I wrote a bunch that make clear how the cache works and when a
>> `GlyphLayout` is created (due to a complete cache miss)
>> - Note that the tests are green before and after this PR
>> - Because `PrismTextLayout.setContent(..)` will request the font strike (for
>> `hashCode` calculation for the layout result cache), I needed to implement
>> `hashCode` and `equals` for some of the stub test cla...
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java
> line 172:
>
>> 170: layout.setWrapWidth((float)wrappingWidth);
>> 171: layout.setLineSpacing(0);
>> 172: layout.setBoundsType(TextLayout.BOUNDS_CENTER);
>
> this change makes sense, but perhaps we should delegate to
> `computeTextHeight(Font font, String text, double wrappingWidth, double
> lineSpacing, TextBoundsType boundsType)` to eliminate parallel paths?
>
> just like we do in `computeTextHeight(Font font, String text, double
> wrappingWidth, TextBoundsType boundsType)` just below it?
>
> that is,
>
>
> public static double computeTextWidth(Font font, String text, double
> wrappingWidth) {
> return computeTextHeight(font, text, wrappingWidth, 0,
> TextLayout.BOUNDS_CENTER);
> }
But then we would return the height? Which is wrong, as it is not the width?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2145#discussion_r3081746427
