Skip to content

Commit d03b002

Browse files
drmarmackarthikpandelu
drmarmac
authored and
karthikpandelu
committedApr 18, 2024
8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
8193286: IntegerSpinnerFactory does not wrap value correctly Reviewed-by: angorya, kpk
·
25+823-ga
1 parent f27077b commit d03b002

File tree

4 files changed

+207
-36
lines changed

4 files changed

+207
-36
lines changed
 

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

+19-20
Original file line numberDiff line numberDiff line change
@@ -790,40 +790,39 @@ private void setText(T value) {
790790
* Convenience method to support wrapping values around their min / max
791791
* constraints. Used by the SpinnerValueFactory implementations when
792792
* the Spinner wrapAround property is true.
793+
*
794+
* This method accepts negative values, wrapping around in the other direction.
793795
*/
794796
static int wrapValue(int value, int min, int max) {
795-
if (max == 0) {
796-
throw new RuntimeException();
797-
}
797+
int span = max - min + 1;
798798

799-
int r = value % max;
800-
if (r > min && max < min) {
801-
r = r + max - min;
802-
} else if (r < min && max > min) {
803-
r = r + max - min;
799+
if (value < 0) {
800+
value = max + value % span + 1;
804801
}
805-
return r;
802+
803+
return min + (value - min) % span;
806804
}
807805

808806
/*
809807
* Convenience method to support wrapping values around their min / max
810808
* constraints. Used by the SpinnerValueFactory implementations when
811809
* the Spinner wrapAround property is true.
810+
*
811+
* This method accepts negative values, wrapping around in the other direction.
812812
*/
813-
static BigDecimal wrapValue(BigDecimal value, BigDecimal min, BigDecimal max) {
814-
if (max.doubleValue() == 0) {
815-
throw new RuntimeException();
813+
static BigDecimal wrapValue(BigDecimal currentValue, BigDecimal newValue, BigDecimal min, BigDecimal max) {
814+
if (newValue.compareTo(min) >= 0 && newValue.compareTo(max) <= 0) {
815+
return newValue;
816816
}
817817

818-
// note that this wrap method differs from the others where we take the
819-
// difference - in this approach we wrap to the min or max - it feels better
820-
// to go from 1 to 0, rather than 1 to 0.05 (where max is 1 and step is 0.05).
821-
if (value.compareTo(min) < 0) {
822-
return max;
823-
} else if (value.compareTo(max) > 0) {
824-
return min;
818+
BigDecimal span = max.subtract(min);
819+
BigDecimal remainder = newValue.remainder(span);
820+
821+
if (remainder.compareTo(BigDecimal.ZERO) == 0) {
822+
return newValue.compareTo(currentValue) >= 0 ? max : min;
825823
}
826-
return value;
824+
825+
return newValue.compareTo(max) > 0 ? min.add(remainder) : max.add(remainder);
827826
}
828827

829828

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

+12-9
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -24,6 +24,7 @@
2424
*/
2525
package javafx.scene.control;
2626

27+
import com.sun.javafx.util.Utils;
2728
import javafx.beans.NamedArg;
2829
import javafx.beans.property.BooleanProperty;
2930
import javafx.beans.property.DoubleProperty;
@@ -365,15 +366,15 @@ public final ObjectProperty<ObservableList<T>> itemsProperty() {
365366
@Override public void decrement(int steps) {
366367
final int max = getItemsSize() - 1;
367368
int newIndex = currentIndex - steps;
368-
currentIndex = newIndex >= 0 ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, 0, max + 1) : 0);
369+
currentIndex = isWrapAround() ? Spinner.wrapValue(newIndex, 0, max) : Utils.clamp(0, newIndex, max);
369370
setValue(_getValue(currentIndex));
370371
}
371372

372373
/** {@inheritDoc} */
373374
@Override public void increment(int steps) {
374375
final int max = getItemsSize() - 1;
375376
int newIndex = currentIndex + steps;
376-
currentIndex = newIndex <= max ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, 0, max + 1) : max);
377+
currentIndex = isWrapAround() ? Spinner.wrapValue(newIndex, 0, max) : Utils.clamp(0, newIndex, max);
377378
setValue(_getValue(currentIndex));
378379
}
379380

@@ -587,7 +588,7 @@ public final IntegerProperty amountToStepByProperty() {
587588
final int min = getMin();
588589
final int max = getMax();
589590
final int newIndex = getValue() - steps * getAmountToStepBy();
590-
setValue(newIndex >= min ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max) + 1 : min));
591+
setValue(isWrapAround() ? Spinner.wrapValue(newIndex, min, max) : Utils.clamp(min, newIndex, max));
591592
}
592593

