On Thu, 6 Mar 2025 16:21:52 GMT, Andy Goryachev <[email protected]> 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
