RFR: 8306083: Text.hitTest is incorrect when Text node is present in TextFlow [v5]

[Karthik P K]

On Wed, 16 Aug 2023 20:15:05 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix character index calculation issue when Text node content is wrapped
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 202:
> 
>> 200:     public boolean setTabSize(int spaces);
>> 201: 
>> 202:     public Hit getHitInfo(float x, float y, String text);
> 
> Perhaps we could add javadoc with an explanation for the 'text' parameter, that it's expected to be null in the case of TextFlow and non-null in the case of Text.

Added javadoc comment

> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 505:
> 
>> 503:                             charIterator.setText(text);
>> 504:                         } else {
>> 505:                             charIterator.setText(new String(getText()));
> 
> getText() is possibly an expensive operation when the layout contains a lot of text.
> Since we are looking only for the following "character", is it possible to optimize this and only use a subset of spans or limit the amount of text passed to the break iterator in some other way?
> What do you think?
> 
> (it's likely to be out of scope for this PR)

I believe it is not possible use only subset of spans because we are calculating insertion index using character index. If text length becomes less than character index then character iterator will fail to execute `following()` method.

> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 754:
> 
>> 752:         int index = 0;
>> 753:         float bottom = 0;
>> 754:         boolean isTextPresent = text == null ? true : false;
> 
> this looks very confusing to me... what text is present?

I agree that the naming and the conditionals were confusing. So optimized the code for better readability.

> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 775:
> 
>> 773:             bottom += lines[index].getBounds().getHeight() + spacing;
>> 774:             if (index + 1 == lineCount) bottom -= lines[index].getLeading();
>> 775:             if (bottom > y && isTextPresent) break;
> 
> I would recommend adding { }'s to ifs here and on line 774.

Added {} as suggested

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300143214
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300147306
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300149070
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300142907