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
