Preserve selection order for multi select value (#8109, #8787)

svn changeset:24030/svn branch:6.8

4 files changed, 309 insertions, 15 deletions

diff --git a/src/com/vaadin/ui/AbstractSelect.java b/src/com/vaadin/ui/AbstractSelect.java
index bb49626741..71624804fa 100644
--- a/src/com/vaadin/ui/AbstractSelect.java
+++ b/src/com/vaadin/ui/AbstractSelect.java
@@ -426,12 +426,15 @@ public abstract class AbstractSelect extends AbstractField implements
                 // (non-visible items can not be deselected)
                 final Collection<?> visible = getVisibleItemIds();
                 if (visible != null) {
+                    // Don't remove those that will be added to preserve order
+                    visible.removeAll(s);
+
                     @SuppressWarnings("unchecked")
                     Set<Object> newsel = (Set<Object>) getValue();
                     if (newsel == null) {
-                        newsel = new HashSet<Object>();
+                        newsel = new LinkedHashSet<Object>();
                     } else {
-                        newsel = new HashSet<Object>(newsel);
+                        newsel = new LinkedHashSet<Object>(newsel);
                     }
                     newsel.removeAll(visible);
                     newsel.addAll(s);
@@ -645,10 +648,10 @@ public abstract class AbstractSelect extends AbstractField implements
 
         if (isMultiSelect()) {
             if (newValue == null) {
-                super.setValue(new HashSet<Object>(), repaintIsNotNeeded);
+                super.setValue(new LinkedHashSet<Object>(), repaintIsNotNeeded);
             } else if (Collection.class.isAssignableFrom(newValue.getClass())) {
-                super.setValue(new HashSet<Object>((Collection<?>) newValue),
-                        repaintIsNotNeeded);
+                super.setValue(new LinkedHashSet<Object>(
+                        (Collection<?>) newValue), repaintIsNotNeeded);
             }
         } else if (newValue == null || items.containsId(newValue)) {
             super.setValue(newValue, repaintIsNotNeeded);
diff --git a/src/com/vaadin/ui/Table.java b/src/com/vaadin/ui/Table.java
index 60b6122270..c4d90d7790 100644
--- a/src/com/vaadin/ui/Table.java
+++ b/src/com/vaadin/ui/Table.java
@@ -2295,7 +2295,7 @@ public class Table extends AbstractSelect implements Action.Container,
      * @return
      */
     private Set<Object> getItemIdsInRange(Object itemId, final int length) {
-        HashSet<Object> ids = new HashSet<Object>();
+        HashSet<Object> ids = new LinkedHashSet<Object>();
         for (int i = 0; i < length; i++) {
             assert itemId != null; // should not be null unless client-server
                                    // are out of sync
@@ -2318,18 +2318,12 @@ public class Table extends AbstractSelect implements Action.Container,
         Set<Object> renderedItemIds = getCurrentlyRenderedItemIds();
 
         @SuppressWarnings("unchecked")
-        HashSet<Object> newValue = new HashSet<Object>(
+        HashSet<Object> newValue = new LinkedHashSet<Object>(
                 (Collection<Object>) getValue());
 
         if (variables.containsKey("clearSelections")) {
             // the client side has instructed to swipe all previous selections
             newValue.clear();
-        } else {
-            /*
-             * first clear all selections that are currently rendered rows (the
-             * ones that the client side counterpart is aware of)
-             */
-            newValue.removeAll(renderedItemIds);
         }
 
         /*
@@ -2346,6 +2340,7 @@ public class Table extends AbstractSelect implements Action.Container,
                 requestRepaint();
             } else if (id != null && containsId(id)) {
                 newValue.add(id);
+                renderedItemIds.remove(id);
             }
         }
 
@@ -2355,9 +2350,17 @@ public class Table extends AbstractSelect implements Action.Container,
                 String[] split = range.split("-");
                 Object startItemId = itemIdMapper.get(split[0]);
                 int length = Integer.valueOf(split[1]);
-                newValue.addAll(getItemIdsInRange(startItemId, length));
+                Set<Object> itemIdsInRange = getItemIdsInRange(startItemId,
+                        length);
+                newValue.addAll(itemIdsInRange);
+                renderedItemIds.removeAll(itemIdsInRange);
             }
         }
+        /*
+         * finally clear all currently rendered rows (the ones that the client
+         * side counterpart is aware of) that the client didn't send as selected
+         */
+        newValue.removeAll(renderedItemIds);
 
         if (!isNullSelectionAllowed() && newValue.isEmpty()) {
             // empty selection not allowed, keep old value
diff --git a/tests/testbench/com/vaadin/tests/components/abstractfield/AbstractFieldTest.java b/tests/testbench/com/vaadin/tests/components/abstractfield/AbstractFieldTest.java
index 40cc2948ee..c1107061ed 100644
--- a/tests/testbench/com/vaadin/tests/components/abstractfield/AbstractFieldTest.java
+++ b/tests/testbench/com/vaadin/tests/components/abstractfield/AbstractFieldTest.java
@@ -28,6 +28,8 @@ public abstract class AbstractFieldTest<T extends AbstractField> extends
         AbstractComponentTest<T> implements ValueChangeListener,
         ReadOnlyStatusChangeListener, FocusListener, BlurListener {
 
+    private boolean sortValueChanges = true;
+
     @Override
     protected void createActions() {
         super.createActions();
@@ -69,6 +71,17 @@ public abstract class AbstractFieldTest<T extends AbstractField> extends
                     }
                 }
             });
+
+            MenuItem sortValueChangesItem = abstractField.addItem(
+                    "Show sorted value changes", new MenuBar.Command() {
+                        public void menuSelected(MenuItem selectedItem) {
+                            sortValueChanges = selectedItem.isChecked();
+                            log("Show sorted value changes: "
+                                    + sortValueChanges);
+                        }
+                    });
+            sortValueChangesItem.setCheckable(true);
+            sortValueChangesItem.setChecked(true);
         }
     }
 
@@ -171,7 +184,7 @@ public abstract class AbstractFieldTest<T extends AbstractField> extends
     @SuppressWarnings({ "rawtypes", "unchecked" })
     private String getValue(Property property) {
         Object o = property.getValue();
-        if (o instanceof Collection) {
+        if (o instanceof Collection && sortValueChanges) {
             // Sort collections to avoid problems with values printed in
             // different order
             try {
diff --git a/tests/testbench/com/vaadin/tests/components/table/MultiSelectValueOrder.html b/tests/testbench/com/vaadin/tests/components/table/MultiSelectValueOrder.html
new file mode 100644
index 0000000000..4bac7741da
--- /dev/null
+++ b/tests/testbench/com/vaadin/tests/components/table/MultiSelectValueOrder.html
@@ -0,0 +1,275 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
+<head profile="http://selenium-ide.openqa.org/profiles/test-case">
+<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
+<link rel="selenium.base" href="http://localhost:8888/" />
+<title>New Test</title>
+</head>
+<body>
+<table cellpadding="1" cellspacing="1" border="1">
+<thead>
+<tr><td rowspan="1" colspan="3">New Test</td></tr>
+</thead><tbody>
+<tr>
+	<td>open</td>
+	<td>/run/com.vaadin.tests.components.table.Tables?restartApplication</td>
+	<td></td>
+</tr>
+<!--Default multi select-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item0</td>
+	<td>6,9</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item4</td>
+	<td>73,0</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[1]/VMenuBar[0]#item1</td>
+	<td>33,13</td>
+</tr>
+<!--ValueChangeListener-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item0</td>
+	<td>14,10</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item3</td>
+	<td>75,5</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[1]/VMenuBar[0]#item0</td>
+	<td>64,6</td>
+</tr>
+<!--Disable sorting value change logging-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item1</td>
+	<td>16,8</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item3</td>
+	<td>22,6</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[1]/VMenuBar[0]#item1</td>
+	<td>16,12</td>
+</tr>
+<!--Clear log to ensure numbers matches with the later case-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item1</td>
+	<td>29,6</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item1</td>
+	<td>40,7</td>
+</tr>
+<!--Start clicking-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[4]/domChild[0]/domChild[0]</td>
+	<td>48,18</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>0. ValueChangeEvent, new value: '[Item 5]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[2]/domChild[1]/domChild[0]</td>
+	<td>40,12:ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>1. ValueChangeEvent, new value: '[Item 5, Item 3]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[6]/domChild[1]/domChild[0]</td>
+	<td>54,15:ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>2. ValueChangeEvent, new value: '[Item 5, Item 3, Item 7]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[8]/domChild[1]/domChild[0]</td>
+	<td>54,9:shift+ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>3. ValueChangeEvent, new value: '[Item 5, Item 3, Item 7, Item 8, Item 9]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[0]/domChild[1]/domChild[0]</td>
+	<td>67,8:ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>4. ValueChangeEvent, new value: '[Item 5, Item 3, Item 7, Item 8, Item 9, Item 1]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[3]/domChild[2]/domChild[0]</td>
+	<td>71,8:shift+ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>5. ValueChangeEvent, new value: '[Item 5, Item 3, Item 7, Item 8, Item 9, Item 1, Item 2, Item 4]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[4]/domChild[2]/domChild[0]</td>
+	<td>56,6:ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>6. ValueChangeEvent, new value: '[Item 3, Item 7, Item 8, Item 9, Item 1, Item 2, Item 4]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[4]/domChild[2]/domChild[0]</td>
+	<td>67,11:ctrl</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>7. ValueChangeEvent, new value: '[Item 3, Item 7, Item 8, Item 9, Item 1, Item 2, Item 4, Item 5]'</td>
+</tr>
+<!--Simple multi select-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item0</td>
+	<td>33,9</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item4</td>
+	<td>44,7</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[1]/VMenuBar[0]#item2</td>
+	<td>40,11</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[2]/VMenuBar[0]#item2</td>
+	<td>58,8</td>
+</tr>
+<!--Clear value-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item0</td>
+	<td>38,6</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item4</td>
+	<td>14,8</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[1]/VMenuBar[0]#item3</td>
+	<td>55,3</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[2]/VMenuBar[0]#item0</td>
+	<td>21,3</td>
+</tr>
+<!--Clear log-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_Smenu#item1</td>
+	<td>26,7</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::Root/VOverlay[0]/VMenuBar[0]#item1</td>
+	<td>30,6</td>
+</tr>
+<!--Start clicking-->
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[4]/domChild[1]/domChild[0]</td>
+	<td>60,9</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>0. ValueChangeEvent, new value: '[Item 5]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[2]/domChild[1]/domChild[0]</td>
+	<td>38,9</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>1. ValueChangeEvent, new value: '[Item 5, Item 3]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[3]/domChild[1]/domChild[0]</td>
+	<td>46,13</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>2. ValueChangeEvent, new value: '[Item 5, Item 3, Item 4]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[2]/domChild[1]/domChild[0]</td>
+	<td>65,8</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>3. ValueChangeEvent, new value: '[Item 5, Item 4]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[4]/domChild[2]/domChild[0]</td>
+	<td>45,7</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>4. ValueChangeEvent, new value: '[Item 4]'</td>
+</tr>
+<tr>
+	<td>mouseClick</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_StestComponent/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[4]/domChild[2]/domChild[0]</td>
+	<td>35,9</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentstableTables::PID_SLog_row_0</td>
+	<td>5. ValueChangeEvent, new value: '[Item 4, Item 5]'</td>
+</tr>
+</tbody></table>
+</body>
+</html>