On Tue, 4 Apr 2023 18:17:55 GMT, Andy Goryachev <ango...@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
