RFR: 8246104: Some complex text doesn't render correctly on macOS [v4]

[Phil Race]

On Tue, 4 Apr 2023 18:17:55 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   formatting and make some fields private
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/font/FallbackResource.java line 257:
> 
>> 255:              */
>> 256:             if (PrismFontFactory.debugFonts) {
>> 257:                 System.err.println("\tToo many font fallbacks!");
> 
> would it be better to use logging?

I can't change just the new cases. And changing hundreds of other cases is out of scope.

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/FallbackResource.java line 376:
> 
>> 374:            s+= "Slot " + i + "=null\n";
>> 375:           } else {
>> 376:           s += "Slot " + i + "="+getSlotResource(i).getFullName()+"\n";
> 
> minor: spaces and indentation

fixing this

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/FallbackResource.java line 379:
> 
>> 377:           }
>> 378:         }
>> 379:         s+= "\n";
> 
> perhaps it might be better to use a StringBuilder here

I don't think it matters - performance isn't an issue.

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/FontFallbackInfo.java line 34:
> 
>> 32:    ArrayList<String> linkedFontFiles;
>> 33:    ArrayList<String> linkedFontNames;
>> 34:    ArrayList<FontResource> linkedFonts;
> 
> could these three be `private final`?

I will make them private

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 41:
> 
>> 39: class CTFontFile extends PrismFontFile {
>> 40: 
>> 41:     private long cgFontRef = 0;
> 
> minor: the assignment is unnecessary

I'd prefer to leave it.

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 89:
> 
>> 87:         if (name != null) {
>> 88:             String family = getFamilyName();
>> 89:             if (family.equals("System Font")) {
> 
> very minor: I'd do `System Font".equals(...)`

I do as you say when I'm worried about NPE. Here I'd quite like a NPE if this is null .. because there'd be something we'd need to investigate

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 104:
> 
>> 102:     public boolean isBold() {
>> 103:         // Need to do this until we add font variation support into the super-class
>> 104:         return fullName.equals("System Font Bold") || super.isBold();
> 
> same here.  I am sure fullName cannot be null

right it should not be possible.

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTGlyphLayout.java line 83:
> 
>> 81:             if (fr == null) return -1;
>> 82:             slot = fr.getSlotForFont(fontName);
>> 83:             if (slot == -1) {
> 
> minor: may be `< 0`?

We should never have -2 etc. If we do, then the code will fail and we'd know about it

> modules/javafx.graphics/src/main/java/com/sun/javafx/font/directwrite/DWFactory.java line 192:
> 
>> 190:         // CJK Ext B Supplementary character fallbacks.
>> 191:         info.add("MingLiU-ExtB", getPathNameWindows("mingliub.ttc"), null);
>> 192:         info.add("Segoe UI Symbol", getPathNameWindows("seguisym.ttf"), null);
> 
> a question: what if these fonts are *not* available in a particular system?  will the code break or it will be able to handle non-existing entry?

No harm. Its quite common on initial windows install to not have these fonts.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157780348
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157782480
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157783750
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157783971
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157775968
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157776846
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157777284
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157778059
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157778520