8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases

[drmarmac]

This PR should fix the issue and cover all relevant cases with new tests.

Note: This involves a small behavior change, as can be seen in dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With this change the wraparound behavior is similar to that of the IntegerSpinner.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

• JDK-8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases (Bug - P3)
• JDK-8193286: IntegerSpinnerFactory does not wrap value correctly (Bug - P3)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Reviewer)
• Karthik P K (@karthikpandelu - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1431/head:pull/1431
$ git checkout pull/1431

Update a local copy of the PR:
$ git checkout pull/1431
$ git pull https://git.openjdk.org/jfx.git pull/1431/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1431

View PR using the GUI difftool:
$ git pr show -t 1431

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1431.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back drmarmac! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@drmarmac This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
8193286: IntegerSpinnerFactory does not wrap value correctly

Reviewed-by: angorya, kpk

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 41 new commits pushed to the master branch:

• f27077b: Merge
• c721a4b: Merge
• 5482b84: Merge
• 3914db2: Merge
• 34c9dcb: Merge
• 0aaaa95: Merge
• 2e16a0c: Merge
• 9208e02: Merge
• a95be13: Merge
• 9ec34b4: 8322236: Build failure after JDK-8313064
• ... and 31 more: https://git.openjdk.org/jfx/compare/47c6193c7ef264e204ba36fb4ae9f76eb0dce318...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@andy-goryachev-oracle, @karthikpandelu) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 01: Full - Incremental (6b38b9ce)
• 00: Full (103ac473)

[kevinrushforth]

Reviewers @andy-goryachev-oracle @karthikpandelu

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[kevinrushforth]

@drmarmac It is generally best to ask whether an assigned bug is actively being worked on before submitting a PR. In this case, I suspect it wasn't being worked on. I've asked the assignee (Karthik) to be one of the reviewers.

[drmarmac]

ok, thanks for the notice!

[andy-goryachev-oracle]

@karthikpandelu I've updated the spinner page in the monkey tester #1406 so we can use it to test this PR (totally missed that this page was incomplete).

[andy-goryachev-oracle]

I am a bit uneasy with the proposed solution. Let me explain:

The first thing that seems weird to me is the mere existence of increment(int)/decrement(int). Well, it is an existing API, and probably there are some use cases that I cannot immediately see.

The real issue is when we have wrapAround enabled and either a large amountToStepBy or the argument for increase/decrease exceeding the (max - min) value. This simply makes no sense, and the modulo arithmetic produces, in my opinion, an unexpected user experience. What I mean is that some combinations of Spinner settings would result in apparently "random" jumps (from the user perspective).

What I think should be done is - when this scenario is detected - the adjustment should default to maybe 1 (or (max - min)/10 in the case of the DoubleSpinnerValueFactory). The goal is to fall back gracefully to something a human can understand - and a modulo division is not one of those.

What do you think?

[andy-goryachev-oracle]

There is one more unexpected behavior - I am going to mention it here, but it might need to be extracted into its own ticket (if people agree it's a bug):

Consider a Spinner with a ListSpinnerValueFactory, initialized with a list of values let's say [ "one", "two", "three", "four" ].
Let's start with "one" as a selected value, and press the Down Arrow button. Since it is based on a list, and majority of human writing systems start at the top and go down (unless we are in Australia), I would expect "two" to appear. But no, we see "four", wait, why? Yes, it might operate with an integer index and treat down arrow button as decrementing that index, but it is so wrong [citation needed]. I would expect that in a list spinner, clicking the down arrow button should navigate to the next item in the list ("as written") instead of going "up" the list.

What do you think?

[kevinrushforth]

The real issue is when we have wrapAround enabled and either a large amountToStepBy or the argument for increase/decrease exceeding the (max - min) value. This simply makes no sense, and the modulo arithmetic produces, in my opinion, an unexpected user experience. What I mean is that some combinations of Spinner settings would result in apparently "random" jumps (from the user perspective).

Why? This is entirely under app control. Is it useful to pass an increment that will produce a step larger than the range? Maybe not, but restricting it doesn't see like the right solution, at least not without a good reason to do so. The "obvious" thing is to do modulo arithmetic, which is what this PR aims to do. By "obvious" thing to do, it is the mathematically correct thing to do. It makes a single step of 10 equivalent to 10 steps of 1 regardless of the min / max.

What I think should be done is - when this scenario is detected - the adjustment should default to maybe 1 (or (max - min)/10 in the case of the DoubleSpinnerValueFactory). The goal is to fall back gracefully to something a human can understand - and a modulo division is not one of those.

And I would argue that either of those choices is more arbitrary and confusing than the proposed solution, and not appreciably better than what is done today.

I see two sensible solutions:

1. Use modulo arithmetic to compute the value (what this PR proposed)
2. Don't process the increment at all (i.e., treat it as an out-of-range request and do nothing)

Choice 1 seems best unless there is a down-side that I haven't considered.

[kevinrushforth]

There is one more unexpected behavior - I am going to mention it here, but it might need to be extracted into its own ticket (if people agree it's a bug):

Consider a Spinner with a ListSpinnerValueFactory, initialized with a list of values let's say [ "one", "two", "three", "four" ]. Let's start with "one" as a selected value, and press the Down Arrow button. Since it is based on a list, and majority of human writing systems start at the top and go down (unless we are in Australia), I would expect "two" to appear. But no, we see "four", wait, why? Yes, it might operate with an integer index and treat down arrow button as decrementing that index, but it is so wrong [citation needed]. I would expect that in a list spinner, clicking the down arrow button should navigate to the next item in the list ("as written") instead of going "up" the list.

This is definitely unrelated to this bug, so should be discussed separately. I can see your point, but this would be more than just a bug fix -- it would be a behavioral change that we would need to carefully consider.

[andy-goryachev-oracle]

• Use modulo arithmetic to compute the value (what this PR proposed)
• Don't process the increment at all (i.e., treat it as an out-of-range request and do nothing)

3. fall back to amountToStepBy=1

try this: integer spinner, min=0. max=100, step=137. initial value 50
there is the sequence when repeatedly pressing arrow down button:
14, 79, 43, 7, 72, 36, 0, ...

makes no sense.

falling back to step=1:

49, 48, 47, 46, ...

[kevinrushforth]

• Use modulo arithmetic to compute the value (what this PR proposed)
• Don't process the increment at all (i.e., treat it as an out-of-range request and do nothing)

3. fall back to amountToStepBy=1

try this: integer spinner, min=0. max=100, step=137. initial value 50 there is the sequence when repeatedly pressing arrow down button: 14, 79, 43, 7, 72, 36, 0, ...

makes no sense.

But if that's what the app says to do, and given that it's a well-defined operation (albeit not very useful in this example), there should be a good reason to ignore it. I'm not convinced that there is, but if there is, option 3 isn't it.

falling back to step=1:

49, 48, 47, 46, ...

That's at least as arbitrary as clamping to the max value (the status quo). Clamping the result to the max says, in effect, "let's ignore the wrap flag" when we conclude that wrapping beyond a certain limit doesn't make sense. Setting the increment to 1 says, in effect, "let's ignore the step value" when we conclude that wrapping beyond a certain limit doesn't make sense. If we really are determined to not honor the step value / wrap flag when we conclude that wrapping beyond a certain limit doesn't make sense, then we should treat it as an app error and do nothing.

[drmarmac]

I don't see that much value in any threshold like step >= max-min, no matter what the behavior beyond this threshold would be.
It would be almost as weird already for less extreme cases: Take min=0, max=100, step=99, initial=50, leading to 50-49-48.., which even appears to move backwards.
Not sure if we can come up with any adjustments that cover all cases nicely - as an app developer I'd prefer either good logic that always works well, or simple logic that is easy to override (which it would be here).

[kleopatra]

afair, we already had extensive discussions in #174 (and the related bug and the mailing list - too lazy to read up on all that) agreeing that modulo math (correctly done!) fits common client programmers' expectations most - doesn't matter the step size.

[kevinrushforth]

Let's proceed with the current plan of correctly-implemented modulo arithmetic. I can't think of a compelling reason to do otherwise.

[andy-goryachev-oracle]

If everyone is ok with modulo arithmetic.

Please clarify the expected behavior in the case of an integer spinner with {min=0; max=100; amountToStepBy=101} and initial value of 0. What should the increment action result in?

(0 + 101) % (100 - 0) + 0 = 1

the code in this PR produces no movement (0). Same for the decrement.

Is this correct?

[kleopatra]

(0 + 101) % (100 - 0) + 0 = 1

the code in this PR produces no movement (0). Same for the decrement.

which means that the modulo arithmetic is not yet quite correct (not that the modulo behavior as such is wrong ;)

nice test case, btw ;)

[andy-goryachev-oracle]

As a user, I would be very surprised with the behavior based on modulo arithmetic.

But I do acknowledge the point made by Kevin - if this is what the application developers wanted, then it is their fault.

[drmarmac]

The code, as currently suggested, effectively calculates x(i+1) = (x(i) + amountToStepBy) MOD (max - min + 1).
I'd say this is the modulo value we actually want, because x=min and x=max should both be included. Look at a simpler example, which would not work with a MOD (max-min) formula:

min=0; max=2; amountToStepBy=1, initial=0 => 0, 1, 2, 0, 1, ...
min=0; max=2; amountToStepBy=2, initial=0 => 0, 2, 0, 2, 0, ...
min=0; max=2; amountToStepBy=3, initial=0 => 0, 0, 0, 0, 0, ...
min=0; max=2; amountToStepBy=4, initial=0 => 0, 1, 2, 0, 1, ...

So your example should result in min=0; max=100; amountToStepBy=101, initial=0 => 0, 0, 0, 0..., which it currently does.

[andy-goryachev-oracle]

the end result of stepping once by amountToStepBy must be equivalent of incrementing by one a total of amountToStepBy times, wouldn't you agree?

so the formula for positive amountToStepBy values should look like

((val + amountToStepBy) % (max - min)) + min

or something along these lines.

[kevinrushforth]

the end result of stepping once by amountToStepBy must be equivalent of incrementing by one a total of amountToStepBy times, wouldn't you agree?

Yes, this is the expectaion.

so the formula for positive amountToStepBy values should look like

((val + amountToStepBy) % (max - min)) + min

or something along these lines.

Not quite. Because the range is inclusive on both ends, the number of discrete positions is max - min + 1. In your above example , where min=0 and max=100, there are 101 discrete positions. A single step by 101 or 101 steps by 1 will both bring you back to the same position you started from.

[andy-goryachev-oracle]

the number of discrete positions is max - min + 1

you are right! my earlier comment is invalid. please ignore.

Now, when wrapping occurs, should max/min be considered as valid steps?

Example, try both integer and double spinners with {min=0, max=10, step=7} starting with value=0. try incrementing.

integer: 0, 7, 3, 10
double: 0, 7, 10, 0, 7, 10

And another question: should the new behavior (whatever everyone agrees to eventually) be documented? Where? This PR? JBS? Javadoc? A markdown file in doc-files/behavior/?

Does it need a CSR?

[kevinrushforth]

Now, when wrapping occurs, should max/min be considered as valid steps?

For integer values (including list indices), yes, since each integer (or list index) is a discrete value that can have the value min, max, or any value in between. And a step of one when you are at "max" should wrap around to "min"

For double... that's a good question. I need to think about it a bit. Often when doing wrap-around for floating-point values (think rotation transforms) we treat max and min the same -- although usually that means treating the range as exclusive of max, and that isn't what is current done for double values of spinner.

Example, try both integer and double spinners with {min=0, max=10, step=7} starting with value=0. try incrementing.

integer: 0, 7, 3, 10
double: 0, 7, 10, 0, 7, 10

The integer is working as I would expect (and the next value should be "6").
The double is not -- it looks like it is clamping rather than wrapping unless the value is already at the max.

And another question: should the new behavior (whatever everyone agrees to eventually) be documented? Where? This PR? JBS? Javadoc? A markdown file in doc-files/behavior/?

If we want to specify the functionality, it needs to be documented in the javadoc-generated API docs. That's the specification. Given that this is a clarification of what we mean by "wrapping" and not a fundamental change, we could either do that as part of this enhancement request or a separate one.

Does it need a CSR?

The current spec is sufficiently vague on what the wrapping algorithm is that as long as we aren't changing anything fundamental, I don't think we do need a CSR. If we end up changing the spec to document the wrapping algorithm (either as part of this PR or separately), then the change in spec would need a CSR.

[drmarmac]

I see these possible definitions for the double spinner behavior with wraparound:

1. Current PR: Wraparound uses amountToStepBy amount, formula: MOD (max - min + amountToStepBy)
2. Make it behave like the integer spinner, meaning the wraparound is always a step size of 1, formula: MOD (max - min + 1)
3. Calculate MOD (max - min), which would make the upper limit non-inclusive (i.e. it will never be reached).
4. Calculate MOD (max - min) only when > max, i.e. allow the maximum, but calculate like it's the same as the minimum, meaning stepping directly from max to min and vice versa is usually not possible. Also, both max and min values will only be reachable from one direction.
   Looking at this again, maybe 4) is actually preferable?

[kevinrushforth]

1 seems arbitrary and wrong.
2 isn't much better, since the "+1" is arbitrary for a double spinner.

Neither of the above options make any sense for the sort of things you would use a wrap-around double spinner for. So that leaves 3 and 4. I was leaning towards recommending something like 4 even before you posted it, and the more I think about it, the more it seems like the best option to me.

[drmarmac]

I've updated the behavior for the double spinner according to option 4.

Consequences of this approach are:

• The wrapping behavior of the double spinner is not the same as for the integer spinner, even for all-integer values.
• Going over the limit with one Spinner arrow, and going back with the other arrow, will generally not result in the previous value.

[andy-goryachev-oracle]

It looks like the issue JDK-8193286 is also fixed by this change, so it probably should be added to this PR.

Also, this PR changes the way {List|Integer|Double}ValueFactory behaves when wrapping is enabled, so even though we determined that no CSR is required, I feel we need to document the new behavior in the ticket.

So here my suggestions:

1. add JDK-8193286 to this PR
2. describe the new behavior in JDK-8242553 description

otherwise, lgtm.

[drmarmac]

/issue add JDK-8193286

[openjdk]

@drmarmac
Adding additional issue to issue list: 8193286: IntegerSpinnerFactory does not wrap value correctly.

[karthikpandelu]

LGTM. This PR fixes both the issues mentioned.
Like @andy-goryachev-oracle mentioned, better to describe new behaviour in JDK-8242553 description.

[drmarmac]

/integrate

[openjdk]

@drmarmac
Your change (at version 6b38b9c) is now ready to be sponsored by a Committer.

[karthikpandelu]

/sponsor

[openjdk]

Going to push as commit d03b002.
Since your change was applied there have been 41 commits pushed to the master branch:

• f27077b: Merge
• c721a4b: Merge
• 5482b84: Merge
• 3914db2: Merge
• 34c9dcb: Merge
• 0aaaa95: Merge
• 2e16a0c: Merge
• 9208e02: Merge
• a95be13: Merge
• 9ec34b4: 8322236: Build failure after JDK-8313064
• ... and 31 more: https://git.openjdk.org/jfx/compare/47c6193c7ef264e204ba36fb4ae9f76eb0dce318...master

Your commit was automatically rebased without conflicts.

[openjdk]

@karthikpandelu @drmarmac Pushed as commit d03b002.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[kevinrushforth]

Thanks for getting this fixed.

@andy-goryachev-oracle We might want to consider a follow-on fix to change the spec (with a CSR) to document this. What do you think?

[andy-goryachev-oracle]

I like the idea of updating the spec (for SpinnerValueFactory.[Integer|Double}SpinnerFactory classes).

Do we really need the CSR for clarifying the behavior?

[kevinrushforth]

I like the idea of updating the spec (for SpinnerValueFactory.[Integer|Double}SpinnerFactory classes).

Can you file a JBS issue to track this?

Do we really need the CSR for clarifying the behavior?

If it really is just clarifying the existing behavior, maybe not. I'd like to see the changes and then decide.

[andy-goryachev-oracle]

Created https://bugs.openjdk.org/browse/JDK-8331214

@drmarmac would you like to update the javadoc or provide the description of the new behavior?

[drmarmac]

thanks for creating the issue, I can create the PR.

[andy-goryachev-oracle]

I can create the PR.

thank you!