From: Johannes Dahlström Date: Thu, 24 Nov 2011 10:44:54 +0000 (+0000) Subject: Enhanced tests and fix for #7668 - re-adding a component at an index before its curre... X-Git-Tag: 7.0.0.alpha1~225^2~3^2~15 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=1c6a751a5a1315b74b3e5adac337058e803e9d1f;p=vaadin-framework.git Enhanced tests and fix for #7668 - re-adding a component at an index before its current one does nothing 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 0b0fff2a34..fb232abe3c 100644 --- 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 5789a65ed3..bdc4db8080 100644 --- 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 3df4d5ff5d..cfbace2a1a 100644 --- 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); assertOrder(layout1, new int[] { 3, 0 }); assertOrder(layout2, new int[] { 2, 1 }); + + layout2.addComponent(children[0]); + assertOrder(layout1, new int[] { 3 }); + assertOrder(layout2, new int[] { 2, 1, 0 }); + + layout1.addComponentAsFirst(children[1]); + assertOrder(layout1, new int[] { 1, 3 }); + assertOrder(layout2, new int[] { 2, 0 }); } @Test @@ -72,6 +80,22 @@ public class AddComponentsTest { // Move D from #2 to #0 layout.addComponent(children[3], 0); assertOrder(layout, new int[] { 3, 0, 1, 2 }); + + // Move A from #1 to end (#4 which becomes #3) + layout.addComponent(children[0]); + assertOrder(layout, new int[] { 3, 1, 2, 0 }); + + // Keep everything in place + layout.addComponent(children[0]); + assertOrder(layout, new int[] { 3, 1, 2, 0 }); + + // Move C from #2 to #0 + layout.addComponentAsFirst(children[2]); + assertOrder(layout, new int[] { 2, 3, 1, 0 }); + + // Keep everything in place + layout.addComponentAsFirst(children[2]); + assertOrder(layout, new int[] { 2, 3, 1, 0 }); } /** 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 6e69dfe915..73c977694b 100644 --- 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); assertOrder(layout1, new int[] { 3, 0 }); assertOrder(layout2, new int[] { 2, 1 }); + + layout2.addComponent(children[0]); + assertOrder(layout1, new int[] { 3 }); + assertOrder(layout2, new int[] { 2, 1, 0 }); + + layout1.addComponentAsFirst(children[1]); + assertOrder(layout1, new int[] { 1, 3 }); + assertOrder(layout2, new int[] { 2, 0 }); } @Test @@ -67,6 +75,22 @@ public class AddComponentsTest { // Move D from #2 to #0 layout.addComponent(children[3], 0); assertOrder(layout, new int[] { 3, 0, 1, 2 }); + + // Move A from #1 to end (#4 which becomes #3) + layout.addComponent(children[0]); + assertOrder(layout, new int[] { 3, 1, 2, 0 }); + + // Keep everything in place + layout.addComponent(children[0]); + assertOrder(layout, new int[] { 3, 1, 2, 0 }); + + // Move C from #2 to #0 + layout.addComponentAsFirst(children[2]); + assertOrder(layout, new int[] { 2, 3, 1, 0 }); + + // Keep everything in place + layout.addComponentAsFirst(children[2]); + assertOrder(layout, new int[] { 2, 3, 1, 0 }); } /**