Proposed IntegerSpinner buggy behavior correction - JDK-8242553

[Kevin Rushforth]

I just took another look at the SpinnerValueFactory API docs. The use of 
the term "circular" heavily implies modulo arithmetic as the expected 
behavior if wrapAround is true. That the usual meaning of "wrap" versus 
"clamp" when you have a range bounded on both ends. Maybe the confusion 
comes from the fact that it isn't stated as clearly as it might be, 
coupled with a single example of a step by one going from max to min (if 
incrementing). I note that example doesn't say what the amountToStepBy 
is, but it wasn't trying to be prescriptive.

Since the current behavior of "wrap around unless you happen to wrap a 
bit too much and then we'll clamp" doesn't make sense in any universe 
that I can think of, it is fine to treat this as a bug. I'm not worried 
about "bug compatibility" here.

Clarifying the spec at the same time seems like a good idea to me. A 
related issue is that the default value of wrapAround is not specified 
(the default is `false`, but you wouldn't know that from reading the 
docs). This should also be addressed at the same time.

You mentioned that this is specific to IntegerValueFactory, but it looks 
like DoubleValueFactory (and maybe ListValueFactory) have the same bug. 
Or am I missing something?

On an unrelated note, I just noticed that the SpinnerValueFactory 
constructor has no documentation. This is because it is an implicitly 
added constructor, which is an anti-pattern for public API. I'll file a 
separate issue for that.

-- Kevin


On 4/15/2020 2:23 AM, Jeanette Winzenburg wrote:
> Hi Ajit,
>
> yes, I read the doc, probably a bit differently - could well be my 
> misunderstanding and misunderstandable wording :)
>
> Trying again:
>
> - I read your suggestion (in 
> https://bugs.openjdk.java.net/browse/JDK-8242553) to imply f.i. that 
> being at value and incrementing a full-cycle (that is max -min +1), I 
> will land on value again
> - for me the doc seemed to imply that in such a case I would land on 
> min. Though, given the "circular" as you pointed out correctly, was my 
> misunderstanding
> - the current implementation is buggy 
> (https://bugs.openjdk.java.net/browse/JDK-8193286) in calculating the 
> remainder (which is what the first bullet amounts to) incorrectly for 
> min != 0
>
> Where do I err?
>
> -- Jeanette
>
>
> Zitat von Ajit Ghaisas <ajit.ghaisas at oracle.com>:
>
>> Hi Jeanette,
>>
>> The doc never assumes amountPerStep = 1. I am quoting it here -
>> “The wrapAround property is used to specify whether the value factory 
>> should be circular. For example, should an integer-based value model 
>> increment from the maximum value back to the minimum value (and vice 
>> versa).”
>>
>> The word “circular” clarifies that once we exceed maximum value, we 
>> should start from minimum.
>> I think, the doc is OK in it’s current form, but implementation needs 
>> to be corrected.
>>
>> Regards,
>> Ajit
>>
>>
>>> On 14-Apr-2020, at 8:01 PM, Jeanette Winzenburg 
>>> <fastegal at swingempire.de> wrote:
>>>
>>>
>>> Hi Ajit,
>>>
>>> thought the doc was simply bad (in specifying the behavior for 
>>> amountPerStep = 1 and not thinking of larger amounts) - my expection 
>>> is a calculated wrap, that is the target as you suggest via modulo 
>>> the difference from current value. Don't know if anybody took the 
>>> doc literally ..
>>>
>>> -- Jeanette
>>>
>>> Zitat von Ajit Ghaisas <ajit.ghaisas at oracle.com>:
>>>
>>>> Hi,
>>>>
>>>>   Once I fix JDK-8193286, I would like to take up JDK-8242553 
>>>> (IntegerSpinner does not wrap around values correctly if 
>>>> amountToStepBy is larger than total numbers between Max and Min)
>>>>
>>>>   The current implementation is not as per what is documented.
>>>>   Refer : 
>>>> https://openjfx.io/javadoc/14/javafx.controls/javafx/scene/control/SpinnerValueFactory.html#wrapAroundProperty
>>>>
>>>>   I propose to fix the current buggy behavior of IntegerSpinner.
>>>>   Although it is a corner case, I would like to know if anybody 
>>>> relies on this buggy behavior?
>>>>
>>>> Regards,
>>>> Ajit
>>>
>>>
>>>
>
>
>