RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
fastegal at openjdk.java.net
Wed Feb 12 11:13:16 UTC 2020
On Fri, 7 Feb 2020 18:04:35 GMT, Kevin Rushforth <kcr 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`.
> 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;
on second thought: correct clamping or not, the start/end indices of selection are invalid whenever text was removed/added at an index before the selection - they are in coordinates of the _old_ text, not the new. If they point to an index > new textLength, incorrect clamping will throw. If they are both < new textLength, it'll fire an intermediate changeEvent with incorrect selection, the total sequence beining
oldSelectedText -> incorrectly-shifted-selectedText
incorrectly-shifted-selectedText -> empty
The correct change notification would be, because at the end of the text change, the selection is cleared,
oldSelectedText -> empty
To me, it looks like using a binding here is not the best of options.
More information about the openjfx-dev