On Fri, 30 Aug 2024 12:00:35 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> This PR adds a couple of null checks to `LogicalFont` and `FTFactory`, that >> make use of `FontConfigManager::getFontConfigFont`, which under certain >> cases, can return null. >> >> I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using >> `-Dprism.useFontConfig=false`. >> >> On Ubuntu, I've manually added some font files to the JDK path >> `$JAVA_HOME/lib/fonts` (which didn't exist), and the test passes now, though >> this message is still printed out: >> >> Error: JavaFX detected no fonts! Please refer to release notes for proper >> font configuration >> >> which is confusing to me, because fonts where found in the JDK path after >> all, and even in the case that there were no fonts found, "the release >> notes" is an ambiguous reference for the user. >> >> Also, instead of adding fonts to the JDK, I tested adding a >> `logicalfonts.properties` file with `-Dprism.fontdir` and without >> `fontConfig`, but this case was already working before the patch for this >> PR, and still works after it. >> >> Note that if there are no fonts in the JDK path, and `prism.fontdir` is not >> provided, without `fontConfig`, after this PR, there will still be an >> exception: >> >> >> Caused by: java.lang.NullPointerException: Cannot invoke >> "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return >> value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null >> at >> javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456) >> at >> javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461) >> at >> javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79) >> at >> javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139) >> at >> javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303) >> at >> javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258) >> at >> javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576) >> ... >> >> >> because `LogicalFont::getSlot0Resource` will return null. Adding null checks >> after this point will require many changes >> (`LogicalFont::getSlotResource(0)` is used in many places). Shouldn't we >> fail gracefully after `LogicalFont::getSlot0Resource` if `slot0FontResource` >> is finally null? > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Use isLinux in FTFactory instead of null check Marked as reviewed by prr (Reviewer). Sorry, I think I was out the week you updated it and then it disappeared off my radar. Seems like a reasonable compromise ------------- PR Review: https://git.openjdk.org/jfx/pull/1546#pullrequestreview-2355080538 PR Comment: https://git.openjdk.org/jfx/pull/1546#issuecomment-2400404424
