Fix ComboBox paging when number of items equals page length (#20213)

When pageLength == number of items, no paging should be shown except
if null selection is allowed, then there should be a second page
with one item.

Change-Id: I01c00f0c9d75a1dbb80d11b07c37c8ad7334ea07

diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java
index 562173a9bfe9b996e8dada0e551026390f6e5026..ce3526af535fb9fc08da0490f7cd3464db588de3 100644 (file)
--- a/server/src/main/java/com/vaadin/ui/ComboBox.java
+++ b/server/src/main/java/com/vaadin/ui/ComboBox.java
@@ -264,7 +264,7 @@ public class ComboBox extends AbstractSelect implements
                 // filtering
                 options = getFilteredOptions();
                 filteredSize = options.size();
-                options = sanitetizeList(options, nullOptionVisible);
+                options = sanitizeList(options, nullOptionVisible);
             }
 
             final boolean paintNullSelection = needNullSelectOption
@@ -408,7 +408,7 @@ public class ComboBox extends AbstractSelect implements
      * {@link #canUseContainerFilter()}).
      * 
      * Use {@link #getFilteredOptions()} and
-     * {@link #sanitetizeList(List, boolean)} if this is not the case.
+     * {@link #sanitizeList(List, boolean)} if this is not the case.
      * 
      * @param needNullSelectOption
      * @return filtered list of options (may be empty) or null if cannot use
@@ -524,22 +524,25 @@ public class ComboBox extends AbstractSelect implements
 
     /**
      * Makes correct sublist of given list of options.
-     * 
+     * <p>
      * If paint is not an option request (affected by page or filter change),
      * page will be the one where possible selection exists.
-     * 
+     * <p>
      * Detects proper first and last item in list to return right page of
      * options. Also, if the current page is beyond the end of the list, it will
      * be adjusted.
+     * <p>
+     * Package private only for testing purposes.
      * 
      * @param options
      * @param needNullSelectOption
      *            flag to indicate if nullselect option needs to be taken into
      *            consideration
      */
-    private List<?> sanitetizeList(List<?> options, boolean needNullSelectOption) {
-
-        if (pageLength != 0 && options.size() > pageLength) {
+    List<?> sanitizeList(List<?> options, boolean needNullSelectOption) {
+        int totalRows = options.size() + (needNullSelectOption ? 1 : 0);
+        if (pageLength != 0 && totalRows > pageLength) {
+            // options will not fit on one page
 
             int indexToEnsureInView = -1;
 

diff --git a/server/src/test/java/com/vaadin/ui/ComboBoxTest.java b/server/src/test/java/com/vaadin/ui/ComboBoxTest.java
new file mode 100644 (file)
index 0000000..b118cb8
--- /dev/null
+++ b/server/src/test/java/com/vaadin/ui/ComboBoxTest.java
@@ -0,0 +1,158 @@
+package com.vaadin.ui;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.vaadin.shared.ui.combobox.FilteringMode;
+
+public class ComboBoxTest {
+
+    private ComboBox comboBox;
+
+    @Before
+    public void setup() {
+        comboBox = new ComboBox();
+        comboBox.setLocale(Locale.ENGLISH);
+    }
+
+    @Test
+    public void options_noFilter() {
+        ComboBox comboBox = new ComboBox();
+        for (int i = 0; i < 10; i++) {
+            comboBox.addItem(i);
+        }
+
+        List<?> options = comboBox.getFilteredOptions();
+        Assert.assertEquals(10, options.size());
+        for (int i = 0; i < 10; i++) {
+            Assert.assertEquals(i, options.get(i));
+        }
+
+    }
+
+    @Test
+    public void options_inMemoryFilteringStartsWith() {
+        for (int i = 0; i < 21; i++) {
+            comboBox.addItem(i);
+        }
+
+        setFilterAndCurrentPage(comboBox, "1", 0);
+
+        List<?> options = comboBox.getFilteredOptions();
+        Assert.assertEquals(11, options.size());
+
+    }
+
+    @Test
+    public void options_inMemoryFilteringContains() {
+        comboBox.setFilteringMode(FilteringMode.CONTAINS);
+        for (int i = 0; i < 21; i++) {
+            comboBox.addItem(i);
+        }
+
+        setFilterAndCurrentPage(comboBox, "2", 0);
+        List<?> options = comboBox.getFilteredOptions();
+        Assert.assertEquals(3, options.size());
+
+    }
+
+    private static void setFilterAndCurrentPage(ComboBox comboBox,
+            String filterString, int currentPage) {
+        Map<String, Object> variables = new HashMap<String, Object>();
+        variables.put("filter", filterString);
+        variables.put("page", currentPage);
+        comboBox.changeVariables(null, variables);
+
+    }
+
+    @Test
+    public void getOptions_moreThanOnePage_noNullItem() {
+        int nrOptions = comboBox.getPageLength() * 2;
+        for (int i = 0; i < nrOptions; i++) {
+            comboBox.addItem(i);
+        }
+        setFilterAndCurrentPage(comboBox, "", 0);
+
+        List<?> goingToClient = comboBox
+                .sanitizeList(comboBox.getFilteredOptions(), false);
+        Assert.assertEquals(comboBox.getPageLength(), goingToClient.size());
+
+    }
+
+    @Test
+    public void getOptions_moreThanOnePage_nullItem() {
+        int nrOptions = comboBox.getPageLength() * 2;
+        for (int i = 0; i < nrOptions; i++) {
+            comboBox.addItem(i);
+        }
+
+        setFilterAndCurrentPage(comboBox, "", 0);
+        List<?> goingToClient = comboBox
+                .sanitizeList(comboBox.getFilteredOptions(), true);
+        // Null item is shown on first page
+        Assert.assertEquals(comboBox.getPageLength() - 1, goingToClient.size());
+
+        setFilterAndCurrentPage(comboBox, "", 1);
+        goingToClient = comboBox.sanitizeList(comboBox.getFilteredOptions(),
+                true);
+        // Null item is not shown on the second page
+        Assert.assertEquals(comboBox.getPageLength(), goingToClient.size());
+
+    }
+
+    @Test
+    public void getOptions_lessThanOnePage_noNullItem() {
+        int nrOptions = comboBox.getPageLength() / 2;
+        for (int i = 0; i < nrOptions; i++) {
+            comboBox.addItem(i);
+        }
+        setFilterAndCurrentPage(comboBox, "", 0);
+
+        List<?> goingToClient = comboBox
+                .sanitizeList(comboBox.getFilteredOptions(), false);
+        Assert.assertEquals(nrOptions, goingToClient.size());
+
+    }
+
+    @Test
+    public void getOptions_lessThanOnePage_withNullItem() {
+        int nrOptions = comboBox.getPageLength() / 2;
+        for (int i = 0; i < nrOptions; i++) {
+            comboBox.addItem(i);
+        }
+        setFilterAndCurrentPage(comboBox, "", 0);
+
+        List<?> goingToClient = comboBox
+                .sanitizeList(comboBox.getFilteredOptions(), true);
+        // All items + null still fit on one page
+        Assert.assertEquals(nrOptions, goingToClient.size());
+
+    }
+
+    @Test
+    public void getOptions_exactlyOnePage_withNullItem() {
+        int nrOptions = comboBox.getPageLength();
+        for (int i = 0; i < nrOptions; i++) {
+            comboBox.addItem(i);
+        }
+        setFilterAndCurrentPage(comboBox, "", 0);
+
+        List<?> goingToClient = comboBox
+                .sanitizeList(comboBox.getFilteredOptions(), true);
+        // Null item on first page
+        Assert.assertEquals(nrOptions - 1, goingToClient.size());
+
+        setFilterAndCurrentPage(comboBox, "", 1);
+        goingToClient = comboBox.sanitizeList(comboBox.getFilteredOptions(),
+                true);
+        // All but one was on the first page
+        Assert.assertEquals(1, goingToClient.size());
+
+    }
+}