On Fri, 7 Feb 2020 18:04:35 GMT, Kevin Rushforth <k...@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.


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

Reply via email to