On Fri, 25 Aug 2023 14:56:24 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review coments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 590:
>
>> 588: int next = moveRight ? bi.following(pos) : bi.preceding(pos);
>> 589: if (next != BreakIterator.DONE) {
>> 590: textField.selectRange(next, next);
>
> I might suggest making this PR depend on
> https://github.com/openjdk/jfx/pull/1220 and make use of
> `TextInputControlHelper.charIterator()` to get the instance cached in
> TextInputControl.
Sure. I'll wait till [PR#1220](https://github.com/openjdk/jfx/pull/1220) to get
integrated and use `TextInputControlHelper` method.
> tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
> line 44:
>
>> 42: import org.junit.Assert;
>> 43: import org.junit.BeforeClass;
>> 44: import org.junit.Test;
>
> should we use junit5 instead?
junit5 would be better. Updated code to use the same
> tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
> line 48:
>
>> 46: import test.util.Util;
>> 47:
>> 48: public class TextFieldCursorMovementTest {
>
> I'd recommend to add a description of the test in a javadoc.
Added description for tests.
> tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
> line 50:
>
>> 48: public class TextFieldCursorMovementTest {
>> 49: static CountDownLatch startupLatch = new CountDownLatch(1);
>> 50: static CountDownLatch caretPositionLatch;
>
> caretPositionLatch is unused
Removed unused caretPositionLatch
> tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
> line 69:
>
>> 67: }
>> 68:
>> 69: private void addTextFieldContent(String text, boolean isRtl) {
>
> do you think we should also test the case when isRtl = false?
Added test cases for mixed text, rtl and ltr text as well.
> tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
> line 81:
>
>> 79: @Test
>> 80: public void testCursorMovementInRTLText() throws Exception {
>> 81: String str = "Aracbic يشترشسيرشي";
>
> spelling "Arabic"
Corrected typo
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307292693
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307293802
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307292890
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307294016
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307293241
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307293413
