On Fri, 9 Feb 2024 07:59:47 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:
>
> 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
