Skip to content

Don't show unexpected decimals for range slider value #6699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Don't show unexpected decimals for range slider value #6699

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56c0d29
Fix for #6424
dimwight Apr 23, 2025
a8e04f8
Test for #6424
dimwight Apr 23, 2025
1ed321e
Refactor to base class
dimwight May 19, 2025
9929432
Avoid BigDecimal
dimwight May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion collect_app/src/main/java/org/odk/collect/android/widgets/range/RangeDecimalWidget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.odk.collect.android.widgets.utilities.RangeWidgetUtils;

import java.math.BigDecimal;
import java.math.RoundingMode;

@SuppressLint("ViewConstructor")
public class RangeDecimalWidget extends QuestionWidget implements Slider.OnChangeListener {
Expand Down Expand Up @@ -92,7 +93,13 @@ public void onValueChange(@NonNull Slider slider, float value, boolean fromUser)

private void setUpActualValueLabel(BigDecimal actualValue) {
if (actualValue != null) {
currentValue.setText(String.valueOf(actualValue.doubleValue()));
double doubleValue;
if (slider.getStepSize() < 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing special about a step size less than 1 as far as I can tell! Floating point precision issues can happen with any step sizes that involve decimal values. I don't know how likely this is but it would be possible to have a step size of 1.3, for example.

Have you been able to determine the underlying issue? I originally thought it was an arithmetic issue but looking at the example on the forum, I'm not sure about that anymore. Is it possible that we are calling BigDecimal(double) somewhere? If so, that could be the issue and we might need to call BigDecimal(string) instead. See https://stackoverflow.com/questions/46828044/bigdecimal-not-giving-exact-output-in-java

I think that's the most likely culprit and that it can be fixed upstream of this method. If you determine that it is in fact an arithmetic issue, then the setScale approach may be appropriate. I think we'd want to use the scale of the stepsize instead of hardcoding it to 3, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for alerting me to the root cause of the issue being the BigDecimal created in RangeWidgetUtils by getActualValue, which is currently called from both RangeDecimalWidget and RangeIntegerWidget.

Since the code of these classes is more or less identical, it can be pushed up into superclass RangeBaseWidget with a constructor parameter to switch the detail differences.

This enables getActualValue just to pass through the float received from the slider, and setUpActualValueLabel can then be adjusted to suit.

doubleValue = actualValue.setScale(3, RoundingMode.HALF_UP).doubleValue();
} else {
doubleValue = actualValue.doubleValue();
}
currentValue.setText(String.valueOf(doubleValue));
} else {
currentValue.setText("");
slider.reset();
Expand Down
38 changes: 23 additions & 15 deletions collect_app/src/test/java/org/odk/collect/android/widgets/range/RangeDecimalWidgetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
package org.odk.collect.android.widgets.range;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.mockValueChangedListener;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.promptWithQuestionDefAndAnswer;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.promptWithReadOnlyAndQuestionDef;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.widgetDependencies;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.widgetTestActivity;

import android.view.View;

import androidx.test.ext.junit.runners.AndroidJUnit4;
Expand All @@ -17,20 +31,6 @@

import java.math.BigDecimal;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.mockValueChangedListener;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.promptWithQuestionDefAndAnswer;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.promptWithReadOnlyAndQuestionDef;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.widgetDependencies;
import static org.odk.collect.android.widgets.support.QuestionWidgetHelpers.widgetTestActivity;

@RunWith(AndroidJUnit4.class)
public class RangeDecimalWidgetTest {
private static final String NO_TICKS_APPEARANCE = "no-ticks";
Expand All @@ -42,7 +42,7 @@ public void setup() {
rangeQuestion = mock(RangeQuestion.class);
when(rangeQuestion.getRangeStart()).thenReturn(BigDecimal.valueOf(1.5));
when(rangeQuestion.getRangeEnd()).thenReturn(BigDecimal.valueOf(5.5));
when(rangeQuestion.getRangeStep()).thenReturn(BigDecimal.valueOf(0.5));
when(rangeQuestion.getRangeStep()).thenReturn(BigDecimal.valueOf(0.1));
}

@Test
Expand Down Expand Up @@ -215,6 +215,14 @@ public void changingSliderValueToAnyOtherThanTheMinOne_setsTheValueCorrectly() {
assertThat(widget.currentValue.getText(), equalTo("5.5"));
}

@Test
public void changingSliderValueToAnIntermediate_setsTheValueCorrectly() {
RangeDecimalWidget widget = createWidget(promptWithQuestionDefAndAnswer(rangeQuestion, null));

SliderExtKt.clickOnFractionalValue(widget.slider, 0.3f);
assertThat(widget.currentValue.getText(), equalTo("2.3"));
}

private RangeDecimalWidget createWidget(FormEntryPrompt prompt) {
RangeDecimalWidget widget = new RangeDecimalWidget(widgetTestActivity(), new QuestionDetails(prompt), widgetDependencies());
widget.slider.layout(0, 0, 100, 10);
Expand Down
4 changes: 4 additions & 0 deletions test-shared/src/main/java/org/odk/collect/testshared/SliderExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ fun Slider.clickOnMaxValue() {
clickOnPosition(width.toFloat())
}

fun Slider.clickOnFractionalValue(fraction: Float) {
clickOnPosition(width.toFloat() * fraction)
}

fun Slider.clickOnPosition(xPosition: Float) {
val currentTime = System.currentTimeMillis()

Expand Down