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

[Karthik P K]

On Wed, 16 Aug 2023 20:32:54 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/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?

Without this condition when `runX + advance` becomes equal to `x`, character index calculated will be wrong. 
For example if we have 2 emojis (😀😃), when we move from first emoji to second, character index returned will be 1 instead of 2 when we just hit the second emoji. This is a failing scenario.

> 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)

Made changes as suggested. As far as I checked, javac does not optimize this.

> 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.)

Yes clear is not required. Removed from all instances.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300156444
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300157259
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1300157792