Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8193286: IntegerSpinnerFactory does not wrap value correctly #174

Closed
wants to merge 1 commit into from

Conversation

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Apr 13, 2020

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8193286: IntegerSpinnerFactory does not wrap value correctly

Download

$ git fetch https://git.openjdk.java.net/jfx pull/174/head:pull/174
$ git checkout pull/174

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 13, 2020

👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Apr 13, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 13, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 15, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 15, 2020

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. 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.

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Apr 16, 2020

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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

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.

* constraints. Used by the SpinnerValueFactory implementations when
* the Spinner wrapAround property is true.
*/
static int wrapValue(int value, int min, int max, boolean wrapUp) {

This comment has been minimized.

@kevinrushforth

kevinrushforth Apr 21, 2020
Member

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.

@@ -578,7 +578,7 @@ public final IntegerProperty amountToStepByProperty() {
final int min = getMin();
final int max = getMax();
final int newIndex = getValue() - steps * getAmountToStepBy();
setValue(newIndex >= min ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max) + 1 : min));
setValue(newIndex >= min ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max, false) : min));

This comment has been minimized.

@kevinrushforth

kevinrushforth Apr 21, 2020
Member

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.

@@ -390,6 +391,184 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
assertEquals(1, (int) intValueFactory.getValue());
}

// TODO : This should wrap around and select a value between min and max
@Test public void intSpinner_testWrapAround_increment_LargeStep() {

This comment has been minimized.

@kevinrushforth

kevinrushforth Apr 21, 2020
Member

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 @Ignored pending the final fix.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 22, 2020

I wonder if it is better to wait and fix it completely in JDK-8242553.

good idea - do it correctly once and for all :)

@aghaisas
Copy link
Collaborator Author

@aghaisas aghaisas commented Apr 22, 2020

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.

@aghaisas aghaisas marked this pull request as draft Apr 22, 2020
@openjdk openjdk bot removed the rfr label Apr 22, 2020
@aghaisas aghaisas closed this Apr 22, 2020
@aghaisas aghaisas deleted the aghaisas:integerSpinner_fix branch May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants