On Thu, 28 Aug 2025 22:42:31 GMT, Phil Race <p...@openjdk.org> wrote:
>> `TextLayout` should deal more gracefully with zero length strings. Currently
>> the exception listed below is the one that is thrown.
>>
>> `new TextLayout("", f, new FontRenderContext(null, false, false));`
>>
>>> Exception in thread "main" java.lang.IllegalArgumentException: Zero length
>>> string passed to TextLayout constructor.
>>> at java.lang.Throwable.<init>(Compiled Code)
>>> at java.lang.Exception.<init>(Compiled Code)
>>> at java.lang.RuntimeException.<init>(Compiled Code)
>>> at java.lang.IllegalArgumentException.<init>(Compiled Code)
>>> at java.awt.font.TextLayout.<init>(Compiled Code)
>>> at test.main(Compiled Code)
>>
>> **REVIEWER NOTE:** Please check the empty-string `TextLayout` behavior
>> documented in `TextLayoutConstructorTest` carefully; a badly-behaving empty
>> `TextLayout` is probably worse than a `TextLayout` which doesn't allow empty
>> strings...
>
> src/java.desktop/share/classes/java/awt/font/TextLine.java line 839:
>
>> 837:
>> 838: if (result == null) {
>> 839: result = new Rectangle2D.Float(Float.MAX_VALUE,
>> Float.MAX_VALUE, Float.MIN_VALUE, Float.MIN_VALUE);
>
> That's interesting. I'd have assumed what you changed it to would be the
> answer in such. a case ? Why was it like this ? Perhaps it was never possible
> before and this was intended to 'flag a problem' ??
I also wondered this, and came to the conclusion that it probably simply
represents an impossibly-positioned, impossibly-sized rectangle that should
never overlap with anything -- similar to `getItalicBounds`, which are the
default bounds used in some of the `TextLayout` methods which are overloaded to
sometimes take a bounds parameter and sometimes not (e.g. `getCaretInfo`). If
this assumption is correct, then `(0, 0, 0, 0)` should also be OK.
> src/java.desktop/share/classes/java/awt/font/TextLine.java line 934:
>
>> 932:
>> 933: if (!requiresBidi) {
>> 934: requiresBidi = Bidi.requiresBidi(chars, 0, characterCount);
>
> This doesn't change anything, does it ? So why is the change made ? This -
> and white space changes - make it look like there are more changes than there
> really are.
Correct. One of the solutions I played around with during the experimental
phase involved creating a zero-length `TextLine` with one `TextLineComponent`
here by replacing any zero-length `chars` with a one-char array, but continuing
to report zero length to the supporting classes. We already store
`chars.length` in `characterCount`, so this bit of subterfuge was easier if
everyone used the variable instead of getting the actual array length again.
When I backed out this experiment, I left the variable use since it seemed
strange that we were mostly ignoring the variable here... but I can back out
the variable use too, if you want.
> src/java.desktop/share/classes/sun/font/TextLabelFactory.java line 122:
>
>> 120: int limit) {
>> 121:
>> 122: if (start > limit || start < lineStart || limit > lineLimit) {
>
> Why is this change needed ? I'm supposing that for zero length
> start==limit==0 but we get here because TextLine calls it in a do { ... }
> block so will call it at least once, and I wonder if that should change ? So
> why did you do it this way ?
Yes, part of it is the `do {} while` looping in
`TextLine.createComponentsOnRun`, but it also avoids having to add an
additional `pcm` null check in `TextLine.init` and a check for zero-length
`numComponents` in `TextLine.fastCreateTextLine`.
Furthermore, even with these additional defensive checks one layer up, we would
end up in a situation where we have an empty `TextLineComponent[]` and
therefore no metrics and no ascent / descent / leading to report (I've assumed
we don't want to report zero for these, since these numbers are consistent for
a given font / FRC regardless of the text being used). Then we'd also start to
have issues with `getCaretInfo` / `getCaretShape` / `getCaretShapes`, which
also rely on the `TextLine` metrics which we won't have with an empty
`TextLineComponent[]`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26947#discussion_r2314278179
PR Review Comment: https://git.openjdk.org/jdk/pull/26947#discussion_r2314278799
PR Review Comment: https://git.openjdk.org/jdk/pull/26947#discussion_r2314282455
