Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Wed, 17 Jan 2024 10:57:44 GMT, Karthik P K wrote: > flickering is observed This is not an issue: the hit info label gets wider than available space, causing a layout (there is no scroll bar). - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1896149715
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Tue, 16 Jan 2024 12:50:59 GMT, Karthik P K wrote: > There seems to be a weird problem with Text (tested on macOS) in the Monkey > Tester. Fixed this issue. Please check. I observed one more issue. When I scroll the Text window around 70% or more with Writing Systems selected and then hover over the text, flickering is observed. Attaching the video for reference. https://github.com/openjdk/jfx/assets/26969459/601aac43-c8d8-4402-9f15-bc23e63572f9 - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1895571801
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Tue, 16 Jan 2024 15:42:28 GMT, Andy Goryachev wrote: >> Added comment and used bit logic in the condition. >> Do you think we should create a method in TextRun? I believe it is out of >> scope of this PR as it will be used in other functions as well. > > Yes, creating such a new method in TextRun might be out of scope for this, > unless we touch all the places where the bit logic is used. Up to you, it's > just a suggestion. I would prefer not to make this change in this PR. So I will keep this as it is for now. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1454584637
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Tue, 16 Jan 2024 10:34:42 GMT, Karthik P K wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
>> line 562:
>>
>>> 560: charIndex += textWidthPrevLine;
>>> 561: charIndex += relIndex;
>>> 562: if (run.getLevel() % 2 != 0) {
>>
>> I wish there was an explanation of the meaning of `level`
>> And since there are several places where it checks for it being odd, I wish
>> there was a method in TextRun with a descriptive name rather than this
>> computation (and bit logic might be faster):
>>
>>
>> public boolean isLevelOdd() { // or whatever the meaning is
>> return (level & 0x01) != 0;
>> }
>
> Added comment and used bit logic in the condition.
> Do you think we should create a method in TextRun? I believe it is out of
> scope of this PR as it will be used in other functions as well.
Yes, creating such a new method in TextRun might be out of scope for this,
unless we touch all the places where the bit logic is used. Up to you, it's
just a suggestion.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453612702
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Fri, 12 Jan 2024 16:58:55 GMT, Andy Goryachev wrote: > There seems to be a weird problem with Text (tested on macOS) in the Monkey > Tester. 'Writing Systems' is a multi-line text with a tricky font (which does > not get rendered correctly in LTR mode for some reason, but does in RTL). So > if you try to hover over Aramaic line, the hit test info does not get updated: > I see this problem when I tested in a separate app as well. I will check on this issue. - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1893679318
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Fri, 12 Jan 2024 16:14:20 GMT, Andy Goryachev wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Code review changes
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
> line 480:
>
>> 478: }
>> 479: } else {
>> 480: // To calculate hit info of Text embedded in TextFlow
>
> the comments on lines 480 and 451 are a bit confusing: both refer to Text.
> could you please clarify?
The first code branch is used to calculate hit info of Text node which are not
embedded in TextFlow and hit info requested on TextFlow, where as the second
code branch is used to calculate hit info of Text node embedded in TextFlow.
Updated the comments.
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
> line 524:
>
>> 522: break;
>> 523: }
>> 524: // For only English Text embedded in TextFlow
>> in RTL orientation
>
> this comment seems misleading (by English you mean LTR, right?)
> would it be possible to re-phrase, explaining why?
> does it mean the RTL text has been handled by the previous code block and now
> we are dealing with LTR?
Yes, it handles the LTR text present in RTL mode. Rephrased the comment.
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
> line 562:
>
>> 560: charIndex += textWidthPrevLine;
>> 561: charIndex += relIndex;
>> 562: if (run.getLevel() % 2 != 0) {
>
> I wish there was an explanation of the meaning of `level`
> And since there are several places where it checks for it being odd, I wish
> there was a method in TextRun with a descriptive name rather than this
> computation (and bit logic might be faster):
>
>
> public boolean isLevelOdd() { // or whatever the meaning is
> return (level & 0x01) != 0;
> }
Added comment and used bit logic in the condition.
Do you think we should create a method in TextRun? I believe it is out of scope
of this PR as it will be used in other functions as well.
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1040:
>
>> 1038:
>> 1039: private int findRunIndex(double x, double y, GlyphList[] runs) {
>> 1040: int runIndex = 0;
>
> some general comment for this method:
> 1. there are many places where runs[runIndex] is repeated within the same
> code block, I wonder if it would make sense to create a local variable. It
> might be tricky because the runIndex changes mid-flight.
> 2. perhaps `runIndex` could be shortened to `ix` to make the lines shorter?
Refactored the function. Changed `runIndex` to `ix`.
I could remove some repetition of `runs[runIndex]`. Had to keep some instances
since previous or next runs are used in some cases.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453234155
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453235252
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453237200
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453239916
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Thu, 11 Jan 2024 10:15:01 GMT, Karthik P K 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: > > Code review changes There seems to be a weird problem with Text (tested on macOS) in the Monkey Tester. 'Writing Systems' is a multi-line text with a tricky font (which does not get rendered correctly in LTR mode for some reason, but does in RTL). So if you try to hover over Aramaic line, the hit test info does not get updated:  hit test is also not updated over some other areas, so you may want to research this. - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1889648627
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Thu, 11 Jan 2024 10:15:01 GMT, Karthik P K 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:
>
> Code review changes
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 474:
> 472: if (x < run.getWidth()) break;
> 473: if (i + 1 < runs.length) {
> 474: if (runs[i + 1].isLinebreak()) break;
could we surround break; with { }'s here and on lines 461, 472, 474 please?
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 480:
> 478: }
> 479: } else {
> 480: // To calculate hit info of Text embedded in TextFlow
the comments on lines 480 and 451 are a bit confusing: both refer to Text.
could you please clarify?
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 492:
> 490: for (TextRun r: runs) {
> 491: if (r.getStart() != curRunStart &&
> r.getTextSpan().getText().equals(text)) {
> 492: isMultiRunText = true;
minor: we could reduce the scope of `isMultiRunText` by moving its declaration
from line 427 to line 490
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 524:
> 522: break;
> 523: }
> 524: // For only English Text embedded in TextFlow in
> RTL orientation
this comment seems misleading (by English you mean LTR, right?)
would it be possible to re-phrase, explaining why?
does it mean the RTL text has been handled by the previous code block and now
we are dealing with LTR?
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 562:
> 560: charIndex += textWidthPrevLine;
> 561: charIndex += relIndex;
> 562: if (run.getLevel() % 2 != 0) {
I wish there was an explanation of the meaning of `level`
And since there are several places where it checks for it being odd, I wish
there was a method in TextRun with a descriptive name rather than this
computation (and bit logic might be faster):
public boolean isLevelOdd() { // or whatever the meaning is
return (level & 0x01) != 0;
}
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1040:
> 1038:
> 1039: private int findRunIndex(double x, double y, GlyphList[] runs) {
> 1040: int runIndex = 0;
some general comment for this method:
1. there are many places where runs[runIndex] is repeated within the same code
block, I wonder if it would make sense to create a local variable. It might be
tricky because the runIndex changes mid-flight.
2. perhaps `runIndex` could be shortened to `ix` to make the lines shorter?
tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java
line 111:
> 109: static Random random;
> 110: static Robot robot;
> 111: static Text textOne;
maybe rename to 'text' since there is only one instance? or `control`?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450655360
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450660028
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450676811
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450680153
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450687339
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450691638
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450693272
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Fri, 12 Jan 2024 05:16:33 GMT, Karthik P K wrote: >> @karthikpandelu , the more I think about it, the less I like the idea of >> overloading (textRunStart and curRunStart). >> what if things will change in the future? >> >> I think it'll be much cleaner to pass a boolean forTextFlow (or forText) or >> some such. we have to compute such a boolean flag anyway, why not just pass >> it? > > Are you suggesting to pass boolean as parameter in addition to textRunStart > and curRunStart? If that is the case then yes I think it would be better. that's right, something like this: public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart, boolean forTextFlow); - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450629952
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Fri, 12 Jan 2024 00:18:38 GMT, Andy Goryachev wrote: >> I changed 0 to -1 to figure out if the `getHitInfo` in `PrismTextLayout` is >> invoked by `Text` or `TextFlow`. >> Added comment regarding this in `com.sun.javafx.scene.tex.TextLayout`. > > @karthikpandelu , the more I think about it, the less I like the idea of > overloading (textRunStart and curRunStart). > what if things will change in the future? > > I think it'll be much cleaner to pass a boolean forTextFlow (or forText) or > some such. we have to compute such a boolean flag anyway, why not just pass > it? Are you suggesting to pass boolean as parameter in addition to textRunStart and curRunStart? If that is the case then yes I think it would be better. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1449853445
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Thu, 11 Jan 2024 10:25:44 GMT, Karthik P K wrote: >> or, would it make more sense to simply pass a boolean flag instead of magic >> values? > > I changed 0 to -1 to figure out if the `getHitInfo` in `PrismTextLayout` is > invoked by `Text` or `TextFlow`. > Added comment regarding this in `com.sun.javafx.scene.tex.TextLayout`. @karthikpandelu , the more I think about it, the less I like the idea of overloading (textRunStart and curRunStart). what if things will change in the future? I think it'll be much cleaner to pass a boolean forTextFlow (or forText) or some such. we have to compute such a boolean flag anyway, why not just pass it? - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1449575341
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Wed, 10 Jan 2024 20:13:24 GMT, Andy Goryachev wrote: >> Karthik P K has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code review changes > > tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java > line 48: > >> 46: import javafx.stage.StageStyle; >> 47: import javafx.stage.Window; >> 48: import org.junit.After; > > should we be using junit5 for all new tests? Made changes in both the test files to use unit5 > is there a reason to cast to int? No, I reused an old test file and developed these tests based on that. Hence the cast was kept as it is. >fx robot does accept double arguments, and we might be dealing with fractional >scale on some platforms Removed the cast. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1448631575 PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1448631045
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Wed, 10 Jan 2024 20:31:43 GMT, Andy Goryachev wrote: >> 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, -1, -1); >> >> -1 looks like magic value, could you please describe it in the >> `com.sun.javafx.scene.tex.TextLayout` javadoc in both cases (textRunStart >> and curRunStart)? > > or, would it make more sense to simply pass a boolean flag instead of magic > values? I changed 0 to -1 to figure out if the `getHitInfo` in `PrismTextLayout` is invoked by `Text` or `TextFlow`. Added comment regarding this in `com.sun.javafx.scene.tex.TextLayout`. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1448627945
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
On Wed, 10 Jan 2024 23:23:14 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1042:
>>
>>> 1040: int runIndex = 0;
>>> 1041: if (runs.length != 0) {
>>> 1042: if (this.getScene().getNodeOrientation() ==
>>> NodeOrientation.RIGHT_TO_LEFT) {
>>
>> I think this should not refer to scene:
>>
>>
>> if (getNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT) {
>
> I agree. Using the scene's orientation seems conceptually wrong. Shouldn't
> this use `Node::getEffectiveNodeOrientation`?
Agreed. scene's orientation shouldn't be used here.
Made changes to use `getEffectiveNodeOrientation`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1448625157
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v2]
> 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: Code review changes - Changes: - all: https://git.openjdk.org/jfx/pull/1323/files - new: https://git.openjdk.org/jfx/pull/1323/files/8d42bd37..b23e4910 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1323=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1323=00-01 Stats: 115 lines in 4 files changed: 5 ins; 18 del; 92 mod Patch: https://git.openjdk.org/jfx/pull/1323.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1323/head:pull/1323 PR: https://git.openjdk.org/jfx/pull/1323
