Clean up slider field binding

Amolith created 3 days ago

Compute slider prerequisites before mutating the recycled view holder.
If an invalid field reaches the slider holder, fail fast instead of
leaving a partially rebound view.

Cover rebinding one holder across incompatible slider step sizes,
including a forced draw, to verify the old step-size reset is not
needed.

Change summary

src/main/java/eu/siacs/conversations/entities/Conversation.java     | 22 
src/test/java/eu/siacs/conversations/entities/ConversationTest.java | 66 
2 files changed, 78 insertions(+), 10 deletions(-)

Detailed changes

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

@@ -3029,25 +3029,27 @@ 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 Field boundField = (Field) item;
+                    final Element validate = boundField.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");
-                    final Float step = steppedSliderStep(field.el);
-                    if (step == null) return;
+                    final Float step = steppedSliderStep(boundField.el);
                     final Float min = rangeFloat(range, "min");
                     final Float max = rangeFloat(range, "max");
-                    if (min == null || max == null) return;
+                    if (step == null || min == null || max == null) {
+                        throw new IllegalStateException("Invalid slider field bound to slider view holder");
+                    }
 
                     float value = min;
-                    final Float parsedValue = parseFloat(firstValue(field.el));
+                    final Float parsedValue = parseFloat(firstValue(boundField.el));
                     if (parsedValue != null && parsedValue >= min && parsedValue <= max) {
                         value = parsedValue;
                     }
-                    binding.slider.setStepSize(0);
+
+                    field = boundField;
+                    binding.slider.clearOnChangeListeners();
+                    setTextOrHide(binding.label, field.getLabel());
+                    setTextOrHide(binding.desc, field.getDesc());
                     binding.slider.setValueFrom(min);
                     binding.slider.setValueTo(max);
                     binding.slider.setValue(value);

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

@@ -17,6 +17,8 @@ import org.robolectric.Shadows;
 import org.robolectric.annotation.Config;
 import org.robolectric.annotation.ConscryptMode;
 
+import android.graphics.Bitmap;
+import android.graphics.Canvas;
 import android.os.Build;
 import android.os.Looper;
 import android.view.ContextThemeWrapper;
@@ -288,6 +290,70 @@ public class ConversationTest {
         }
     }
 
+    @Test
+    public void sliderFieldViewHolderRebindsDifferentStepSizesWithoutCrashing() {
+        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 steppedField = sliderField("xs:integer", "0", "70", "35", "0", "35", "70");
+            holder.bind(session.new Field(
+                    eu.siacs.conversations.xmpp.forms.Field.parse(steppedField),
+                    session.TYPE_SLIDER_FIELD));
+
+            final var integerField = sliderField("xs:integer", "0", "10", "7");
+            holder.bind(session.new Field(
+                    eu.siacs.conversations.xmpp.forms.Field.parse(integerField),
+                    session.TYPE_SLIDER_FIELD));
+
+            Assert.assertEquals(0f, binding.slider.getValueFrom(), 0.0001f);
+            Assert.assertEquals(10f, binding.slider.getValueTo(), 0.0001f);
+            Assert.assertEquals(7f, binding.slider.getValue(), 0.0001f);
+            Assert.assertEquals(1f, binding.slider.getStepSize(), 0.0001f);
+        } finally {
+            session.loadingTimer.cancel();
+        }
+    }
+
+    @Test
+    public void sliderFieldViewHolderRebindsFromLargeDiscreteToSmallDiscreteRangeWithoutCrashing() {
+        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 discreteField = sliderField("xs:integer", "0", "100", "50", "0", "10", "20");
+            holder.bind(session.new Field(
+                    eu.siacs.conversations.xmpp.forms.Field.parse(discreteField),
+                    session.TYPE_SLIDER_FIELD));
+
+            final var continuousField = sliderField(
+                    "xs:decimal", "0", "1", "0.5", "0", "0.1", "0.2", "0.3", "0.4", "0.5", "0.6", "0.7", "0.8", "0.9", "1");
+            holder.bind(session.new Field(
+                    eu.siacs.conversations.xmpp.forms.Field.parse(continuousField),
+                    session.TYPE_SLIDER_FIELD));
+
+            Assert.assertEquals(0f, binding.slider.getValueFrom(), 0.0001f);
+            Assert.assertEquals(1f, binding.slider.getValueTo(), 0.0001f);
+            Assert.assertEquals(0.5f, binding.slider.getValue(), 0.0001f);
+            Assert.assertEquals(0.1f, binding.slider.getStepSize(), 0.0001f);
+            binding.slider.measure(
+                    View.MeasureSpec.makeMeasureSpec(100, View.MeasureSpec.EXACTLY),
+                    View.MeasureSpec.makeMeasureSpec(100, View.MeasureSpec.EXACTLY));
+            binding.slider.layout(0, 0, 100, 100);
+            binding.slider.draw(new Canvas(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888)));
+        } finally {
+            session.loadingTimer.cancel();
+        }
+    }
+
     private static Element sliderField(
             final String datatype,
             final String min,