RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v11]
John Hendrikx
jhendrikx at openjdk.org
Wed Feb 14 05:32:08 UTC 2024
On Fri, 9 Feb 2024 07:59:47 GMT, Karthik P K <kpk at 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:
>
> Review comments
I think we should simplify the `getHitInfo` method in the `TextLayout` interface.
The code currently seems to be making distinctions where there are none. `TextFlow`s provide spans, while `Text`s provide a text. `getHitInfo` can take advantage of this to avoid the `text` and `forTextFlow` parameters altogether. This will reduce confusion (as there is no distinction) and also avoids cases that are non-sensical (providing `text` while `forTextFlow` is `true` or vice versa).
Previous changes to this code (when the parameter `text` was introduced to `getHitInfo`) should probably be partially undone and simplified with this new knowledge.
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. {@code true} for TextFlow and {@code false} for Text node.
I have the impression that we're overcomplicating things here. There is a flag (`forTextFlow`) for Text/TextFlow, and a String (`text`) for Text/TextFlow, and there are already `setContent` methods for Text/TextFlow.
I don't see a need for any of these changes to `getHitInfo` at all.
If the content was set with `setContent(TextSpan[] spans)`, then we know it is a TextFlow (actually we don't care, we have spans which is the difference we're interested in). This fact can be checked at any time with:
boolean isThisForTextFlow = this.spans != null;
See how the `setContent` methods work; they either set `spans` or they don't. The rest of the code is already full of alternate paths with `if (spans == null) { /* do Text stuff */ } else { /* do TextFlow stuff */ }`
The `text` parameter is also already known, because this is explicitely set for `Text` nodes because they use `setContent(String text, Object font)`. However, before using it (specifically for `Text`), make sure that check `spans == null` as it is filled for `TextFlow` as well at a later point.
So, I think:
- the `text` parameter should never have been added (it wasn't until recently)
- `forTextFlow` flag is unnecessary, just check `spans != null`.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 424:
> 422:
> 423: @Override
> 424: public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart, boolean forTextFlow) {
This method has become huge, with I think upto 7 levels of nesting. It would benefit from some splitting.
Suggestions in that area:
- Split it in one that handles RTL and one for LTR **or**
- Split it in one that handles `spans != null` (`TextFlow`) and one that handles `Text`
You can also reduce the nesting of the first `if/else` by returning early:
if (lineIndex >= getLineCount()) {
charIndex = getCharCount();
insertionIndex = charIndex + 1;
return new Hit(charIndex, insertionIndex, leading); // return early here
} else { // no need to nest 150+ lines of code
More suggestions inline below
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 432:
> 430: int ltrIndex = 0;
> 431: int textWidthPrevLine = 0;
> 432: float xHitPos = x;
This variable seems important, yet is only used in the RTL+Text branches.
It seems that this variable can also be easily calculated as:
originalX - (getMirroringWidth() - x) + bounds.getMinX
This calculation can be done in the final RTL+Text branch near the end, no need to precalculate it IMHO
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 438:
> 436: if (lineIndex >= getLineCount()) {
> 437: charIndex = getCharCount();
> 438: insertionIndex = charIndex + 1;
By returning early here, you can avoid the `else` and save a lot of nesting.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 440:
> 438: insertionIndex = charIndex + 1;
> 439: } else {
> 440: if (isMirrored && (forTextFlow || spans == null)) {
The extra condition here adds nothing of value, the original `if` was correct.
Because `spans == null` means "for Text", and `forTextFlow == true` means "for TextFlow". Since it always is either a `TextFlow` or a `Text` the `||` always evaluates to `true` -- unless of course you pass in this flag incorrectly (again, I think the flag should be removed).
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 448:
> 446: TextRun run = null;
> 447: //TODO binary search
> 448: if (text == null || spans == null) {
`text` now refers to the `text` parameter here, and not to the original `text` field -- as discussed earlier, I think you shouldn't be passing in `text` as argument at all.
Furthermore, I think the original code was also flawed; `text` is only `null` for a short while, and it is never `null` when `spans == null`. In other words, this `if` should IMHO be:
Suggestion:
if (spans == null) { // not a TextFlow, must be Text
Use `getText()` function to get the value of the text (it will just use the one from `Text` set via `setContent(String text, Object font)` or it will calculate it for `TextFlow` from the spans set via `setContent(TextSpan[] spans)`;
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 468:
> 466: for (int i = 0; i < runIndex; i++) {
> 467: xHitPos -= runs[i].getWidth();
> 468: }
This code modifies the `x` parameter that was passed in as an argument, making it harder than necessary to reason about it (I have to talk about "original x" etc), however, I think this loop, and `runIndex` is not necessary, because `xHitPos` can just be calculated directly (see comment on `xHitPos`).
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 567:
> 565: int[] trailing = new int[1];
> 566: if (text != null && spans != null) {
> 567: charIndex = run.getOffsetAtX(x, trailing);
Here also: `text` is no longer referring to the field `text` but to the argument `text`. I also again think the check is wrong. It should just be:
if (spans != null) { // must be a TextFlow
There are more oddities in the code below (which you didn't change this round -- see my comments inline:
// !! getText() ***never*** returns `null`, the check is not needed
if (getText() != null && insertionIndex < getText().length) {
if (!leading) {
BreakIterator charIterator = BreakIterator.getCharacterInstance();
// before, when `text` was a field, this could never be null here
if (text != null) {
charIterator.setText(text);
} else {
charIterator.setText(new String(getText()));
}
int next = charIterator.following(insertionIndex);
if (next == BreakIterator.DONE) {
insertionIndex += 1;
} else {
insertionIndex = next;
}
}
} else if (!leading) {
insertionIndex += 1;
}
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 576:
> 574: int indexOffset;
> 575: if (isMirrored) {
> 576: indexOffset = run.getOffsetAtX(xHitPos, trailing);
Here is the only place where `xHitPos` is used again (the RTL + Text case). I think it can just be calculated (you need to save off the original `x` value, or even better, don't modify the `x` argument but use a different variable for the `x` calculation).
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1035:
> 1033: curRunStart = ((TextRun) runs[runIndex]).getStart();
> 1034: }
> 1035: TextLayout.Hit h = layout.getHitInfo((float)x, (float)y, getText(), textRunStart, curRunStart, false);
I think `getText()` as a parameter is not needed here (it is passed at line 260 when it calls `setContent`). Also, I think this should be `getTextInternal()` -- see the comment in that method.
I also think the `false` is not needed, see comments on the interface.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1050:
> 1048:
> 1049: private int findRunIndex(double x, double y, GlyphList[] runs) {
> 1050: int ix = 0;
The naming leaves something to be desired. `ix` is the run index is it not? Maybe leave it as `runIndex` ?
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1061:
> 1059: while (ix < lastIndex) {
> 1060: GlyphList r = runs[ix];
> 1061: GlyphList nr = runs[ix + 1];
Suggestion:
GlyphList run = runs[ix];
GlyphList nextRun = runs[ix + 1];
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1075:
> 1073: double ptY = ptInParent.getY();
> 1074: while (ix < lastIndex) {
> 1075: GlyphList r = runs[ix];
Suggestion:
GlyphList run = runs[ix];
modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 202:
> 200: double x = point.getX();
> 201: double y = point.getY();
> 202: TextLayout.Hit h = layout.getHitInfo((float)x, (float)y, null, 0, 0, true);
The `null` and `true` here are not needed IMHO (what would even happen when they conflict, `null` + `false`, or non-`null` + `true`?)
tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 107:
> 105: */
> 106:
> 107: public class RTLTextCharacterIndexTest {
This is a system test, which don't run with the build system. Are you sure this will work on all platforms? I don't see a specific `Font` used, which means `X_LEADING_OFFSET` may be incorrect when the platform is supplying a different font.
tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 121:
> 119:
> 120: final int Y_OFFSET = 30;
> 121: final int X_LEADING_OFFSET = 10;
If they're constant, they should also be `static`, otherwise may want to write them with lower case.
-------------
Changes requested by jhendrikx (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1879325091
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488868375
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488870805
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488873469
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488871928
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488878650
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488876272
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488858423
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488882538
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488881462
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488888035
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488889561
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488889939
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488890087
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488891380
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488892493
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1488894397
More information about the openjfx-dev
mailing list