On Sun, 12 Apr 2026 18:00:07 GMT, Marius Hanl <[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 classes (the JavaFX font
> classes do that as well)
> - I ren...
No ill effects as far as i can see with the Monkey Tester on macOS 26.3.1
I do see build errors in Eclipse:
Description Resource Type Path Location
GlyphLayout cannot be resolved to a type
TextLayoutUtilsContractTest.java Java Problem
/controls/src/test/java/test/javafx/scene/control/skin line 245
GlyphLayout cannot be resolved to a type
TextLayoutUtilsContractTest.java Java Problem
/controls/src/test/java/test/javafx/scene/control/skin line 277
GlyphLayout cannot be resolved to a type
TextLayoutUtilsContractTest.java Java Problem
/controls/src/test/java/test/javafx/scene/control/skin line 310
GlyphLayout cannot be resolved to a type
TextLayoutUtilsContractTest.java Java Problem
/controls/src/test/java/test/javafx/scene/control/skin line 342
The type com.sun.javafx.text.GlyphLayout is not accessible
TextLayoutUtilsContractTest.java Java Problem
/controls/src/test/java/test/javafx/scene/control/skin line 30
(I've merged the latest master branch before testing)
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);
}
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TextLayoutUtilsContractTest.java
line 53:
> 51: * @see test.com.sun.javafx.pgstub.StubTextLayout
> 52: */
> 53: class TextLayoutUtilsContractTest {
Renaming this test was a good idea.
I might suggest adding a comment explaining that since it's a headless test, we
are dealing with the StubToolkit and its simplified text layout.
-------------
PR Review: https://git.openjdk.org/jfx/pull/2145#pullrequestreview-4108070561
PR Review Comment: https://git.openjdk.org/jfx/pull/2145#discussion_r3081477371
PR Review Comment: https://git.openjdk.org/jfx/pull/2145#discussion_r3081418804
