This one took me actually some hours to fully understand.
I recommend reading the ticket description made by Phil as it already explains
most of it.
What is the problem (in `Utils`):
- `Utils.computeTextWidth(..)` does not initialize the `boundsType`. As a
consequence, we do not cache the layout result
- For the text width, it makes sense to set it, so the result is cached.
Note that the `boundsType` does not matter for the width calculation itself,
but we really want it set so the result will be cached if possible
- At one point due to `Utils.computeTextHeight(..)` method call, the
`boundsType` is initialized. Now calls to `Utils.computeTextWidth(..)` do cache
the result
- So in order to fix that, `Utils.computeTextWidth(..)` does now set the the
property as well
Ok, but why only cache this specific usecase (Phil was also wondering about
that) (in `PrismTextLayout`):
- First of all, the previous method name was confusing (me).
What it actually does:
- decide if the layout result should be cached
- when we have a cache hit, if we can reuse it completely or ONLY the text
runs
- So what `PrismTextLayout` does is to cache the layout result for simple cases
with the default setting.
- This is biased towards modena, which specifies `-fx-bounds-type:
logical_vertical_center;` for ever `Text` node
- If one of those setting or the bounds type do not match, we will not
cache the text layout or reuse it completely, but the text runs instead
- That means the logic is fine and there's a thought behind it: Only cache if
the settings match those we are most likely to receive from the application
- In case we have already a cache result but for example the `wrapWidth` or
the `boundsType` is different, we can reuse the runs but not the whole cached
layout result
- So we should not cache results with `boundsType = 0`, as those will have
another height than with `boundsType = BOUNDS_CENTER`
- So the logic is unchanged there
As `StubTextLayout` extends `PrismTextLayout`, we can actually write meaningful
tests:
- So I wrote a bunch that make clear how the cache works and when a
`GlyphLayout` is created (due to a complete cache miss)
- Note that the tests are green before and after this PR
- Because `PrismTextLayout.setContent(..)` will request the font strike (for
`hashCode` calculation for the layout result cache), I needed to implement
`hashCode` and `equals` for some of the stub test classes (the JavaFX font
classes do that as well)
- I renamed the `StubFontContractTest` to a better name since we do test a bit
more now
- And back in 2024 when I wrote this test class, I forgot the copyright
header 🙃. Added now
Can we improve more?
- The ticket suggests that we could cache more in `LabeledSkinBase`. There are
also some TODOs in this class. This could be a potential follow-up.
---------
- [x] I confirm that I make this contribution in accordance with the [OpenJDK
Interim AI Policy](https://openjdk.org/legal/ai).
-------------
Commit messages:
- 8301283: Util methods for computing text/width height giving up some
performance
Changes: https://git.openjdk.org/jfx/pull/2145/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=2145&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8301283
Stats: 618 lines in 7 files changed: 398 ins; 207 del; 13 mod
Patch: https://git.openjdk.org/jfx/pull/2145.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/2145/head:pull/2145
PR: https://git.openjdk.org/jfx/pull/2145
