RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

Kevin Rushforth kcr at openjdk.java.net
Fri Feb 7 18:26:54 UTC 2020

On Sat, 21 Dec 2019 15:36:14 GMT, Oliver Kopp <github.com+1366654+koppor at openjdk.org> wrote:

> This is a WIP PR. Requesting for comments.
> I could not replicate the test given at https://bugs.openjdk.java.net/browse/JDK-8176270 without TestFX. I nevertheless let my try to replicate the @Test things in here.
> The fix is just a wild guess without really understanding the side effects of `.addListener`.

I left comments inline. This will need changes. Also, you will need to provide a test that fails without the fix and passes with the fix.

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

> 167:                 // Ensure that the last character to get is within the bounds of the txt string
> 168:                 if (end >= start + length) end = length-1;
> 169:                 // In case the start is after the whole txt, nothing valid is selected. Thus, return the default.

First, I think the existing test is simply wrong: Why should the `start` value matter when checking whether the `end` value needs to be clamped -- it's an `end` point not a number of characters. I think the entire problem might be the fact that it is comparing against `start + length`. Second, the value to be clamped to was correct before your change. The `substring` method to which `end` is passed is documented as subtracting 1.

I think the test should be something like:

    if (end > length) end = length;

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

> 169:                 // In case the start is after the whole txt, nothing valid is selected. Thus, return the default.
> 170:                 if (start >= length) return "";
> 171:                 return txt.substring(start, end);

This change seems OK, and might be clearer and the existing code, but the existing code is correct, and produces the same effect.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 2088:

> 2087:         Semaphore semaphore = new Semaphore(0);
> 2088:         Platform.runLater(semaphore::release);
> 2089:         semaphore.acquire();

Since controls tests run using the StubToolkit, `Platform.runLater` does not do what you expect. If you need to run a pulse to get some code to run that normally would run as part of pulse, you can call that method directly. See other controls tests for how this is done.


Changes requested by kcr (Lead).

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

More information about the openjfx-dev mailing list