From c4a0bc318ec9c6b41d652e7020ee39784cc1eb10 Mon Sep 17 00:00:00 2001 From: Johannes Dahlström Date: Mon, 11 Feb 2013 17:04:15 +0200 Subject: Properly pass the AbstractField internal value to shared state in Slider and ProgressIndicator (#10921) * In case of a null value, 0 is stored to shared state. This is consistent with how CheckBox is already implemented. * This somewhat hacky solution should be replaced with a better one once AbstractField itself is migrated. See #11064. Change-Id: I2b313af8491a6deccdc7a509dcd1b718482cdcd4 --- server/src/com/vaadin/ui/CheckBox.java | 7 +++++++ server/src/com/vaadin/ui/ProgressIndicator.java | 15 +++++++++++++++ server/src/com/vaadin/ui/Slider.java | 16 ++++++++++++++++ 3 files changed, 38 insertions(+) (limited to 'server') diff --git a/server/src/com/vaadin/ui/CheckBox.java b/server/src/com/vaadin/ui/CheckBox.java index 22b90b224f..0ace0a4f26 100644 --- a/server/src/com/vaadin/ui/CheckBox.java +++ b/server/src/com/vaadin/ui/CheckBox.java @@ -110,6 +110,13 @@ public class CheckBox extends AbstractField { return (CheckBoxState) super.getState(); } + /* + * Overridden to keep the shared state in sync with the AbstractField + * internal value. Should be removed once AbstractField is refactored to use + * shared state. + * + * See tickets #10921 and #11064. + */ @Override protected void setInternalValue(Boolean newValue) { super.setInternalValue(newValue); diff --git a/server/src/com/vaadin/ui/ProgressIndicator.java b/server/src/com/vaadin/ui/ProgressIndicator.java index 96c2d2814a..c481aa1e8f 100644 --- a/server/src/com/vaadin/ui/ProgressIndicator.java +++ b/server/src/com/vaadin/ui/ProgressIndicator.java @@ -157,4 +157,19 @@ public class ProgressIndicator extends AbstractField implements return getState().pollingInterval; } + /* + * Overridden to keep the shared state in sync with the AbstractField + * internal value. Should be removed once AbstractField is refactored to use + * shared state. + * + * See tickets #10921 and #11064. + */ + @Override + protected void setInternalValue(Float newValue) { + super.setInternalValue(newValue); + if (newValue == null) { + newValue = 0.0f; + } + getState().state = newValue; + } } diff --git a/server/src/com/vaadin/ui/Slider.java b/server/src/com/vaadin/ui/Slider.java index 2bf05f895c..e63fdc5e10 100644 --- a/server/src/com/vaadin/ui/Slider.java +++ b/server/src/com/vaadin/ui/Slider.java @@ -263,6 +263,22 @@ public class Slider extends AbstractField { getState().value = newFieldValue; } + /* + * Overridden to keep the shared state in sync with the AbstractField + * internal value. Should be removed once AbstractField is refactored to use + * shared state. + * + * See tickets #10921 and #11064. + */ + @Override + protected void setInternalValue(Double newValue) { + super.setInternalValue(newValue); + if (newValue == null) { + newValue = 0.0; + } + getState().value = newValue; + } + /** * Thrown when the value of the slider is about to be set to a value that is * outside the valid range of the slider. -- cgit v1.2.3 From 834275130dce0fee314e2c5b0b092579ba3905a4 Mon Sep 17 00:00:00 2001 From: John Ahlroos Date: Thu, 21 Feb 2013 15:55:57 +0200 Subject: Fixed memory leak in Tree #11053 Change-Id: I4c58367d4041b5e61989fca5be6b037f7f8e0c01 --- server/src/com/vaadin/ui/Tree.java | 17 +++++++++ .../tests/server/component/tree/TreeTest.java | 41 ++++++++++++++++++++++ 2 files changed, 58 insertions(+) (limited to 'server') diff --git a/server/src/com/vaadin/ui/Tree.java b/server/src/com/vaadin/ui/Tree.java index 608c947d59..219b39020b 100644 --- a/server/src/com/vaadin/ui/Tree.java +++ b/server/src/com/vaadin/ui/Tree.java @@ -1670,4 +1670,21 @@ public class Tree extends AbstractSelect implements Container.Hierarchical, return itemDescriptionGenerator; } + @Override + public void containerItemSetChange( + com.vaadin.data.Container.ItemSetChangeEvent event) { + + // Ensure removed items are cleaned up from expanded list + for (Object expandedItemId : expanded) { + if (getItem(expandedItemId) == null) { + expanded.remove(expandedItemId); + if (this.expandedItemId == expandedItemId) { + this.expandedItemId = null; + } + } + } + + super.containerItemSetChange(event); + } + } diff --git a/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java b/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java index 16a7091947..f467a42339 100644 --- a/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java +++ b/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java @@ -1,5 +1,8 @@ package com.vaadin.tests.server.component.tree; +import java.lang.reflect.Field; +import java.util.HashSet; + import junit.framework.TestCase; import com.vaadin.data.Container; @@ -71,4 +74,42 @@ public class TreeTest extends TestCase { assertTrue(Container.Hierarchical.class.isAssignableFrom(tree4 .getContainerDataSource().getClass())); } + + public void testRemoveExpandedItems() throws Exception { + tree.expandItem("parent"); + tree.expandItem("child"); + + Field expandedField = tree.getClass() + .getDeclaredField("expanded"); + Field expandedItemIdField = tree.getClass().getDeclaredField( + "expandedItemId"); + + expandedField.setAccessible(true); + expandedItemIdField.setAccessible(true); + + HashSet expanded = (HashSet) expandedField.get(tree); + Object expandedItemId = expandedItemIdField.get(tree); + + assertEquals(2, expanded.size()); + assertTrue("Contains parent", expanded.contains("parent")); + assertTrue("Contains child", expanded.contains("child")); + assertEquals("child", expandedItemId); + + tree.removeItem("parent"); + + expanded = (HashSet) expandedField.get(tree); + expandedItemId = expandedItemIdField.get(tree); + + assertEquals(1, expanded.size()); + assertTrue("Contains child", expanded.contains("child")); + assertEquals("child", expandedItemId); + + tree.removeItem("child"); + + expanded = (HashSet) expandedField.get(tree); + expandedItemId = expandedItemIdField.get(tree); + + assertEquals(0, expanded.size()); + assertNull(expandedItemId); + } } -- cgit v1.2.3 From 7af5b3fceb75b6f505a9a6d0a843b788bb06d9a7 Mon Sep 17 00:00:00 2001 From: John Ahlroos Date: Thu, 21 Feb 2013 16:46:07 +0200 Subject: Fixed another memory leak in Tree #11053 Change-Id: Idf45309b83dfe718fe6e58b6cf57b211eeb9e811 --- server/src/com/vaadin/ui/Tree.java | 16 +++++++++++++--- .../tests/server/component/tree/TreeTest.java | 21 +++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) (limited to 'server') diff --git a/server/src/com/vaadin/ui/Tree.java b/server/src/com/vaadin/ui/Tree.java index 219b39020b..32c5712f0f 100644 --- a/server/src/com/vaadin/ui/Tree.java +++ b/server/src/com/vaadin/ui/Tree.java @@ -414,6 +414,9 @@ public class Tree extends AbstractSelect implements Container.Hierarchical, final Object id = itemIdMapper.get(keys[i]); if (id != null && isExpanded(id)) { expanded.remove(id); + if (expandedItemId == id) { + expandedItemId = null; + } fireCollapseEvent(id); } } @@ -841,6 +844,10 @@ public class Tree extends AbstractSelect implements Container.Hierarchical, super.setContainerDataSource(new ContainerHierarchicalWrapper( newDataSource)); } + + // Ensure previous expanded items are cleaned up if they don't exist in + // the new container + cleanupExpandedItems(); } /* Expand event and listener */ @@ -1675,6 +1682,12 @@ public class Tree extends AbstractSelect implements Container.Hierarchical, com.vaadin.data.Container.ItemSetChangeEvent event) { // Ensure removed items are cleaned up from expanded list + cleanupExpandedItems(); + + super.containerItemSetChange(event); + } + + private void cleanupExpandedItems() { for (Object expandedItemId : expanded) { if (getItem(expandedItemId) == null) { expanded.remove(expandedItemId); @@ -1683,8 +1696,5 @@ public class Tree extends AbstractSelect implements Container.Hierarchical, } } } - - super.containerItemSetChange(event); } - } diff --git a/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java b/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java index f467a42339..c1d7653c01 100644 --- a/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java +++ b/server/tests/src/com/vaadin/tests/server/component/tree/TreeTest.java @@ -112,4 +112,25 @@ public class TreeTest extends TestCase { assertEquals(0, expanded.size()); assertNull(expandedItemId); } + + public void testRemoveExpandedItemsOnContainerChange() throws Exception { + tree.expandItem("parent"); + tree.expandItem("child"); + + tree.setContainerDataSource(new HierarchicalContainer()); + + Field expandedField = tree.getClass().getDeclaredField("expanded"); + Field expandedItemIdField = tree.getClass().getDeclaredField( + "expandedItemId"); + + expandedField.setAccessible(true); + expandedItemIdField.setAccessible(true); + + HashSet expanded = (HashSet) expandedField.get(tree); + assertEquals(0, expanded.size()); + + Object expandedItemId = expandedItemIdField.get(tree); + assertNull(expandedItemId); + } + } -- cgit v1.2.3