On Wed, 17 Jan 2024 10:54:46 GMT, Karthik P K <k...@openjdk.org> wrote:
>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation >> conditions were not considered, hence hit test values such as character >> index and insertion index values were incorrect. >> >> Added checks for RTL orientation of nodes and fixed the issue in >> `getHitInfo()` to calculate correct hit test values. >> >> Added system tests to validate the changes. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix multiline text insertion index calculation issue Noticed a problem - on Windows 11 and on macOS 14.2.1 hit into shows different values for Text and TextFlow. This issue can be seen with pretty much every text, even "aaa\nbbb\ncccc". In this screenshot, the line corresponds to the mouse position:  modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 213: > 211: * @param textRunStart Start position of first Text run where hit > info is requested. > 212: * @param curRunStart Start position of text run where hit info is > requested. > 213: * @param forTextFlow Indicates if the hit info is requested for > TextFlow or Text node. True for TextFlow and False for Text node. technically, it's "true" and "false", e.g. `{@code true}` modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 430: > 428: int insertionIndex = -1; > 429: int relIndex = 0; > 430: int LTRIndex = 0; minor: it's a bit confusing to have a local var start with an uppercase letter. maybe 'ltrIndex' ? modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 453: > 451: if (isMirrored) { > 452: int runIndex = -1; > 453: for (int i = runs.length - 1; i >=0; i--) { please add a space after `i >= ` modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 510: > 508: if (run.getTextSpan() != null && > run.getTextSpan().getText().equals(text)) { > 509: if ((x > run.getWidth() && !isMultiRunText) > || textWidthPrevLine > 0) { > 510: BaseBounds textBounds = new BoxBounds(); here (and on line 544) we allocate `textBounds` inside the for loop. Maybe we should allocate one before line 494 and use that instance in both for loops to avoid gc pressure? The downside is that in some cases it may not be used, but I feel it'll better since alloc is cheap. modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 521: > 519: } > 520: for (int j = runs.length - 1; j >= 0; j--) { > 521: if > (runs[j].getTextSpan().getText().equals(text) && runs[j].getStart() != > curRunStart) { minor optimization: do the int comparison first, equals second modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1062: > 1060: } else { > 1061: double ptX = localToParent(x, y).getX(); > 1062: double ptY = localToParent(x, y).getY(); localToParent(x, y) computed twice, could we avoid that? tests/system/src/test/java/test/robot/javafx/scene/RTLTextFlowCharacterIndexTest.java line 274: > 272: while (x > X_LEADING_OFFSET) { > 273: moveMouseOverTextFlow(x, Y_OFFSET); > 274: if (isLeading) { indentation? ------------- PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1896553605 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456279189 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456284396 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456341154 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456297979 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456299608 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456305703 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456326090
