On Thu, 12 Oct 2023 09:40:18 GMT, Karthik P K <k...@openjdk.org> wrote:
>> The text run selected in `PrismTextLayout::getHitInfo()` method for
>> character index calculation was not correct when hitTest was invoked for
>> Text node in a TextFlow with more than one Text child. Hence wrong character
>> index value was calculated.
>>
>> Since only x, y coordinates were available in the above mentioned method,
>> sending the text as a parameter to this method is necessary so as to know if
>> the text run selected for character index calculation is correct. Along with
>> this change modified the `PrismTextLayout::getHitInfo()` method to calculate
>> the correct character index.
>>
>> Added tests to validate the changes.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix repeating text node issue
I think this looks good (with minor suggestions). Tested with the MonkeyTester
and with a rich text that contains multiple Text instances with exactly the
same text.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
line 213:
> 211: * @return returns a {@link Hit} object containing character index,
> insertion index and position of cursor on the character.
> 212: */
> 213: public Hit getHitInfo(float x, float y, String text, int
> textRunStart, int curRunStart);
please add new `@param` blocks to javadoc
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 458:
> 456: for (int i = 0; i < lineIndex; i++) {
> 457: for (TextRun r: lines[i].runs) {
> 458: if (r.getTextSpan() != null &&
> r.getTextSpan().getText().equals(text) && r.getStart() >= textRunStart) {
speedup: you may want to compute `>=textRunStart` condition before
`.equals(text)` to save CPU cycles (basically, swap the two conditions here)
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 466:
> 464: boolean isPrevNodeExcluded = false;
> 465: for (TextRun r: runs) {
> 466: if (!r.getTextSpan().getText().equals(text) ||
> (r.getTextSpan().getText().equals(text) && r.getStart() < textRunStart)) {
same suggestion about swapping conditions to save some CPU cycles
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 763:
> 761: if (!textFound) {
> 762: for (TextRun r : lines[index].runs) {
> 763: if (r.getTextSpan() == null ||
> (r.getTextSpan().getText().equals(text) && r.getStart() == runStart)) {
and definitely here `==` should go before `.equals()`
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1032:
> 1030: while (runIndex < runs.length - 1) {
> 1031: if (ptInParent.getY() > runs[runIndex].getLocation().y
> 1032: && ptInParent.getY() < runs[runIndex +
> 1].getLocation().y) {
[minor]: the formatting looks off. If we rename 'ptInParent` to `p` it will
fit on one line.
Also, ptInParent.getY() gets called twice, may be use a local variable? Also
will make it shorter.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1039:
> 1037: }
> 1038: int textRunStart = runs.length == 0 ? 0 : ((TextRun)
> runs[0]).getStart();
> 1039: int curRunStart = runs.length == 0 ? 0 : ((TextRun)
> runs[runIndex]).getStart();
very minor: we are computing run.length == 0 twice. I know it might look
neater than
int textRunStart;
int curRunStart;
if(runs.length == 0) {
textRunStart = 0;
curRunStart = 0;
} else {
...
}
-------------
PR Review: https://git.openjdk.org/jfx/pull/1157#pullrequestreview-1674985140
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357290999
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357293077
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357293685
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357294513
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357297816
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1357301413
