Proposed IntegerSpinner buggy behavior correction - JDK-8242553

Wed Apr 22 12:40:08 UTC 2020

Hi Ajit,

Thanks for writing this up. I think we are clear on what should happen 
for IntegerSpinnerValueFactory and ListSpinnerValueFactory. To answer 
your two questions, I'll take the second one first:

> 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 :)

Once you switch to a proper modulo function, the easy answer is that 
negative values will "just work". A negative amountToStepBy might even 
make sense in some use cases (think of an increasing debt value), so 
there is no reason not to support it. A negative number of steps doesn't 
really make sense, since if you want to call increment(-N) you could 
equivalently call decrement(N), but again, it will "just work" as 
expected once the wrapping function is fixed. If we were designing from 
scratch I'd specify that nsteps must be >= 0, but it isn't worth it to 
make the change now (which would require a CSR) to prohibit it.

> 1. Need to decide what would be better way to fix 
> DoubleSpinnerValueFactory wrapAround behavior?

It might be helpful to compare it to the Integer flavor. By definition, 
IntegerSpinnerValueFactory represents a set of discrete values: min and 
max are two distinct values, and there is a well-defined ordering when 
wrapping. If you wrap, it should be a continuous (in both positive and 
negative directions) list of values like so:

min, min+1, min+2, ... max-2, max-1, max, min, min+1, min+2, ...

The formula is simple. After incrementing or decrementing by N * 
amountToStepBy, do the following if wrapAround is true:

     if (val < min || val > max) val = (val - min) % (max - min + 1) + min;

The above works regardless of step size because integers are naturally 
discrete values. It is well-behaved and natural to consider max and min 
being 1 apart from each other when wrapping.

By contrast, it's harder to know what the right answer is for 
DoubleSpinnerValueFactory. It's quite possible that in some cases, the 
app expects the values of DoubleValue factory to also be discrete when 
it comes to wrapping, meaning that min and max are distinct values such 
that if your step was 1.0, you might expect to go from max-1.0 to max to 
min to min+1.0 (although even that definition has problems for 
non-integer values of amountToStepBy, but it's (mostly) solvable). It's 
also quite likely for other cases -- especially ones that would lend 
themselves to wrapAround mode in graphical apps -- where it isn't a set 
of discrete values, but rather is such that min and max represent the 
same value (i.e., the same end result). The simplest example is an angle 
in degrees where 0.0 and 360.0 mean the same thing when used as a 
rotation angle. In this latter use case, stepping from 359.0 to 360.0 to 
0.0 to 1.0 would cause a discontinuous pause in motion if you mapped the 
output to a rotation angle.

I'm not sure what the right answer is, but the latter seems like a valid 
use case and I would guess at least as common as a "set of discrete 
values" for doubles. Another thing to consider is that even in what I 
will call "discrete" mode (no I'm not really proposing that we add a 
property to control it), you have to take the step size into account; 
otherwise when the step size is < 0.5 you will get stuck at max when 
incrementing by 1 amountToStepBy, unless you have special case logic for 
that.

Any thoughts from other developers?

-- Kevin

On 4/22/2020 4:48 AM, Ajit Ghaisas wrote:
> 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
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>