On Mon, 13 Apr 2020 06:59:08 GMT, Ajit Ghaisas <aghai...@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
