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

[Andy Goryachev]

On Tue, 8 Aug 2023 13:57:31 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 Text node was embedded in TextFlow. Hence wrong character index value was calculated for the same.
>> 
>> 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 character index calculation issue when Text node content is wrapped

The updated code works correctly, as far as I can tell.  Left some minor comments.
This PR definitely needs another pair of eyes.

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.

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)

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?

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.

modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java line 396:

> 394:         for (int i = 0; i < glyphCount; i++) {
> 395:             float advance = getAdvance(i);
> 396:             if (runX + advance >= x) {

this is very interesting.  how did it work before?
could there be any scenarios that might have failed before and are fixed with this change?

tests/system/src/test/java/test/robot/javafx/scene/TextCharacterIndexTest.java line 136:

> 134:     private void mouseClick(double x, double y) {
> 135:         Util.runAndWait(() -> {
> 136:             robot.mouseMove((int) (scene.getWindow().getX() + scene.getX() + x),

minor: local Window variable eliminates multiple invocations of getWindow()
(I am too lazy to see if javac is smart enough to optimizes it away)

tests/system/src/test/java/test/robot/javafx/scene/TextCharacterIndexTest.java line 153:

> 151:             textTwo = new Text("This is Text");
> 152:             textTwo.setFont(new Font(48));
> 153:             textFlow.getChildren().clear();

is this clear() a necessary part of the test?
setAll() "Clears the ObservableList and adds all the elements..." according to its javadoc

this comment applies here and in all other places (line 164, etc.)

tests/system/src/test/java/test/robot/javafx/scene/TextCharacterIndexTest.java line 403:

> 401:         // String testString = textOne.getText();
> 402:         // testString += textTwo.getText();
> 403:         // isSurrogatePair = Character.isSurrogate(testString.charAt(charIndex));

clean up?

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

PR Review: https://git.openjdk.org/jfx/pull/1157#pullrequestreview-1581344829
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296381587
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296389135
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296393704
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296379365
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296397150
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296399169
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296406718
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296408302