8242553: IntegerSpinner and DoubleSpinner do not wrap around values c…

…orrectly in some cases

8193286: IntegerSpinnerFactory does not wrap value correctly

Reviewed-by: angorya, kpk

modules/javafx.controls/src/main/java/javafx/scene/control/Spinner.java

@@ -790,40 +790,39 @@ private void setText(T value) {
      * Convenience method to support wrapping values around their min / max
      * constraints. Used by the SpinnerValueFactory implementations when
      * the Spinner wrapAround property is true.
+     *
+     * This method accepts negative values, wrapping around in the other direction.
      */
     static int wrapValue(int value, int min, int max) {
-        if (max == 0) {
-            throw new RuntimeException();
-        }
+        int span = max - min + 1;
 
-        int r = value % max;
-        if (r > min && max < min) {
-            r = r + max - min;
-        } else if (r < min && max > min) {
-            r = r + max - min;
+        if (value < 0) {
+            value = max + value % span + 1;
         }
-        return r;
+
+        return min + (value - min) % span;
     }
 
     /*
      * Convenience method to support wrapping values around their min / max
      * constraints. Used by the SpinnerValueFactory implementations when
      * the Spinner wrapAround property is true.
+     *
+     * This method accepts negative values, wrapping around in the other direction.
      */
-    static BigDecimal wrapValue(BigDecimal value, BigDecimal min, BigDecimal max) {
-        if (max.doubleValue() == 0) {
-            throw new RuntimeException();
+    static BigDecimal wrapValue(BigDecimal currentValue, BigDecimal newValue, BigDecimal min, BigDecimal max) {
+        if (newValue.compareTo(min) >= 0 && newValue.compareTo(max) <= 0) {
+            return newValue;
         }
 
-        // note that this wrap method differs from the others where we take the
-        // difference - in this approach we wrap to the min or max - it feels better
-        // to go from 1 to 0, rather than 1 to 0.05 (where max is 1 and step is 0.05).
-        if (value.compareTo(min) < 0) {
-            return max;
-        } else if (value.compareTo(max) > 0) {
-            return min;
+        BigDecimal span = max.subtract(min);
+        BigDecimal remainder = newValue.remainder(span);
+
+        if (remainder.compareTo(BigDecimal.ZERO) == 0) {
+            return newValue.compareTo(currentValue) >= 0 ? max : min;
         }
-        return value;
+
+        return newValue.compareTo(max) > 0 ? min.add(remainder) : max.add(remainder);
     }
 
 

modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -24,6 +24,7 @@
  */
 package javafx.scene.control;
 
+import com.sun.javafx.util.Utils;
 import javafx.beans.NamedArg;
 import javafx.beans.property.BooleanProperty;
 import javafx.beans.property.DoubleProperty;
@@ -365,15 +366,15 @@ public final ObjectProperty<ObservableList<T>> itemsProperty() {
         @Override public void decrement(int steps) {
             final int max = getItemsSize() - 1;
             int newIndex = currentIndex - steps;
-            currentIndex = newIndex >= 0 ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, 0, max + 1) : 0);
+            currentIndex = isWrapAround() ? Spinner.wrapValue(newIndex, 0, max) : Utils.clamp(0, newIndex, max);
             setValue(_getValue(currentIndex));
         }
 
         /** {@inheritDoc} */
         @Override public void increment(int steps) {
             final int max = getItemsSize() - 1;
             int newIndex = currentIndex + steps;
-            currentIndex = newIndex <= max ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, 0, max + 1) : max);
+            currentIndex = isWrapAround() ? Spinner.wrapValue(newIndex, 0, max) : Utils.clamp(0, newIndex, max);
             setValue(_getValue(currentIndex));
         }
 
@@ -587,7 +588,7 @@ public final IntegerProperty amountToStepByProperty() {
             final int min = getMin();
             final int max = getMax();
             final int newIndex = getValue() - steps * getAmountToStepBy();
-            setValue(newIndex >= min ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max) + 1 : min));
+            setValue(isWrapAround() ? Spinner.wrapValue(newIndex, min, max) : Utils.clamp(min, newIndex, max));
         }
 
         /** {@inheritDoc} */
@@ -596,7 +597,7 @@ public final IntegerProperty amountToStepByProperty() {
             final int max = getMax();
             final int currentValue = getValue();
             final int newIndex = currentValue + steps * getAmountToStepBy();
-            setValue(newIndex <= max ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max) - 1 : max));
+            setValue(isWrapAround() ? Spinner.wrapValue(newIndex, min, max) : Utils.clamp(min, newIndex, max));
         }
     }
 
@@ -846,8 +847,9 @@ public final DoubleProperty amountToStepByProperty() {
             final BigDecimal maxBigDecimal = BigDecimal.valueOf(getMax());
             final BigDecimal amountToStepByBigDecimal = BigDecimal.valueOf(getAmountToStepBy());
             BigDecimal newValue = currentValue.subtract(amountToStepByBigDecimal.multiply(BigDecimal.valueOf(steps)));
-            setValue(newValue.compareTo(minBigDecimal) >= 0 ? newValue.doubleValue() :
-                    (isWrapAround() ? Spinner.wrapValue(newValue, minBigDecimal, maxBigDecimal).doubleValue() : getMin()));
+            setValue(isWrapAround() ?
+                Spinner.wrapValue(currentValue, newValue, minBigDecimal, maxBigDecimal).doubleValue() :
+                Utils.clamp(minBigDecimal, newValue, maxBigDecimal).doubleValue());
         }
 
         /** {@inheritDoc} */
@@ -857,8 +859,9 @@ public final DoubleProperty amountToStepByProperty() {
             final BigDecimal maxBigDecimal = BigDecimal.valueOf(getMax());
             final BigDecimal amountToStepByBigDecimal = BigDecimal.valueOf(getAmountToStepBy());
             BigDecimal newValue = currentValue.add(amountToStepByBigDecimal.multiply(BigDecimal.valueOf(steps)));
-            setValue(newValue.compareTo(maxBigDecimal) <= 0 ? newValue.doubleValue() :
-                    (isWrapAround() ? Spinner.wrapValue(newValue, minBigDecimal, maxBigDecimal).doubleValue() : getMax()));
+            setValue(isWrapAround() ?
+                Spinner.wrapValue(currentValue, newValue, minBigDecimal, maxBigDecimal).doubleValue() :
+                Utils.clamp(minBigDecimal, newValue, maxBigDecimal).doubleValue());
         }
     }