RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly

[Kevin Rushforth]

On Mon, 13 Apr 2020 06:59:08 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

> Issue : https://bugs.openjdk.java.net/browse/JDK-8193286
> 
> Root Cause :
> Incorrect implementation.
> Current implementation of int wrapValue(int,int,int) in Spinner.java works well if min is 0.
> Hence this implementation works with ListSpinnerValueFactory, but fails with IntegerSpinnerValueFactory.
> 
> Fix :
> Added a method for IntegerSpinnerValueFactory with separate implementation.
> 
> Testing :
> Added unit tests to test increment/decrement in multiple steps and with different amountToStepBy values.
> 
> Also, identified a related behavioral issue https://bugs.openjdk.java.net/browse/JDK-8242553, which will be addressed
> separately.

I checked and there is a case that (mostly) works today that will break with your proposed fix. I left a couple inline
comments.

I wonder if it is better to wait and fix it completely in
[JDK-8242553](https://bugs.openjdk.java.net/browse/JDK-8242553).

modules/javafx.controls/src/main/java/javafx/scene/control/Spinner.java line 793:

> 792:      */
> 793:     static int wrapValue(int value, int min, int max, boolean wrapUp) {
> 794:         int ret = 0;

This is essentially a throw-away function. While it does handle the specific case in the bug, namely that of
incrementing or decrementing a positive number of steps of an `amountToStepBy` that is also positive as long as
`currentVal + nsteps * amountToStepBy <= `2 * max`.

It does it by taking a boolean parameter and doing a simple subtractions, which is not how it eventually needs to be
fixed. It won't handle the case of wrapping around by more than `max-min+1` nor will it handle the case of wrapping
around with a negative `amountToStepBy`.

Also, I did find one case where it will yield differently incorrect behavior from today. in the case where min = 0, and
you step by an amount that puts the new value at `newValue > 2 * max` the old code would at least compute an
almost-correct value using `newVal % max`. The new code will cause it to be clamped at max.

modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java line 581:

> 580:             final int newIndex = getValue() - steps * getAmountToStepBy();
> 581:             setValue(newIndex >= min ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max, false) :
> min)); 582:         }

This will eventually need a new wrapValue function that doesn't take a boolean parameter. The existing one is broken,
but passing a boolean isn't the right answer, either.

modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java line 395:

> 394:     // TODO : This should wrap around and select a value between min and max
> 395:     @Test public void intSpinner_testWrapAround_increment_LargeStep() {
> 396:         intValueFactory.setWrapAround(true);

I don't like the testing for known buggy behavior. If a partial fix is still planned, a better thing would be a test
that verifies correct behavior that is `@Ignore`d pending the final fix.

-------------

Changes requested by kcr (Lead).

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