On Tue, 25 Mar 2025 18:36:51 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Revert "fixed bad merge"
>>
>> This reverts commit 0e9e8ee63895bd1d976398587add5b96958d38aa.
>
> 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.
let's see if it works
> 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.
the problem is that we have `com.sun.javafx.text.TextLine implements
com.sun.javafx.scene.text.TextLine`
we probably should rename the concrete class to something like `PrismTextLine`
for clarity, but that will create a much larger diff.
> 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`?
Locale.ROOT is probably a better choice. the main reason is to avoid
unexpected conversion in non-English locales.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012989686
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012985556
PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r2012987712
