On Fri, 22 Mar 2024 22:32:25 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> In https://github.com/openjdk/jfx/pull/1405, I identified some shortcomings >> of the stub font implementation. As I don't want to clutter the PR with >> that, I decided to cherrypick the improvements I did to a new ticket and PR. >> >> The current implementation has the following shortcomings: >> - It does not reliable detect the System Font, as a consequence, tests in >> TableColumnHeaderTest.java are failing on my local machine >> - Another consequence of this is, that the font size is always estimated >> with 0, as it is not detected >> - One shortcoming currently is, that the stub font siie estimate is not >> considering bold fonts. That would improve writing tests for some scenarios, >> e.g. for TableColumnHeader, where we would expect that the size of the >> header is bigger since it is bold >> >> Some tests were failing for the following reasons: >> - `AreaChartTest.java` - `expected -30.0, was -30.00000000004` - I added >> ceiling to the data. >> - `StackedBarChartTest.java` - since we now calculate correctly, the path >> changed >> - A test tried to load `Helvetica`, which is not supported in the stub font >> loader. I changed it >> - The default System font is considered a `Regular` one (style) >> >> I wrote tests and documented the stub behaviour. >> I did some minor changes here: >> - System font is now detected, also in bold and italic >> - A bold font will be calculated with a little bit more width (1px). >> Checkout the test as well for that >> >> Note: This only changes test setup, no 'production' code. > > modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java > line 76: > >> 74: FontHelper.setNativeFont(font, nativeFont, >> font.getName(), "Amble LtCn", "Regular"); >> 75: case "amble light condensed italic" -> >> 76: FontHelper.setNativeFont(font, nativeFont, >> font.getName(), "Amble LtCn", "Italic"); > > does it make sense to handle/log the default case, or this is indeed the > desired code (i.e. a no-op)? Right now, if we try to load a font that is not known by the test setup, the font will not have a 'native font' since no branch will get executed. With that, the width calculation will return 0. Just checked the Prism font code, there the code will fallback to the system font if it can not load the specified font (family). That would also be an idea here, not sure if it will have consequences on some tests. Also okay for me. What do you think? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1538339248
