On Sat, 21 Dec 2019 15:36:14 GMT, Oliver Kopp <github.com+1366654+kop...@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
