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 <faste...@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.rushfo...@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.ghai...@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 <faste...@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.ghai...@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 >>>>> >>>>> >>>>> >>> >>> >>> > > >