593594
/** {@inheritDoc} */
@@ -596,7 +597,7 @@ public final IntegerProperty amountToStepByProperty() {
596597
final int max = getMax();
597598
final int currentValue = getValue();
598599
final int newIndex = currentValue + steps * getAmountToStepBy();
599-
setValue(newIndex <= max ? newIndex : (isWrapAround() ? Spinner.wrapValue(newIndex, min, max) - 1 : max));
600+
setValue(isWrapAround() ? Spinner.wrapValue(newIndex, min, max) : Utils.clamp(min, newIndex, max));
600601
}
601602
}
602603

@@ -846,8 +847,9 @@ public final DoubleProperty amountToStepByProperty() {
846847
final BigDecimal maxBigDecimal = BigDecimal.valueOf(getMax());
847848
final BigDecimal amountToStepByBigDecimal = BigDecimal.valueOf(getAmountToStepBy());
848849
BigDecimal newValue = currentValue.subtract(amountToStepByBigDecimal.multiply(BigDecimal.valueOf(steps)));
849-
setValue(newValue.compareTo(minBigDecimal) >= 0 ? newValue.doubleValue() :
850-
(isWrapAround() ? Spinner.wrapValue(newValue, minBigDecimal, maxBigDecimal).doubleValue() : getMin()));
850+
setValue(isWrapAround() ?
851+
Spinner.wrapValue(currentValue, newValue, minBigDecimal, maxBigDecimal).doubleValue() :
852+
Utils.clamp(minBigDecimal, newValue, maxBigDecimal).doubleValue());
851853
}
852854

853855
/** {@inheritDoc} */
@@ -857,8 +859,9 @@ public final DoubleProperty amountToStepByProperty() {
857859
final BigDecimal maxBigDecimal = BigDecimal.valueOf(getMax());
858860
final BigDecimal amountToStepByBigDecimal = BigDecimal.valueOf(getAmountToStepBy());
859861
BigDecimal newValue = currentValue.add(amountToStepByBigDecimal.multiply(BigDecimal.valueOf(steps)));
860-
setValue(newValue.compareTo(maxBigDecimal) <= 0 ? newValue.doubleValue() :
861-
(isWrapAround() ? Spinner.wrapValue(newValue, minBigDecimal, maxBigDecimal).doubleValue() : getMax()));
862+
setValue(isWrapAround() ?
863+
Spinner.wrapValue(currentValue, newValue, minBigDecimal, maxBigDecimal).doubleValue() :
864+
Utils.clamp(minBigDecimal, newValue, maxBigDecimal).doubleValue());
862865
}
863866
}
864867

‎modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java

+164-6
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,15 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
377377
assertEquals(7, (int) intValueFactory.getValue());
378378
}
379379

