RFR: 8342565: [TestBug] StubTextLayout [v9]

[Kevin Rushforth]

On Thu, 6 Mar 2025 16:21:52 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Changed the StubTextLayout to use product PrismTextLayout with much simplified glyph layout (via stubbed fonts).  The new layout assumes all the glyphs are squares of font size, while the bold type face produces wider glyphs (by 1 pixel).  The default font size has changed from 10 to 12 to make it closer to win/linux.
>> 
>> This brings the test environment closer to the product configuration and expands the capabilities of our headless testing pipeline, which will be useful for upcoming behavior tests.
>> 
>> Existing tests have been adjusted/reworked mainly due to default font size change.
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert "fixed bad merge"
>   
>   This reverts commit 0e9e8ee63895bd1d976398587add5b96958d38aa.

Looks good. I left a few questions and a couple suggestions. I'll reapprove if you decide to change anything.

modules/javafx.controls/src/test/java/test/javafx/scene/chart/StackedBarChartTest.java line 116:

> 114:         String bounds = computeBoundsString((Region)childrenList.get(0), (Region)childrenList.get(1),
> 115:                 (Region)childrenList.get(2));
> 116:         assertEquals("10 440 216 34 236 397 216 77 461 345 216 129 ", bounds);

I presume these changes are needed because they are also hard-coded values that are changing with the StubToolkit changes?

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 100:

> 98: import test.com.sun.javafx.scene.control.test.RT_22463_Person;
> 99: 
> 100: /**

Minor: We don't typically use javadoc-style comments in the class docs of test classes, but since we have a global RFE to address this, I don't think it matters all that much.

modules/javafx.graphics/src/main/java/com/sun/javafx/text/GlyphLayoutManager.java line 42:

> 40:  */
> 41: public class GlyphLayoutManager {
> 42:     private static GlyphLayout REUSABLE_INSTANCE = newInstance();

This can be final.

modules/javafx.graphics/src/main/java/com/sun/javafx/text/GlyphLayoutManager.java line 43:

> 41: public class GlyphLayoutManager {
> 42:     private static GlyphLayout REUSABLE_INSTANCE = newInstance();
> 43:     private static final AtomicBoolean GUARD = new AtomicBoolean(false);

Minor: "IN_USE" seems like a better name (and is what the original code used).

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 57:

> 55:  * Prism TextLayout
> 56:  */
> 57: public abstract class PrismTextLayout implements TextLayout {

Minor: Instead of making this abstract, did you consider leaving it concrete, overriding the methods you need in StubTextLayout? It's fine to leave the change as you have it.

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayoutFactory.java line 37:

> 35:     /* Same strategy as GlyphLayout */
> 36:     private static final TextLayout REUSABLE_INSTANCE = FACTORY.createLayout();
> 37:     private static final AtomicBoolean GUARD = new AtomicBoolean(false);

Minor: IN_USE might be a better name.

modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java line 45:

> 43:     int script;
> 44:     TextSpan span;
> 45:     com.sun.javafx.scene.text.TextLine line;

I recommend an import rather than listing the fully-qualified class here and below.

modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontResource.java line 99:

> 97:         if (bold == null) {
> 98:             String name = font.getStyle();
> 99:             bold = name.toLowerCase(Locale.US).contains("bold");

Should this be `Locale.ROOT` or does it really need to be `Locale.US`?

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

Marked as reviewed by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1667#pullrequestreview-2714620831
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012620345
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012660760
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012603455
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012605436
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012731466
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012734374
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012742406
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012853062