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

[Kevin Rushforth]

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