[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsEx
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2020-06-30 23:17:06
Message-ID: GH4IEP96M2Pvqk6rTqnwe3vHHee11TIowuKe-dotYCw=.4c1e5ded-ef4c-48c0-9894-35bbfe921398 () github ! com
[Download RAW message or body]

On Tue, 16 Jun 2020 09:03:35 GMT, Robert Lichtenberger <rlichten@openjdk.org> wrote:

> > This PR fixes JDK-8176270 by clamping the end index of the selected text to the \
> > length of the text.
> 
> Robert Lichtenberger has updated the pull request incrementally with one additional \
> commit since the last revision: 
> 8176270: Adding ChangeListener to TextField.selectedTextProperty causes \
> StringOutOfBoundsException 
> Move replaceSelectionAtEndWithListener test to general test class.
> Add a more general test for selection/text properties and replace/undo/redo \
> operations.

Changing from using bindings to using listeners seems reasonable as long as there \
can't be a memory leak as a result (it should be fine).

As I note inline below, I think the clamping is still wrong. Maybe you can't ever hit \
a case where it matters now, but it should still be fixed.

modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java line \
186:

> 185:                 int length = txt.length();
> 186:                 if (end > start + length) end = length;
> 187:                 if (start > length-1) start = end = 0;

As I mentioned in [this comment in PR \
#73](https://github.com/openjdk/jfx/pull/73#discussion_r376528686), I think this test \
is wrong. The value of `start` has no impact on whether `end` is out of range. So... \
shouldn't this simply be `if (end > length) end = length;` ?

-------------

PR: https://git.openjdk.java.net/jfx/pull/138


[prev in list] [next in list] [prev in thread] [next in thread]