On Tue, 22 Aug 2023 20:46:21 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> The fix uses character BreakIterator instead of the logic that relies on
> caretBounds/hitTest/rangeShape in TextInputControl.nextCharacterVisually().
>
> I believe this is a more reliable method of navigation, as it behaves in sync
> with the jdk break iterator, thought it might work differently around
> grapheme clusters, considering a recent change JDK-8291660
>
> This change also introduces TextInputControlHelper class (impl. detail) which
> gives access to character- and word- break iterators cached by
> TextInputControl (*some say* these iterators and associated editing logic
> should be a part of Content implementation, but that's a discussion for
> another day).
modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java
line 759:
> 757: /**
> 758: * Returns a cached instance of character break iterator, creating
> it if necessary.
> 759: * The instance is initialized with the given text.
"updated with the current text" might be more accurate ? It obviously has to be
up to date to be used.
I'm also unsure of the performance consequences of having a BI spanning the
entire text of the TextControl, which could be quite large, but that's what the
code was already doing I suppose.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextAreaSkin.java
line 572:
> 570: if (isRTL()) {
> 571: // Text node is mirrored.
> 572: moveRight = !moveRight;
This method confuses me as to what it actually wants to do and why
As in, "move right, no, your other right" (!)
isRTL() checks the NodeOrientation so it seems in such a case "right" means
"left".
Have you checked the fix with both orientations ?
Seems to me it reverses the meaning of the arrow keys without regard to the
text.
And he BreakIterator is getting the next logical position, but this method
claims to move to the next visual position.
The method is called nextCharacterVISUALLY, but with this code its actually
moving LOGICALLY,
so I'm not seeing how this works for mixed directional text for either setting
of "isRTL()" which seems to me to be very questionable to even check here. IMO
the arrow key should work the same way no matter whether the UI is laid out
with nodes from RTL or LTR.
Perhaps the word "Visually" was not the best choice of name here ? And "right"
is questionable too. But I'm not 100% sure.
if you really do want LOGICALLY, then this code will probably work, except I
find the isRTL() questionable even then.
What exactly was wrong in the existing code ? Other than it being very complex.
How did its calculations go wrong ?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1220#discussion_r1350772981
PR Review Comment: https://git.openjdk.org/jfx/pull/1220#discussion_r1350820007
