On Wed, 31 Jan 2024 10:24:20 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 issue with multiline text Almost works! At least all tests where only text segments are present: bidi, RTL, mixed text, multi line. It does fail when a Node is added to TextFlow, will explain in a separate comment. Inline Nodes mess up computation. In this example, the Text.hitInfo shows bogus values anywhere in the word "trailing":  modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 513: > 511: if ((x > run.getWidth() && (!isMultiRunText > || run.getStart() == curRunStart)) || textWidthPrevLine > 0) { > 512: getBounds(run.getTextSpan(), textBounds); > 513: x -= (runs[0].getLocation().x - > textBounds.getMinX()); suggestion: we are still in the for loop, so perhaps it makes sense to extract `(runs[0].getLocation().x - textBounds.getMinX());` to a variable outside of the loop modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 517: > 515: for (int j = runs.length - 1; j > i; j--) { > 516: if (runs[j].getStart() != curRunStart && > runs[j].getTextSpan().getText().equals(text) && !runs[j].isLinebreak()) { > 517: ltrIndex += runs[j].getLength(); runs[j] is repeated 4 times... should it be a local variable? ------------- PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1854843704 PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1920000439 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1473468220 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1473469342
