Enhanced tests and fix for #7668 - re-adding a component at an index before its curre...

Simplified the logic in AbstractOrderedLayout/CssLayout#addComponent*() - super.addComponent() is now called first so that the invariant that there's never duplicate elements in the component list always holds, this fixes #7668. addComponent(c, index) now has a piece of additional logic handling the case when a component is moved to an index *after* its current one.

svn changeset:22117/svn branch:6.7

diff --git a/src/com/vaadin/ui/AbstractOrderedLayout.java b/src/com/vaadin/ui/AbstractOrderedLayout.java
index 0b0fff2a3463649081b95f73d16ff2fd013cee80..fb232abe3cb3398eb6189ebabdbebd7fbdc4fe1d 100644 (file)
--- a/src/com/vaadin/ui/AbstractOrderedLayout.java
+++ b/src/com/vaadin/ui/AbstractOrderedLayout.java
@@ -53,14 +53,9 @@ public abstract class AbstractOrderedLayout extends AbstractLayout implements
      */
     @Override
     public void addComponent(Component c) {
+        super.addComponent(c);
         components.add(c);
-        try {
-            super.addComponent(c);
-            requestRepaint();
-        } catch (IllegalArgumentException e) {
-            components.remove(c);
-            throw e;
-        }
+        requestRepaint();
     }
 
     /**
@@ -71,14 +66,9 @@ public abstract class AbstractOrderedLayout extends AbstractLayout implements
      *            the component to be added.
      */
     public void addComponentAsFirst(Component c) {
+        super.addComponent(c);
         components.addFirst(c);
-        try {
-            super.addComponent(c);
-            requestRepaint();
-        } catch (IllegalArgumentException e) {
-            components.remove(c);
-            throw e;
-        }
+        requestRepaint();
     }
 
     /**
@@ -91,14 +81,14 @@ public abstract class AbstractOrderedLayout extends AbstractLayout implements
      *            in and after the position are shifted forwards.
      */
     public void addComponent(Component c, int index) {
-        components.add(index, c);
-        try {
-            super.addComponent(c);
-            requestRepaint();
-        } catch (IllegalArgumentException e) {
-            components.remove(c);
-            throw e;
+        // if this already contains c, super.addComponent() will remove it
+        // so everything after c's original position shifts one place down
+        if(this == c.getParent() && index > getComponentIndex(c)) {
+            index--;
         }
+        super.addComponent(c);
+        components.add(index, c);
+        requestRepaint();
     }
 
     /**

diff --git a/src/com/vaadin/ui/CssLayout.java b/src/com/vaadin/ui/CssLayout.java
index 5789a65ed3886a2fe044df114c9af505d70103e0..bdc4db80800bd2753724903785d17365157b0910 100644 (file)
--- a/src/com/vaadin/ui/CssLayout.java
+++ b/src/com/vaadin/ui/CssLayout.java
@@ -76,14 +76,9 @@ public class CssLayout extends AbstractLayout implements LayoutClickNotifier {
      */
     @Override
     public void addComponent(Component c) {
+        super.addComponent(c);
         components.add(c);
-        try {
-            super.addComponent(c);
-            requestRepaint();
-        } catch (IllegalArgumentException e) {
-            components.remove(c);
-            throw e;
-        }
+        requestRepaint();
     }
 
     /**
@@ -94,14 +89,9 @@ public class CssLayout extends AbstractLayout implements LayoutClickNotifier {
      *            the component to be added.
      */
     public void addComponentAsFirst(Component c) {
+        super.addComponent(c);
         components.addFirst(c);
-        try {
-            super.addComponent(c);
-            requestRepaint();
-        } catch (IllegalArgumentException e) {
-            components.remove(c);
-            throw e;
-        }
+        requestRepaint();
     }
 
     /**
@@ -114,14 +104,14 @@ public class CssLayout extends AbstractLayout implements LayoutClickNotifier {
      *            in and after the position are shifted forwards.
      */
     public void addComponent(Component c, int index) {
-        components.add(index, c);
-        try {
-            super.addComponent(c);
-            requestRepaint();
-        } catch (IllegalArgumentException e) {
-            components.remove(c);
-            throw e;
+        // if this already contains c, super.addComponent() will remove it
+        // so everything after c's original position shifts one place down
+        if(this == c.getParent() && index > components.indexOf(c)) {
+            index--;
         }
+        super.addComponent(c);
+        components.add(index, c);
+        requestRepaint();
     }
 
     /**

diff --git a/tests/server-side/com/vaadin/tests/server/component/abstractorderedlayout/AddComponentsTest.java b/tests/server-side/com/vaadin/tests/server/component/abstractorderedlayout/AddComponentsTest.java
index 3df4d5ff5d6495126c7235950ad336f53a2a769f..cfbace2a1a881c27f8bc724977385d6e6ea30bd0 100644 (file)
--- a/tests/server-side/com/vaadin/tests/server/component/abstractorderedlayout/AddComponentsTest.java
+++ b/tests/server-side/com/vaadin/tests/server/component/abstractorderedlayout/AddComponentsTest.java
@@ -40,6 +40,14 @@ public class AddComponentsTest {
         layout1.addComponent(children[3], 0);\r
         assertOrder(layout1, new int[] { 3, 0 });\r
         assertOrder(layout2, new int[] { 2, 1 });\r
+        \r
+        layout2.addComponent(children[0]);\r
+        assertOrder(layout1, new int[] { 3 });\r
+        assertOrder(layout2, new int[] { 2, 1, 0 });\r
+        \r
+        layout1.addComponentAsFirst(children[1]);\r
+        assertOrder(layout1, new int[] { 1, 3 });\r
+        assertOrder(layout2, new int[] { 2, 0 });\r
     }\r
     \r
     @Test\r
@@ -72,6 +80,22 @@ public class AddComponentsTest {
         // Move D from #2 to #0\r
         layout.addComponent(children[3], 0);\r
         assertOrder(layout, new int[] { 3, 0, 1, 2 });\r
+        \r
+        // Move A from #1 to end (#4 which becomes #3)\r
+        layout.addComponent(children[0]);\r
+        assertOrder(layout, new int[] { 3, 1, 2, 0 });\r
+\r
+        // Keep everything in place\r
+        layout.addComponent(children[0]);\r
+        assertOrder(layout, new int[] { 3, 1, 2, 0 });\r
+        \r
+        // Move C from #2 to #0\r
+        layout.addComponentAsFirst(children[2]);\r
+        assertOrder(layout, new int[] { 2, 3, 1, 0 });\r
+\r
+        // Keep everything in place\r
+        layout.addComponentAsFirst(children[2]);\r
+        assertOrder(layout, new int[] { 2, 3, 1, 0 });\r
     }\r
 \r
     /**\r

diff --git a/tests/server-side/com/vaadin/tests/server/component/csslayout/AddComponentsTest.java b/tests/server-side/com/vaadin/tests/server/component/csslayout/AddComponentsTest.java
index 6e69dfe915912f8d770e65a97c42d7c194f667a3..73c977694b2584011c26b926942b8a74530b3ec8 100644 (file)
--- a/tests/server-side/com/vaadin/tests/server/component/csslayout/AddComponentsTest.java
+++ b/tests/server-side/com/vaadin/tests/server/component/csslayout/AddComponentsTest.java
@@ -39,6 +39,14 @@ public class AddComponentsTest {
         layout1.addComponent(children[3], 0);\r
         assertOrder(layout1, new int[] { 3, 0 });\r
         assertOrder(layout2, new int[] { 2, 1 });\r
+        \r
+        layout2.addComponent(children[0]);\r
+        assertOrder(layout1, new int[] { 3 });\r
+        assertOrder(layout2, new int[] { 2, 1, 0 });\r
+        \r
+        layout1.addComponentAsFirst(children[1]);\r
+        assertOrder(layout1, new int[] { 1, 3 });\r
+        assertOrder(layout2, new int[] { 2, 0 });\r
     }\r
     \r
     @Test\r
@@ -67,6 +75,22 @@ public class AddComponentsTest {
         // Move D from #2 to #0\r
         layout.addComponent(children[3], 0);\r
         assertOrder(layout, new int[] { 3, 0, 1, 2 });\r
+        \r
+        // Move A from #1 to end (#4 which becomes #3)\r
+        layout.addComponent(children[0]);\r
+        assertOrder(layout, new int[] { 3, 1, 2, 0 });\r
+\r
+        // Keep everything in place\r
+        layout.addComponent(children[0]);\r
+        assertOrder(layout, new int[] { 3, 1, 2, 0 });\r
+        \r
+        // Move C from #2 to #0\r
+        layout.addComponentAsFirst(children[2]);\r
+        assertOrder(layout, new int[] { 2, 3, 1, 0 });\r
+\r
+        // Keep everything in place\r
+        layout.addComponentAsFirst(children[2]);\r
+        assertOrder(layout, new int[] { 2, 3, 1, 0 });\r
     }\r
 \r
     /**\r