380+
@Test public void intSpinner_testIncrement_negativeStep() {
381+
intValueFactory.increment(-1);
382+
assertEquals(4, (int) intValueFactory.getValue());
383+
intValueFactory.increment(-2);
384+
assertEquals(2, (int) intValueFactory.getValue());
385+
intValueFactory.increment(-15);
386+
assertEquals(0, (int) intValueFactory.getValue());
387+
}
388+
380389
@Test public void intSpinner_testIncrement_manyCalls() {
381390
for (int i = 0; i < 100; i++) {
382391
intValueFactory.increment(1);
@@ -399,6 +408,15 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
399408
assertEquals(3, (int) intValueFactory.getValue());
400409
}
401410

411+
@Test public void intSpinner_testDecrement_negativeStep() {
412+
intValueFactory.decrement(-1);
413+
assertEquals(6, (int) intValueFactory.getValue());
414+
intValueFactory.decrement(-2);
415+
assertEquals(8, (int) intValueFactory.getValue());
416+
intValueFactory.decrement(-15);
417+
assertEquals(10, (int) intValueFactory.getValue());
418+
}
419+
402420
@Test public void intSpinner_testDecrement_manyCalls() {
403421
for (int i = 0; i < 100; i++) {
404422
intValueFactory.decrement(1);
@@ -432,6 +450,32 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
432450
assertEquals(2, (int) intValueFactory.getValue());
433451
}
434452

453+
@Test public void intSpinner_testWrapAround_increment_largeStep() {
454+
intValueFactory.setWrapAround(true);
455+
intValueFactory.increment(11);
456+
assertEquals(5, (int)intValueFactory.getValue());
457+
intValueFactory.increment(12);
458+
assertEquals(6, (int)intValueFactory.getValue());
459+
intValueFactory.increment(22);
460+
assertEquals(6, (int)intValueFactory.getValue());
461+
intValueFactory.increment(23);
462+
assertEquals(7, (int)intValueFactory.getValue());
463+
}
464+
465+
@Test public void intSpinner_testWrapAround_increment_negativeStep() {
466+
intValueFactory.setWrapAround(true);
467+
intValueFactory.increment(-1);
468+
assertEquals(4, (int)intValueFactory.getValue());
469+
intValueFactory.increment(-11);
470+
assertEquals(4, (int)intValueFactory.getValue());
471+
intValueFactory.increment(-12);
472+
assertEquals(3, (int)intValueFactory.getValue());
473+
intValueFactory.increment(-22);
474+
assertEquals(3, (int)intValueFactory.getValue());
475+
intValueFactory.increment(-23);
476+
assertEquals(2, (int)intValueFactory.getValue());
477+
}
478+
435479
@Test public void intSpinner_testWrapAround_decrement_oneStep() {
436480
intValueFactory.setWrapAround(true);
437481
intValueFactory.decrement(1); // 4
@@ -453,6 +497,32 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
453497
assertEquals(8, (int) intValueFactory.getValue());
454498
}
455499

500+
@Test public void intSpinner_testWrapAround_decrement_largeStep() {
501+
intValueFactory.setWrapAround(true);
502+
intValueFactory.decrement(11);
503+
assertEquals(5, (int)intValueFactory.getValue());
504+
intValueFactory.decrement(12);
505+
assertEquals(4, (int)intValueFactory.getValue());
506+
intValueFactory.decrement(22);
507+
assertEquals(4, (int)intValueFactory.getValue());
508+
intValueFactory.decrement(23);
509+
assertEquals(3, (int)intValueFactory.getValue());
510+
}
511+
512+
@Test public void intSpinner_testWrapAround_decrement_negativeStep() {
513+
intValueFactory.setWrapAround(true);
514+
intValueFactory.decrement(-1);
515+
assertEquals(6, (int)intValueFactory.getValue());
516+
intValueFactory.decrement(-11);
517+
assertEquals(6, (int)intValueFactory.getValue());
518+
intValueFactory.decrement(-12);
519+
assertEquals(7, (int)intValueFactory.getValue());
520+
intValueFactory.decrement(-22);
521+
assertEquals(7, (int)intValueFactory.getValue());
522+
intValueFactory.decrement(-23);
523+
assertEquals(8, (int)intValueFactory.getValue());
524+
}
525+
456526
@Test public void intSpinner_assertDefaultConverterIsNonNull() {
457527
assertNotNull(intValueFactory.getConverter());
458528
}
@@ -563,6 +633,11 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
563633
assertEquals(0.6, dblValueFactory.getValue(), 0);
564634
}
565635

636+
@Test public void dblSpinner_testIncrement_negativeStep() {
637+
dblValueFactory.increment(-2);
638+
assertEquals(0.4, dblValueFactory.getValue(), 0);
639+
}
640+
566641
@Test public void dblSpinner_testIncrement_manyCalls() {
567642
for (int i = 0; i < 100; i++) {
568643
dblValueFactory.increment(1);
@@ -585,6 +660,11 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
585660
assertEquals(0.4, dblValueFactory.getValue());
586661
}
587662

663+
@Test public void dblSpinner_testDecrement_negativeStep() {
664+
dblValueFactory.decrement(-2);
665+
assertEquals(0.6, dblValueFactory.getValue());
666+
}
667+
588668
@Test public void dblSpinner_testDecrement_manyCalls() {
589669
for (int i = 0; i < 100; i++) {
590670
dblValueFactory.decrement(1);
@@ -604,7 +684,6 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
604684
dblValueFactory.increment(1); // 0.90
605685
dblValueFactory.increment(1); // 0.95
606686
dblValueFactory.increment(1); // 1.00
607-
dblValueFactory.increment(1); // 0.00
608687
dblValueFactory.increment(1); // 0.05
609688
dblValueFactory.increment(1); // 0.10
610689
assertEquals(0.10, dblValueFactory.getValue(), 0);
@@ -615,9 +694,33 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
615694
dblValueFactory.setValue(0.80);
616695
dblValueFactory.increment(2); // 0.90
617696
dblValueFactory.increment(2); // 1.00
618-
dblValueFactory.increment(2); // 0.00
619697
dblValueFactory.increment(2); // 0.10
620-
assertEquals(0.10, dblValueFactory.getValue(), 0);
698+
dblValueFactory.increment(2); // 0.20
699+
assertEquals(0.2, dblValueFactory.getValue(), 0);
700+
}
701+
702+
@Test public void dblSpinner_testWrapAround_increment_largeStep() {
703+
dblValueFactory.setWrapAround(true);
704+
dblValueFactory.increment(20);
705+
assertEquals(0.5, dblValueFactory.getValue(), 0);
706+
dblValueFactory.increment(30);
707+
assertEquals(1.0, dblValueFactory.getValue(), 0);
708+
dblValueFactory.increment(40);
709+
assertEquals(1.0, dblValueFactory.getValue(), 0);
710+
dblValueFactory.increment(50);
711+
assertEquals(0.5, dblValueFactory.getValue(), 0);
712+
}
713+
714+
@Test public void dblSpinner_testWrapAround_increment_negativeStep() {
715+
dblValueFactory.setWrapAround(true);
716+
dblValueFactory.increment(-9);
717+
assertEquals(0.05, dblValueFactory.getValue());
718+
dblValueFactory.increment(-1);
719+
assertEquals(0.0, dblValueFactory.getValue());
720+
dblValueFactory.increment(-1);
721+
assertEquals(0.95, dblValueFactory.getValue());
722+
dblValueFactory.increment(-20);
723+
assertEquals(0.95, dblValueFactory.getValue());
621724
}
622725

623726
@Test public void dblSpinner_testWrapAround_decrement_oneStep() {
@@ -627,7 +730,6 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
627730
dblValueFactory.decrement(1); // 0.10
628731
dblValueFactory.decrement(1); // 0.05
629732
dblValueFactory.decrement(1); // 0.00
630-
dblValueFactory.decrement(1); // 1.00
631733
dblValueFactory.decrement(1); // 0.95
632734
dblValueFactory.decrement(1); // 0.90
633735
assertEquals(0.90, dblValueFactory.getValue(), 0);
@@ -638,9 +740,33 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
638740
dblValueFactory.setValue(0.20);
639741
dblValueFactory.decrement(2); // 0.10
640742
dblValueFactory.decrement(2); // 0.00
641-
dblValueFactory.decrement(2); // 1.00
642743
dblValueFactory.decrement(2); // 0.90
643-
assertEquals(0.90, dblValueFactory.getValue());
744+
dblValueFactory.decrement(2); // 0.80
745+
assertEquals(0.80, dblValueFactory.getValue());
746+
}
747+
748+
@Test public void dblSpinner_testWrapAround_decrement_largeStep() {
749+
dblValueFactory.setWrapAround(true);
750+
dblValueFactory.decrement(20);
751+
assertEquals(0.5, dblValueFactory.getValue());
752+
dblValueFactory.decrement(30);
753+
assertEquals(0.0, dblValueFactory.getValue());
754+
dblValueFactory.decrement(40);
755+
assertEquals(0.0, dblValueFactory.getValue());
756+
dblValueFactory.decrement(50);
757+
assertEquals(0.5, dblValueFactory.getValue());
758+
}
759+
760+
@Test public void dblSpinner_testWrapAround_decrement_negativeStep() {
761+
dblValueFactory.setWrapAround(true);
762+
dblValueFactory.decrement(-9);
763+
assertEquals(0.95, dblValueFactory.getValue());
764+
dblValueFactory.decrement(-1);
765+
assertEquals(1.0, dblValueFactory.getValue());
766+
dblValueFactory.decrement(-1);
767+
assertEquals(0.05, dblValueFactory.getValue());
768+
dblValueFactory.decrement(-20);
769+
assertEquals(0.05, dblValueFactory.getValue());
644770
}
645771

646772
@Test public void dblSpinner_assertDefaultConverterIsNonNull() {
@@ -752,6 +878,13 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
752878
assertEquals("string3", listValueFactory.getValue());
753879
}
754880

881+
@Test public void listSpinner_testIncrement_negativeStep() {
882+
listValueFactory.increment(-1);
883+
assertEquals("string1", listValueFactory.getValue());
884+
listValueFactory.increment(-15);
885+
assertEquals("string1", listValueFactory.getValue());
886+
}
887+
755888
@Test public void listSpinner_testIncrement_manyCalls() {
756889
for (int i = 0; i < 100; i++) {
757890
listValueFactory.increment(1);
@@ -774,6 +907,13 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
774907
assertEquals("string1", listValueFactory.getValue());
775908
}
776909

910+
@Test public void listSpinner_testDecrement_negativeStep() {
911+
listValueFactory.decrement(-1);
912+
assertEquals("string2", listValueFactory.getValue());
913+
listValueFactory.decrement(-15);
914+
assertEquals("string3", listValueFactory.getValue());
915+
}
916+
777917
@Test public void listSpinner_testDecrement_manyCalls() {
778918
for (int i = 0; i < 100; i++) {
779919
listValueFactory.decrement(1);
@@ -807,6 +947,15 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
807947
assertEquals("string3", listValueFactory.getValue());
808948
}
809949

950+
@Test public void listSpinner_testWrapAround_increment_negativeStep() {
951+
listValueFactory.setWrapAround(true);
952+
listValueFactory.increment(-2); // string1 -> string2
953+
listValueFactory.increment(-2); // string2 -> string3
954+
listValueFactory.increment(-2); // string3 -> string1
955+
listValueFactory.increment(-2); // string1 -> string2
956+
assertEquals("string2", listValueFactory.getValue());
957+
}
958+
810959
@Test public void listSpinner_testWrapAround_decrement_oneStep() {
811960
listValueFactory.setWrapAround(true);
812961
listValueFactory.decrement(1); // string3
@@ -828,6 +977,15 @@ public void expectExceptionWhenOneArgsDecrementCalled_noValueFactory() {
828977
assertEquals("string2", listValueFactory.getValue());
829978
}
830979

980+
@Test public void listSpinner_testWrapAround_decrement_negativeStep() {
981+
listValueFactory.setWrapAround(true);
982+
listValueFactory.decrement(-2); // string1 -> string3
983+
listValueFactory.decrement(-2); // string3 -> string2
984+
listValueFactory.decrement(-2); // string2 -> string1
985+
listValueFactory.decrement(-2); // string1 -> string3
986+
assertEquals("string3", listValueFactory.getValue());
987+
}
988+
831989
@Test public void listSpinner_assertDefaultConverterIsNonNull() {
832990
assertNotNull(listValueFactory.getConverter());
833991
}

‎modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java

+12-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -42,6 +42,7 @@
4242
import javafx.stage.Screen;
4343
import javafx.stage.Stage;
4444
import javafx.stage.Window;
45+
import java.math.BigDecimal;
4546
import java.util.List;
4647
import com.sun.javafx.PlatformUtil;
4748
import java.security.AccessController;
@@ -102,6 +103,16 @@ public static long clamp(long min, long value, long max) {
102103
return value;
103104
}
104105

106+
/**
107+
* Simple utility function which clamps the given value to be strictly
108+
* between the min and max values.
109+
*/
110+
public static BigDecimal clamp(BigDecimal min, BigDecimal value, BigDecimal max) {
111+
if (value.compareTo(min) < 0) return min;
112+
if (value.compareTo(max) > 0) return max;
113+
return value;
114+
}
115+
105116
/**
106117
* Simple utility function which clamps the given value to be strictly
107118
* above the min value.

0 commit comments

Comments
 (0)
Please sign in to comment.