RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v4]

[Karthik P K]

On Wed, 17 Jan 2024 19:45:51 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> 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:
> 
I see this problem as well. I will check on this issue.

> 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' ?

Changed to `ltrIndex`

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

Agreed. I think allocating the `textBounds` before line 494 is better. Made this change

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

Created local variable to get Point2D value

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

Corrected indentation

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

PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1897964532
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1457049491
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1457049303
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1457047727
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1457047377