RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v4]
Phil Race
prr at openjdk.org
Fri Jun 2 18:40:17 UTC 2023
On Fri, 2 Jun 2023 07:19:01 GMT, Karthik P K <kpk at openjdk.org> wrote:
>> The main issue is that insertion index was not computed in the PrismTextLayout, supposedly to allow for delaying the computation until it's needed (if needed at all). A good intention, but the execution had two issues:
>> 1. HitInfo obtained in TextFlow had its 'text' field set to null (TextFlow:204), disabling any possibility of computing the index correctly
>> 2. The code that computed the index had an issue in that if an exception happened, it left the object in an inconsistent state, leading to other problems down the road, and more exceptions (incl. AIOB)
>>
>> What we did here is to compute the insertion index correctly at the source.
>> I've also checked that in the case of an empty line the insertion index is correct in both cases (at the last line and in the middle of a document).
>
>> Assuming line 473 is the one still there today, it looks to me as if that would be reached if you had the caret on an empty line that isn't the last line.
>
> In this case also code hits the condition in 451 as the text run is not null.
>
>>it seems to me that this new code has been pretty much copied from the supposedly buggy code there ..
> I'm not sure that code is actually buggy (comments over in that bug), rather that the instance was constructed
> with bad data.
>
> Like Andy said, since text was null in `HitInfo::getInsertionIndex()` method in the case of TextFlow, correct insertion index was not calculated. I had to use pretty much same code in PrismTextLayout as I had to consider all the cases especially the U+FE0F Variation Selector-16 characters.
I did note that TextFlow passed null which really looked like a WIP to me but its a separate issue.
The exception in 8302511 which is the entire subject of that bug isn't possible if text == null.
My point is that if you pass an incorrect charIndex to BreakIterator.following() you'll get the same exception
in your copy of that code. So how are you making sure you never do that ?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1214692324
More information about the openjfx-dev
mailing list