On Wed, 14 Feb 2024 04:39:04 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments
>
> 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
The calculation done ln.no.486 calculates the effective value of x relative to
the Text node on which hit test is requested. As far as I understood, the above
suggested calculation doesn't perform the same calculation. So I did not make
any changes.
Am I missing something here. Please let me know if you have any suggestions.
> 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).
The additional conditions are added to differentiate between the three cases:
Text, TexFlow and Text embedded in TextFlow. Hence not make changes in this.
> 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)`;
As I mentioned earlier, the condition is required to differentiate between
Text, TexFlow and Text embedded in TextFlow. Hence not removing condition, made
modification to use `forTextFlow` instead of `text == null`
> 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;
> }
Made changes to use `forTextFlow` instead of condition `text != null` so that
it is easier to understand
> 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).
Added comment above regarding this
> 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.
Used `getTextInternal()` instead of `getText()`. I believe the text parameter
is needed as mentioned before
> 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`?)
Removed last parameter as suggested
> 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.
I see that the tests don't run on all the platforms. I will look into this and
update the PR
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494314778
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494322207
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494317666
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494324563
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494323474
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494326008
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494326979
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494328009
