On Fri, 24 Mar 2023 21:37:16 GMT, Phil Race <p...@openjdk.org> wrote:
> This PR addresses some font problems on macOS. > (1) Garbled Arabic with the System font > (2) Non-ideal fallback fonts for all fonts > (3) No bold for System font. > > In particular the standard System Font was garbling Arabic text - random > glyphs from another font. > The root of this issue is that several releases ago macOS introduced UI fonts > that are hidden from > enumeration. must be loaded by a special API and cannot be loaded by name, > even if you extract > the name from the core text font loaded by that special API. > These fonts usually have a name that begins with "." such as the postscript > name ".AppleUISystemFont" > and "name" for the main instance of this will be "System Font Regular". These > live in files called "SFNS.ttf" > and "SFArabic.ttf" and similar. > > JavaFX has its own "System" font family logical name and in order to map to > the macOS system font, > several releases ago we started using the special API referenced above. > > Normally we have our own list of fall back fonts for other scripts. > But when we encounter complex scripts such as Arabic, and ask CoreText to > layout the glyphs, > it may use other system fonts for Arabic (eg ".SFArabic") and the FX code > dynamically adds > this new font to its fallback list - by name. > But this new font can't be directly referenced by name either, but that's > what used to work and the FX code tries. > > So to fix this we need to grab the reference to the font that was returned by > CoreText and use > that to create the PrismFont we add as a fall back. > The code that first recognises it needs to go this route is in > CTGlyphLayout.java when > "getSlotForFont(String name") fails. > Instead it calls new code in CTFactory which in turn calls a new constructor > and supporting code in CTFontFile.java. > There's also supporting changes in native code - coretext.c > > Additionally to support that when we ask the platform to enumerate fallbacks, > we might get such "." fonts, > we need to make some more changes > Now on to more "fallback" enhancements. > Prior to this fix, on macOS we had a short, hard-coded global list. > This fix calls the macOS API to get the cascading fallback list for a font. > This improves the fallback behaviour in respect to providing > (1) More appropriate styles, (2) More supported code pointd, (3) Code point > support coming from a preferred font. > There was some desirable re-factoring as part of this, as the location of > fallbacks was pushed from > "if <windows> else if <linux> else <mac> in shared code, down into the > appropriate platform factory > > This also made it easier to do some required changes to allow fallbacks to be > pre-initialised with the > native font resource, not just a name+ file. The new class FontFallbackInfo > encapsulates this and changes > were made to follow it. > In practice on macOS it seems that some "." cascading fallbacks for the "." > system font come with a "NULL" > file name. Without being able to identify the file we aren't able to use the > font to skip them. > More changes would be needed to provide a PrismFont subclass that can handle > this case. > Fortunately that isn't happening for any of the fallbacks we get from complex > text layout. > > The final thing this fixes is that "System Bold" was the same as "System > Regular". > The needed name getting lost along the way to constructing the native font. > > The work done here also highlighted that we really need to add code that at > least recognises OpenType variation fonts. > And some day after that we could expose API that allows apps to request > non-named variations. some minor comments; overall - a great improvement. thank you, Phil! I want to share two observations that came out of testing with a RichTextArea prototype. 1. I've got java.lang.OutOfMemoryError: Java heap space UNSUPPORTED (log once): POSSIBLE ISSUE: unit 0 GLD_TEXTURE_INDEX_2D is unloadable and bound to sampler type (Float) - using zero texture because texture unloadable <img width="1199" alt="Screenshot 2023-04-04 at 07 55 50" src="https://user-images.githubusercontent.com/107069028/229888096-277e5257-193c-4c13-b32d-2ae7f012d792.png"> I suspect it happened due to instrumentation and/or a breakpoint (I was running in Eclipse debugger and Scenic View 11.0.2 attached). I can't reproduce the issue anymore. 2. the following sequence in the RichTextAreaDemo produces an unexpected result: - type "bold plain", select "bold" and make it bold, select all set size 300%, change font to Courier New Result: the bold segment changes to regular and the regular changes to bold. It's highly probably I have a bug in my code somewhere, mentioning it here just in case. 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? 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 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 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`? 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 modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 65: > 63: } > 64: > 65: private long ctFontRef = 0; minor: I'd add a blank line, remove the assignment. 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(...)` 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 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`? 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? ------------- Marked as reviewed by angorya (Committer). PR Review: https://git.openjdk.org/jfx/pull/1067#pullrequestreview-1371535673 PR Comment: https://git.openjdk.org/jfx/pull/1067#issuecomment-1496439369 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157609552 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157611498 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157612031 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157613189 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157624589 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157625222 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157625859 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157626171 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157627666 PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157630093