RFR: 8306083: Text.hitTest is incorrect when more than one Text node in TextFlow [v9]

[Andy Goryachev]

On Thu, 12 Oct 2023 09:40:18 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> The text run selected in `PrismTextLayout::getHitInfo()` method for character index calculation was not correct when hitTest was invoked for Text node in a TextFlow with more than one Text child. Hence wrong character index value was calculated.
>> 
>> Since only x, y coordinates were available in the above mentioned method, sending the text as a parameter to this method is necessary so as to know if the text run selected for character index calculation is correct. Along with this change modified the `PrismTextLayout::getHitInfo()` method to calculate the correct character index.
>> 
>> Added tests to validate the changes.
>
> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix repeating text node issue

I think this looks good (with minor suggestions).  Tested with the MonkeyTester and with a rich text that contains multiple Text instances with exactly the same text.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 213:

> 211:      * @return returns a {@link Hit} object containing character index, insertion index and position of cursor on the character.
> 212:      */
> 213:     public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart);

please add new `@param` blocks to javadoc

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 458:

> 456:                 for (int i = 0; i < lineIndex; i++) {
> 457:                     for (TextRun r: lines[i].runs) {
> 458:                         if (r.getTextSpan() != null && r.getTextSpan().getText().equals(text) && r.getStart() >= textRunStart) {

speedup: you may want to compute `>=textRunStart` condition before `.equals(text)` to save CPU cycles (basically, swap the two conditions here)

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 466:

> 464:                 boolean isPrevNodeExcluded = false;
> 465:                 for (TextRun r: runs) {
> 466:                     if (!r.getTextSpan().getText().equals(text) || (r.getTextSpan().getText().equals(text) && r.getStart() < textRunStart)) {

same suggestion about swapping conditions to save some CPU cycles

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 763:

> 761:             if (!textFound) {
> 762:                 for (TextRun r : lines[index].runs) {
> 763:                     if (r.getTextSpan() == null || (r.getTextSpan().getText().equals(text) && r.getStart() == runStart)) {

and definitely here `==` should go before `.equals()`

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1032:

> 1030:             while (runIndex < runs.length - 1) {
> 1031:                 if (ptInParent.getY() > runs[runIndex].getLocation().y
> 1032:                         && ptInParent.getY() < runs[runIndex + 1].getLocation().y) {

[minor]: the formatting looks off.  If we rename 'ptInParent` to `p` it will fit on one line.
Also, ptInParent.getY() gets called twice, may be use a local variable?  Also will make it shorter.

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1039:

> 1037:         }
> 1038:         int textRunStart = runs.length == 0 ? 0 : ((TextRun) runs[0]).getStart();
> 1039:         int curRunStart = runs.length == 0 ? 0 : ((TextRun) runs[runIndex]).getStart();

very minor: we are computing run.length == 0 twice.  I know it might look neater than

int textRunStart;
int curRunStart;
if(runs.length == 0) {
  textRunStart = 0;
  curRunStart = 0;
} else {
 ...
}

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

PR Review: https://git.openjdk.org/jfx/pull/1157#pullrequestreview-1674985140
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357290999
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357293077
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357293685
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357294513
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357297816
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357301413