Proposed IntegerSpinner buggy behavior correction - JDK-8242553

Wed Apr 22 11:48:00 UTC 2020

Jeanette and Kevin,

Thanks for taking a look at this in detail.

Regarding DoubleSpinnerValueFactory - 
The wraparound behavior is incorrect with DoubleSpinnerValueFactory as well. 
If amountToStepBy is lesser than max-min, once we try to increment from max, we just clamp to min and start incrementing from there.
If amountToStepBy is greater than max-min, once we try to increment from max, we clamp it to min - and further increments just keep on calming to min.

Regarding ListSpinnerValueFactory - 
It fares better than other two SpinnerValueFactories - as ListSpinnerValueFactory  does not support amountToStepBy in constructor.
min is alway 0, and max is (number of items in the list - 1).
There is a way to specify number of steps in increment and decrement functions. If we specify this to be more than number of items in the list, modulo calculation is in place to select correct value.
Nothing needs to be done for ListSpinnerValueFactory as of now - but it uses common Spinner.wrapValue() method.
Need to test it after correcting the modulo logic for IntegerSpinnerValueFactory.

What I garnered from this discussion is - below changes need to be made -

SpinnerValueFactory:
- Update the documentation about the “circular” way of wrap around behavior to be more explicit
- Update the documentation about default value of wraparound property

IntegerSpinnerValueFactory:
- Fix the wraparound behavior for all valid values (smaller and larger amountToStepBy * no of steps) - The correct way to fix this is to do modulo calculation

DoubleSpinnerValueFactory:
- Fix the wraparound behavior for all valid values (smaller and larger amountToStepBy* no of steps) - Need to decide what would be the best fit for this as the current implementation is to clamp and again continue from there. One possible options is to mimic the logic (with Double values) to match what we will end up doing for IntegerSpinnerValueFactory.

ListSpinnerValueFactory:
- Test that it continues to work as it works now after fixing IntegerSpinnerValueFactory.

Two questions :
1. Need to decide what would be better way to fix DoubleSpinnerValueFactory wrapAround behavior?
2. Do we really care about negative values in no of steps OR amountToStepBy?
I am asking this as we already have separate increment() and decrement() methods. Should we bar such negative values? That’s another behavior change though :)

Regards,
Ajit

> On 21-Apr-2020, at 4:36 PM, Jeanette Winzenburg <fastegal at swingempire.de> wrote:
> 
> 
> part of the confusion might be the reporter's workaround in https://bugs.openjdk.java.net/browse/JDK-8193286 which effectively clamps (doesn't make a difference, having amountPerStep=1 and not incrementing programatically)
> 
> commented that bug with an alternative implemenation and a couple of tests (for the most simple case ;)
> 
> -- Jeanette
> 
> Zitat von Kevin Rushforth <kevin.rushforth at oracle.com>:
> 
>> 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
>>>>> 
>>>>> 
>>>>> 
>>> 
>>> 
>>> 
> 
> 
> 