On Wed, 16 Aug 2023 20:32:54 GMT, Andy Goryachev <ango...@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
