Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Wed, 22 Apr 2020 09:06:09 GMT, Jeanette Winzenburg wrote: >> 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). > >> >> I wonder if it is better to wait and fix it completely in >> [JDK-8242553](https://bugs.openjdk.java.net/browse/JDK-8242553). > > good idea - do it correctly once and for all :) Thanks @kevinrushforth for taking a detailed look at this. I wanted to fix this and then fix the buggy behavior change in JDK-8242553 separately. As my proposed Spinner.wrapValue() does not work well in some cases and it's going to get modified anyway - I guess, you and @kleopatra are right in suggesting to fix it entirely in JDK-8242553. I will close this PR. - PR: https://git.openjdk.java.net/jfx/pull/174
Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Tue, 21 Apr 2020 23:34:07 GMT, Kevin Rushforth wrote: > > I wonder if it is better to wait and fix it completely in > [JDK-8242553](https://bugs.openjdk.java.net/browse/JDK-8242553). good idea - do it correctly once and for all :) - PR: https://git.openjdk.java.net/jfx/pull/174
Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Mon, 13 Apr 2020 06:59:08 GMT, Ajit Ghaisas 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
Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Wed, 15 Apr 2020 23:18:46 GMT, Kevin Rushforth 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'm not sure this is quite right, but I need to look at it more closely. It > should fix the bug in question for > well-behaved values, but might make it possible to return a value outside the > range of [min, max]. > I also want to think about it in light of > [JDK-8242553](https://bugs.openjdk.java.net/browse/JDK-8242553). While a > partial fix seems OK, we need to avoid the situation where some values cause > a change from one buggy behavior to a > different buggy behavior on the way to fixing it right. The fix works for well behaved values - and - maintains buggy behavior (to be addressed in JDK-8242553) if (amountToStepBy * no of steps) is greater than the total numbers between min and max. The Spinner.wrapValue() does return value outside min-max range if (amountToStepBy * steps) is greater than total numbers between min and max. It gets clamped to either min or max based on condition in IntegerSpinnerValueFactory valueProperty() listener. I am planning to change Spinner.wrapValue() to use modulo as part of JDK-8242553. This fix is restricted to addressing the issue of stepping through of valid (amountToStepBy * no of steps) which was reported as bug. - PR: https://git.openjdk.java.net/jfx/pull/174
Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Mon, 13 Apr 2020 06:59:08 GMT, Ajit Ghaisas 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'm not sure this is quite right, but I need to look at it more closely. It should fix the bug in question for well-behaved values, but might make it possible to return a value outside the range of [min, max]. I also want to think about it in light of [JDK-8242553](https://bugs.openjdk.java.net/browse/JDK-8242553). While a partial fix seems OK, we need to avoid the situation where some values cause a change from one buggy behavior to a different buggy behavior on the way to fixing it right. - PR: https://git.openjdk.java.net/jfx/pull/174
