On Wed, 28 Feb 2024 12:18:08 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:
>
> Simplified approach
Tested with the MonkeyTester, using different justification (left, right,
center, justify) and node orientation, for both Text and TextFlow. Multiple
Text instances, rich text, inline nodes, bidi, various line breaking scenarios.
I could not find any issues! The new code also looks much cleaner and much,
much easier to understand. I left a few non-essential suggestions inline.
Overall, thank you @karthikpandelu and @hjohn. Good job guys!
P.S. Tested on on macOS 14.3.1. The results on Windows/Linux should be
identical, but if we can get some testing done on these two platforms that
would be nice.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
line 212:
> 210: * and non-null in the case of {@link
> javafx.scene.text.Text}
> 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.
please remove these three parameters (text, textRunStart, curRunStart)
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 440:
> 438: TextRun run = null;
> 439: x -= bounds.getMinX();
> 440: //TODO binary search
we can remove this TODO because the new approach is incompatible with the
binary search
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 443:
> 441: for (int i = 0; i < runs.length; i++) {
> 442: run = runs[i];
> 443: if (x < run.getWidth()) break;
I would strongly recommend placing `break`s on separate lines for convenience
during debugging.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 445:
> 443: if (x < run.getWidth()) break;
> 444: if (i + 1 < runs.length) {
> 445: if (runs[i + 1].isLinebreak()) break;
same here
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 710:
> 708: while (index < lineCount) {
> 709: bottom += lines[index].getBounds().getHeight() + spacing;
> 710: if (index + 1 == lineCount) bottom -=
> lines[index].getLeading();
please keep one statement per line, if possible
if (index + 1 == lineCount) {
bottom -= lines[index].getLeading();
}
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 711:
> 709: bottom += lines[index].getBounds().getHeight() + spacing;
> 710: if (index + 1 == lineCount) bottom -=
> lines[index].getLeading();
> 711: if (bottom > y) break;
same here
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1036:
> 1034: double py = y;
> 1035:
> 1036: if(isSpan()) {
please insert a space between `if` and `(`
-------------
PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1907661966
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506809039
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506822747
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506822242
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506822301
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506825343
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506825604
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506827456
