Permit slider values without options

Amolith created

Change summary

src/main/java/eu/siacs/conversations/entities/Conversation.java     | 195 
src/test/java/eu/siacs/conversations/entities/ConversationTest.java | 138 
2 files changed, 274 insertions(+), 59 deletions(-)

Detailed changes

src/main/java/eu/siacs/conversations/entities/Conversation.java 🔗

@@ -97,13 +97,14 @@ import org.json.JSONException;
 import org.json.JSONObject;
 
 import java.lang.ref.WeakReference;
+import java.math.BigDecimal;
+import java.math.RoundingMode;
 import java.time.LocalDateTime;
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeParseException;
 import java.time.format.DateTimeFormatter;
 import java.time.format.FormatStyle;
-import java.text.DecimalFormat;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -1868,6 +1869,116 @@ public class Conversation extends AbstractEntity
         }
     }
 
+    static Float steppedSliderStep(final Element field) {
+        final Element validate = field.findChild("validate", "http://jabber.org/protocol/xdata-validate");
+        final String datatype = validate == null ? null : validate.getAttribute("datatype");
+        if (!isNumericDatatype(datatype)) return null;
+
+        final Element range = validate.findChild("range", "http://jabber.org/protocol/xdata-validate");
+        final Float min = rangeFloat(range, "min");
+        final Float max = rangeFloat(range, "max");
+        if (min == null || max == null || min >= max) return null;
+
+        final String value = firstValue(field);
+        final Float parsedValue;
+        if (value == null || value.equals("")) {
+            parsedValue = null;
+        } else {
+            parsedValue = parseFloat(value);
+            if (parsedValue == null || parsedValue < min || parsedValue > max) return null;
+        }
+
+        final List<Float> options = optionValues(field);
+        if (options == null) return null;
+
+        final Float step;
+        if (options.size() < 2) {
+            step = isIntegerDatatype(datatype) ? 1f : 0f;
+        } else {
+            step = uniformStep(options);
+            if (step == null) return null;
+            for (final float option : options) {
+                if (!landsOnStep(option, min, step)) return null;
+            }
+        }
+        if (step == null || (step > 0f && !landsOnStep(max, min, step))) return null;
+        return parsedValue == null || step == 0f || landsOnStep(parsedValue, min, step) ? step : null;
+    }
+
+    static String formatSliderValue(final float value, final String datatype) {
+        final BigDecimal decimal = new BigDecimal(Float.toString(value));
+        if (isIntegerDatatype(datatype)) {
+            return decimal.setScale(0, RoundingMode.HALF_UP).toPlainString();
+        }
+        if ("xs:decimal".equals(datatype)) {
+            return decimal.stripTrailingZeros().toPlainString();
+        }
+        return Float.toString(value);
+    }
+
+    private static Float uniformStep(final List<Float> options) {
+        final List<Float> sortedOptions = new ArrayList<>(options);
+        Collections.sort(sortedOptions);
+        final float step = sortedOptions.get(1) - sortedOptions.get(0);
+        if (step <= 0f) return null;
+        for (int i = 2; i < sortedOptions.size(); i++) {
+            if (!(Math.abs(step - (sortedOptions.get(i) - sortedOptions.get(i - 1))) < 0.0001f)) return null;
+        }
+        return step;
+    }
+
+    private static List<Float> optionValues(final Element field) {
+        final List<Float> values = new ArrayList<>();
+        for (final Option option : Option.forField(field)) {
+            final Float value = parseFloat(option.getValue());
+            if (value == null) return null;
+            values.add(value);
+        }
+        return values;
+    }
+
+    private static Float rangeFloat(final Element range, final String name) {
+        return range == null ? null : parseFloat(range.getAttribute(name));
+    }
+
+    private static Float parseFloat(final String value) {
+        if (value == null || value.equals("")) return null;
+        try {
+            final float parsed = Float.parseFloat(value);
+            return Float.isFinite(parsed) ? parsed : null;
+        } catch (final NumberFormatException e) {
+            return null;
+        }
+    }
+
+    private static String firstValue(final Element field) {
+        for (final Element child : field.getChildren()) {
+            if ("value".equals(child.getName()) && "jabber:x:data".equals(child.getNamespace())) {
+                return child.getContent();
+            }
+        }
+        return null;
+    }
+
+    private static boolean landsOnStep(final float value, final float min, final float step) {
+        final double multiple = ((double) value - (double) min) / (double) step;
+        return Math.abs(Math.rint(multiple) - multiple) < 0.0001f;
+    }
+
+    private static boolean isIntegerDatatype(final String datatype) {
+        return "xs:integer".equals(datatype)
+                || "xs:int".equals(datatype)
+                || "xs:long".equals(datatype)
+                || "xs:short".equals(datatype)
+                || "xs:byte".equals(datatype);
+    }
+
+    private static boolean isNumericDatatype(final String datatype) {
+        return isIntegerDatatype(datatype)
+                || "xs:decimal".equals(datatype)
+                || "xs:double".equals(datatype);
+    }
+
     public class ConversationPagerAdapter extends PagerAdapter {
         protected WeakReference<ViewPager> mPager = new WeakReference<>(null);
         protected WeakReference<TabLayout> mTabs = new WeakReference<>(null);
@@ -2229,7 +2340,7 @@ public class Conversation extends AbstractEntity
                     String datatype = validate.getAttribute("datatype");
                     if (datatype == null) return;
 
-                    if (datatype.equals("xs:integer") || datatype.equals("xs:int") || datatype.equals("xs:long") || datatype.equals("xs:short") || datatype.equals("xs:byte")) {
+                    if (isIntegerDatatype(datatype)) {
                         textinput.setInputType(flags | InputType.TYPE_CLASS_NUMBER | InputType.TYPE_NUMBER_FLAG_SIGNED);
                     }
 
@@ -2919,63 +3030,32 @@ public class Conversation extends AbstractEntity
                 @Override
                 public void bind(Item item) {
                     field = (Field) item;
+                    binding.slider.clearOnChangeListeners();
                     setTextOrHide(binding.label, field.getLabel());
                     setTextOrHide(binding.desc, field.getDesc());
                     final Element validate = field.el.findChild("validate", "http://jabber.org/protocol/xdata-validate");
                     final String datatype = validate == null ? null : validate.getAttribute("datatype");
                     final Element range = validate == null ? null : validate.findChild("range", "http://jabber.org/protocol/xdata-validate");
-                    // NOTE: range also implies open, so we don't have to be bound by the options strictly
-                    // Also, we don't have anywhere to put labels so we show only values, which might sometimes be bad...
-                    Float min = null;
-                    try { min = range.getAttribute("min") == null ? null : Float.valueOf(range.getAttribute("min")); } catch (NumberFormatException e) { }
-                    Float max = null;
-                    try { max = range.getAttribute("max") == null ? null : Float.valueOf(range.getAttribute("max"));  } catch (NumberFormatException e) { }
-
-                    List<Float> options = field.getOptions().stream().map(o -> Float.valueOf(o.getValue())).collect(Collectors.toList());
-                    Collections.sort(options);
-                    if (options.size() > 0) {
-                        // min/max should be on the range, but if you have options and didn't put them on the range we can imply
-                        if (min == null) min = options.get(0);
-                        if (max == null) max = options.get(options.size()-1);
-                    }
-
-                    if (field.getValues().size() > 0) {
-                        final var val = Float.valueOf(field.getValue().getContent());
-                        if ((min == null || val >= min) && (max == null || val <= max)) {
-                            binding.slider.setValue(Float.valueOf(field.getValue().getContent()));
-                        } else {
-                            binding.slider.setValue(min == null ? Float.MIN_VALUE : min);
-                        }
-                    } else {
-                        binding.slider.setValue(min == null ? Float.MIN_VALUE : min);
+                    final Float step = steppedSliderStep(field.el);
+                    if (step == null) return;
+                    final Float min = rangeFloat(range, "min");
+                    final Float max = rangeFloat(range, "max");
+                    if (min == null || max == null) return;
+
+                    float value = min;
+                    final Float parsedValue = parseFloat(firstValue(field.el));
+                    if (parsedValue != null && parsedValue >= min && parsedValue <= max) {
+                        value = parsedValue;
                     }
-                    binding.slider.setValueFrom(min == null ? Float.MIN_VALUE : min);
-                    binding.slider.setValueTo(max == null ? Float.MAX_VALUE : max);
-                    if ("xs:integer".equals(datatype) || "xs:int".equals(datatype) || "xs:long".equals(datatype) || "xs:short".equals(datatype) || "xs:byte".equals(datatype)) {
-                        binding.slider.setStepSize(1);
-                    } else {
-                        binding.slider.setStepSize(0);
-                    }
-
-                    if (options.size() > 0) {
-                        float step = -1;
-                        Float prev = null;
-                        for (final Float option : options) {
-                            if (prev != null) {
-                                float nextStep = option - prev;
-                                if (step > 0 && step != nextStep) {
-                                    step = -1;
-                                    break;
-                                }
-                                step = nextStep;
-                            }
-                            prev = option;
-                        }
-                        if (step > 0) binding.slider.setStepSize(step);
-                    }
-
-                    binding.slider.addOnChangeListener((slider, value, fromUser) -> {
-                        field.setValues(List.of(new DecimalFormat().format(value)));
+                    binding.slider.setStepSize(0);
+                    binding.slider.setValueFrom(min);
+                    binding.slider.setValueTo(max);
+                    binding.slider.setValue(value);
+                    binding.slider.setStepSize(step);
+
+                    binding.slider.addOnChangeListener((slider, sliderValue, fromUser) -> {
+                        if (!fromUser) return;
+                        field.setValues(List.of(formatSliderValue(sliderValue, datatype)));
                     });
                 }
             }
@@ -3164,13 +3244,10 @@ public class Conversation extends AbstractEntity
                                 viewType = TYPE_CHECKBOX_FIELD;
                             }
                         } else if (
-                            range != null && range.getAttribute("min") != null && range.getAttribute("max") != null && (
-                                "xs:integer".equals(datatype) || "xs:int".equals(datatype) || "xs:long".equals(datatype) || "xs:short".equals(datatype) || "xs:byte".equals(datatype) ||
-                                "xs:decimal".equals(datatype) || "xs:double".equals(datatype)
-                            )
+                            range != null && range.getAttribute("min") != null && range.getAttribute("max") != null && isNumericDatatype(datatype)
                         ) {
-                            // has a range and is numeric, use a slider
-                            viewType = TYPE_SLIDER_FIELD;
+                            // has a range and is numeric, use a slider if it can represent every valid value
+                            viewType = steppedSliderStep(el) == null ? TYPE_TEXT_FIELD : TYPE_SLIDER_FIELD;
                         } else if (fieldType.equals("list-single")) {
                             if (fillableFieldCount == 1 && actionsAdapter.countProceed() < 1 && Option.forField(el).size() < 50) {
                                 viewType = TYPE_BUTTON_GRID_FIELD;

src/test/java/eu/siacs/conversations/entities/ConversationTest.java 🔗

@@ -19,6 +19,8 @@ import org.robolectric.annotation.ConscryptMode;
 
 import android.os.Build;
 import android.os.Looper;
+import android.view.ContextThemeWrapper;
+import android.view.LayoutInflater;
 import android.view.View;
 import android.widget.ListView;
 import android.widget.RelativeLayout;
@@ -26,6 +28,9 @@ import androidx.viewpager.widget.ViewPager;
 import com.google.android.material.tabs.TabLayout;
 import eu.siacs.conversations.Conversations;
 import eu.siacs.conversations.R;
+import eu.siacs.conversations.databinding.CommandSliderFieldBinding;
+import eu.siacs.conversations.services.XmppConnectionService;
+import eu.siacs.conversations.xml.Element;
 import eu.siacs.conversations.xml.Namespace;
 import eu.siacs.conversations.xmpp.Jid;
 import im.conversations.android.xmpp.model.disco.info.Feature;
@@ -173,4 +178,137 @@ public class ConversationTest {
             1,
             tabs.getSelectedTabPosition());
     }
+
+    @Test
+    public void steppedSliderStepRejectsInRangeValueMissingFromOptionLattice() {
+        final var field = sliderField("xs:integer", "0", "68", "7", "0", "34", "68");
+
+        Assert.assertNull(
+                "A value the option-derived slider cannot represent should fall back to text input",
+                Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void steppedSliderStepAcceptsCompatibleOptionLattice() {
+        final var field = sliderField("xs:integer", "0", "70", "35", "0", "35", "70");
+
+        Assert.assertEquals(Float.valueOf(35f), Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void steppedSliderStepUsesIntegerStepWithoutOptions() {
+        final var field = sliderField("xs:integer", "0", "10", "7");
+
+        Assert.assertEquals(Float.valueOf(1f), Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void steppedSliderStepUsesContinuousStepWithoutOptionsForDecimal() {
+        final var field = sliderField("xs:decimal", "0", "1", "0.000001");
+
+        Assert.assertEquals(Float.valueOf(0f), Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void formatSliderValueDoesNotClampLargeIntegerDatatypesToInt() {
+        Assert.assertEquals("3000000000", Conversation.formatSliderValue(3_000_000_000f, "xs:long"));
+    }
+
+    @Test
+    public void formatSliderValueUsesPlainDecimalLexicalFormForDecimalDatatype() {
+        Assert.assertEquals("0.000001", Conversation.formatSliderValue(0.000001f, "xs:decimal"));
+    }
+
+    @Test
+    public void steppedSliderStepRejectsIncompatibleBounds() {
+        final var field = sliderField("xs:integer", "0", "70", "34", "0", "34", "68");
+
+        Assert.assertNull(Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void steppedSliderStepRejectsMalformedOptions() {
+        final var field = sliderField("xs:integer", "0", "70", "35", "0", "not-a-number", "70");
+
+        Assert.assertNull(Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void steppedSliderStepRejectsValuesOutsideRange() {
+        final var below = sliderField("xs:integer", "0", "70", "-5", "0", "35", "70");
+        final var above = sliderField("xs:integer", "0", "70", "999", "0", "35", "70");
+
+        Assert.assertNull(Conversation.steppedSliderStep(below));
+        Assert.assertNull(Conversation.steppedSliderStep(above));
+    }
+
+    @Test
+    public void steppedSliderStepRejectsDuplicateOptions() {
+        final var field = sliderField("xs:integer", "0", "70", "35", "0", "35", "35", "70");
+
+        Assert.assertNull(Conversation.steppedSliderStep(field));
+    }
+
+    @Test
+    public void rangedListSingleWithCustomValueFallsBackToTextInputWhenSliderCannotRepresentIt() {
+        final var session = withOccupantId.pagerAdapter.new CommandSession(
+                "test", "node", mock(XmppConnectionService.class));
+        try {
+            session.responseElement = new Element("x", Namespace.DATA);
+            session.responseElement.setAttribute("type", "form");
+            final var field = sliderField("xs:integer", "0", "68", "7", "0", "34", "68");
+            field.setAttribute("type", "list-single");
+
+            Assert.assertEquals(session.TYPE_TEXT_FIELD, session.mkField(field).viewType);
+        } finally {
+            session.loadingTimer.cancel();
+        }
+    }
+
+    @Test
+    public void sliderFieldViewHolderBindsEmptyValueWithoutCrashing() {
+        final var context = new ContextThemeWrapper(
+                RuntimeEnvironment.getApplication(),
+                com.google.android.material.R.style.Theme_MaterialComponents_DayNight_NoActionBar);
+        final var session = withOccupantId.pagerAdapter.new CommandSession(
+                "test", "node", mock(XmppConnectionService.class));
+        try {
+            final var binding = CommandSliderFieldBinding.inflate(LayoutInflater.from(context));
+            final var holder = session.new SliderFieldViewHolder(binding);
+            final var field = sliderField("xs:integer", "0", "70", "", "0", "35", "70");
+            final var item = session.new Field(
+                    eu.siacs.conversations.xmpp.forms.Field.parse(field),
+                    session.TYPE_SLIDER_FIELD);
+
+            holder.bind(item);
+
+            Assert.assertEquals(0f, binding.slider.getValue(), 0.0001f);
+        } finally {
+            session.loadingTimer.cancel();
+        }
+    }
+
+    private static Element sliderField(
+            final String datatype,
+            final String min,
+            final String max,
+            final String value,
+            final String... options) {
+        final var field = new Element("field", Namespace.DATA);
+        field.setAttribute("type", "text-single");
+        final var validate = field.addChild("validate", "http://jabber.org/protocol/xdata-validate");
+        validate.setAttribute("datatype", datatype);
+        final var range = validate.addChild("range", "http://jabber.org/protocol/xdata-validate");
+        range.setAttribute("min", min);
+        range.setAttribute("max", max);
+        if (value != null) {
+            field.addChild("value", Namespace.DATA).setContent(value);
+        }
+        for (final String option : options) {
+            field.addChild("option", Namespace.DATA)
+                    .addChild("value", Namespace.DATA)
+                    .setContent(option);
+        }
+        return field;
+    }
 }