Merge of (#6476) to Vaadin 7.

Table.setVisibleColumns should throw exception from duplicate property
ids.

Change-Id: I1465a99bd0c6241c3b31d88f5480fee99bbdc0ad

2 files changed, 11 insertions, 17 deletions

diff --git a/server/src/com/vaadin/ui/Table.java b/server/src/com/vaadin/ui/Table.java
index 65dea7594b..b035b6da8c 100644
--- a/server/src/com/vaadin/ui/Table.java
+++ b/server/src/com/vaadin/ui/Table.java
@@ -625,10 +625,10 @@ public class Table extends AbstractSelect implements Action.Container,
                     "Can not set visible columns to null value");
         }
 
-        // TODO add error check that no duplicate identifiers exist
+        final LinkedList<Object> newVC = new LinkedList<Object>();
 
-        // Checks that the new visible columns contains no nulls and properties
-        // exist
+        // Checks that the new visible columns contains no nulls, properties
+        // exist and that there are no duplicates before adding them to newVC.
         final Collection<?> properties = getContainerPropertyIds();
         for (int i = 0; i < visibleColumns.length; i++) {
             if (visibleColumns[i] == null) {
@@ -636,18 +636,17 @@ public class Table extends AbstractSelect implements Action.Container,
             } else if (!properties.contains(visibleColumns[i])
                     && !columnGenerators.containsKey(visibleColumns[i])) {
                 throw new IllegalArgumentException(
-                        "Ids must exist in the Container or as a generated column , missing id: "
+                        "Ids must exist in the Container or as a generated column, missing id: "
+                                + visibleColumns[i]);
+            } else if (newVC.contains(visibleColumns[i])) {
+                throw new IllegalArgumentException(
+                        "Ids must be unique, duplicate id: "
                                 + visibleColumns[i]);
+            } else {
+                newVC.add(visibleColumns[i]);
             }
         }
 
-        // If this is called before the constructor is finished, it might be
-        // uninitialized
-        final LinkedList<Object> newVC = new LinkedList<Object>();
-        for (int i = 0; i < visibleColumns.length; i++) {
-            newVC.add(visibleColumns[i]);
-        }
-
         // Removes alignments, icons and headers from hidden columns
         if (this.visibleColumns != null) {
             boolean disabledHere = disableContentRefreshing();
diff --git a/server/tests/src/com/vaadin/tests/server/component/table/TableVisibleColumns.java b/server/tests/src/com/vaadin/tests/server/component/table/TableVisibleColumns.java
index be312044db..ee3eefba05 100644
--- a/server/tests/src/com/vaadin/tests/server/component/table/TableVisibleColumns.java
+++ b/server/tests/src/com/vaadin/tests/server/component/table/TableVisibleColumns.java
@@ -54,15 +54,10 @@ public class TableVisibleColumns {
         try {
             t.setVisibleColumns(new Object[] { "Property 0", "Property 1",
                     "Property 2", "Property 1" });
-            // FIXME: Multiple properties in the Object array should be detected
-            // (#6476)
-            // junit.framework.Assert.fail("IllegalArgumentException expected");
         } catch (IllegalArgumentException e) {
             // OK, expected
         }
-        // FIXME: Multiple properties in the Object array should be detected
-        // (#6476)
-        // assertArrayEquals(defaultColumns3, t.getVisibleColumns());
+        assertArrayEquals(defaultColumns3, t.getVisibleColumns());
     }
 
     @Test