#1061: merge to 6.0 of BeanItemContainer filtering fixes and other improvements, interactive test class

svn changeset:7269/svn branch:6.0

5 files changed, 370 insertions, 56 deletions

diff --git a/src/com/itmill/toolkit/data/Container.java b/src/com/itmill/toolkit/data/Container.java
index 36b0caca93..5df1b34f91 100644
--- a/src/com/itmill/toolkit/data/Container.java
+++ b/src/com/itmill/toolkit/data/Container.java
@@ -543,8 +543,8 @@ public interface Container {
     }
 
     /**
-     * Interface is implemented by containers that allow reducing their
-     * visiblecontents with set of filters.
+     * Interface is implemented by containers that allow reducing their visible
+     * contents with set of filters.
      * 
      * When a set of filters are set, only items that match the filters are
      * included in the visible contents of the container. Still new items that
@@ -553,6 +553,19 @@ public interface Container {
      * multiple filters are added, all filters must match for an item to be
      * visible in the container.
      * 
+     * When an {@link com.itmill.toolkit.data.Ordered} or
+     * {@link com.itmill.toolkit.data.Indexed} container is filtered, all
+     * operations of these interfaces should only use the filtered contents and
+     * the filtered indices to the container.
+     * 
+     * Adding items (if supported) to a filtered
+     * {@link com.itmill.toolkit.data.Ordered} or
+     * {@link com.itmill.toolkit.data.Indexed} container should insert them
+     * immediately after the indicated visible item. The unfiltered position of
+     * items added at index 0, at index
+     * {@link com.itmill.toolkit.data.Container#size()} or at an undefined
+     * position is up to the implementation.
+     * 
      * @since 5.0
      */
     public interface Filterable extends Container {
diff --git a/src/com/itmill/toolkit/data/util/BeanItem.java b/src/com/itmill/toolkit/data/util/BeanItem.java
index f30d5fd820..b867491cdb 100644
--- a/src/com/itmill/toolkit/data/util/BeanItem.java
+++ b/src/com/itmill/toolkit/data/util/BeanItem.java
@@ -60,7 +60,7 @@ public class BeanItem extends PropertysetItem {
             for (int i = 0; i < pd.length; i++) {
                 final Method getMethod = pd[i].getReadMethod();
                 final Method setMethod = pd[i].getWriteMethod();
-                final Class type = pd[i].getPropertyType();
+                final Class<?> type = pd[i].getPropertyType();
                 final String name = pd[i].getName();
 
                 if ((getMethod != null)
@@ -103,14 +103,15 @@ public class BeanItem extends PropertysetItem {
             final PropertyDescriptor[] pd = info.getPropertyDescriptors();
 
             // Add all the bean properties as MethodProperties to this Item
-            for (final Iterator iter = propertyIds.iterator(); iter.hasNext();) {
+            final Iterator iter = propertyIds.iterator();
+            while (iter.hasNext()) {
                 final Object id = iter.next();
                 for (int i = 0; i < pd.length; i++) {
                     final String name = pd[i].getName();
                     if (name.equals(id)) {
                         final Method getMethod = pd[i].getReadMethod();
                         final Method setMethod = pd[i].getWriteMethod();
-                        final Class type = pd[i].getPropertyType();
+                        final Class<?> type = pd[i].getPropertyType();
                         if ((getMethod != null)) {
                             final Property p = new MethodProperty(type, bean,
                                     getMethod, setMethod);
diff --git a/src/com/itmill/toolkit/data/util/BeanItemContainer.java b/src/com/itmill/toolkit/data/util/BeanItemContainer.java
index 24b16573fc..7fa747372d 100644
--- a/src/com/itmill/toolkit/data/util/BeanItemContainer.java
+++ b/src/com/itmill/toolkit/data/util/BeanItemContainer.java
@@ -8,6 +8,9 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 import com.itmill.toolkit.data.Container;
 import com.itmill.toolkit.data.Item;
@@ -16,23 +19,33 @@ import com.itmill.toolkit.data.Container.Filterable;
 import com.itmill.toolkit.data.Container.Indexed;
 import com.itmill.toolkit.data.Container.ItemSetChangeNotifier;
 import com.itmill.toolkit.data.Container.Sortable;
+import com.itmill.toolkit.data.Property.ValueChangeEvent;
+import com.itmill.toolkit.data.Property.ValueChangeListener;
+import com.itmill.toolkit.data.Property.ValueChangeNotifier;
 
 /**
  * An {@link ArrayList} backed container for {@link BeanItem}s.
  * <p>
- * Bean objects act as identifiers.
+ * Bean objects act as identifiers. For this reason, they should implement
+ * Object.equals(Object) and Object.hashCode() .
  * 
  * @param <BT>
+ * 
+ * @since 5.4
  */
 public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
-        ItemSetChangeNotifier {
+        ItemSetChangeNotifier, ValueChangeListener {
+    // filtered and unfiltered item IDs
     private ArrayList<BT> list = new ArrayList<BT>();
-    private final HashMap<BT, BeanItem> beanToItem = new HashMap<BT, BeanItem>();
+    private ArrayList<BT> allItems = new ArrayList<BT>();
+    private final Map<BT, BeanItem> beanToItem = new HashMap<BT, BeanItem>();
+
     private final Class<BT> type;
     private final BeanItem model;
-    private LinkedList<ItemSetChangeListener> itemSetChangeListeners;
-    private ArrayList<BT> allItems;
-    private HashSet<Filter> filters;
+
+    private List<ItemSetChangeListener> itemSetChangeListeners;
+
+    private Set<Filter> filters = new HashSet<Filter>();
 
     public BeanItemContainer(Class<BT> type) throws InstantiationException,
             IllegalAccessException {
@@ -65,15 +78,52 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
         throw new UnsupportedOperationException();
     }
 
-    @SuppressWarnings("unchecked")
     public Item addItemAt(int index, Object newItemId)
             throws UnsupportedOperationException {
+        if (index < 0 || index > size()) {
+            return null;
+        } else if (index == 0) {
+            // add before any item, visible or not
+            return addItemAtInternalIndex(0, newItemId);
+        } else {
+            // if index==size(), adds immediately after last visible item
+            return addItemAfter(getIdByIndex(index - 1), newItemId);
+        }
+    }
+
+    /**
+     * Adds new item at given index of the internal (unfiltered) list.
+     * <p>
+     * The item is also added in the visible part of the list if it passes the
+     * filters.
+     * </p>
+     * 
+     * @param index
+     *            Internal index to add the new item.
+     * @param newItemId
+     *            Id of the new item to be added.
+     * @return Returns new item or null if the operation fails.
+     */
+    @SuppressWarnings("unchecked")
+    private Item addItemAtInternalIndex(int index, Object newItemId) {
+        // Make sure that the Item has not been created yet
+        if (allItems.contains(newItemId)) {
+            return null;
+        }
         if (newItemId.getClass().isAssignableFrom(type)) {
             BT pojo = (BT) newItemId;
-            list.add(index, pojo);
+            // "list" will be updated in filterAll()
+            allItems.add(index, pojo);
             BeanItem beanItem = new BeanItem(pojo);
             beanToItem.put(pojo, beanItem);
-            fireItemSetChange();
+            // add listeners to be able to update filtering on property changes
+            for (Filter filter : filters) {
+                // addValueChangeListener avoids adding duplicates
+                addValueChangeListener(beanItem, filter.propertyId);
+            }
+
+            // it is somewhat suboptimal to filter all items
+            filterAll();
             return beanItem;
         } else {
             return null;
@@ -95,15 +145,21 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
 
     public Item addItemAfter(Object previousItemId, Object newItemId)
             throws UnsupportedOperationException {
-        int index = indexOfId(previousItemId) + 1;
-        if (index > 0) {
-            addItemAt(index, newItemId);
+        // only add if the previous item is visible
+        if (list.contains(previousItemId)) {
+            return addItemAtInternalIndex(allItems.indexOf(previousItemId) + 1,
+                    newItemId);
+        } else {
+            return null;
         }
-        return null;
     }
 
     public Object firstItemId() {
-        return list.iterator().next();
+        if (list.size() > 0) {
+            return list.get(0);
+        } else {
+            return null;
+        }
     }
 
     public boolean isFirstId(Object itemId) {
@@ -115,26 +171,28 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
     }
 
     public Object lastItemId() {
-        try {
+        if (list.size() > 0) {
             return list.get(list.size() - 1);
-        } catch (ArrayIndexOutOfBoundsException e) {
+        } else {
             return null;
         }
     }
 
     public Object nextItemId(Object itemId) {
-        try {
-            return list.get(list.indexOf(itemId) + 1);
-        } catch (Exception e) {
+        int index = list.indexOf(itemId);
+        if (index >= 0 && index < list.size() - 1) {
+            return list.get(index + 1);
+        } else {
             // out of bounds
             return null;
         }
     }
 
     public Object prevItemId(Object itemId) {
-        try {
-            return list.get(list.indexOf(itemId) - 1);
-        } catch (Exception e) {
+        int index = list.indexOf(itemId);
+        if (index > 0) {
+            return list.get(index - 1);
+        } else {
             // out of bounds
             return null;
         }
@@ -151,14 +209,17 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
     }
 
     public Item addItem(Object itemId) throws UnsupportedOperationException {
-        if (list.isEmpty()) {
-            return addItemAt(0, itemId);
+        if (list.size() > 0) {
+            // add immediately after last visible item
+            int lastIndex = allItems.indexOf(lastItemId());
+            return addItemAtInternalIndex(lastIndex + 1, itemId);
         } else {
-            return addItemAt(indexOfId(lastItemId()) + 1, itemId);
+            return addItemAtInternalIndex(0, itemId);
         }
     }
 
     public boolean containsId(Object itemId) {
+        // only look at visible items after filtering
         return list.contains(itemId);
     }
 
@@ -185,7 +246,12 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
     }
 
     public boolean removeAllItems() throws UnsupportedOperationException {
+        allItems.clear();
         list.clear();
+        // detach listeners from all BeanItems
+        for (BeanItem item : beanToItem.values()) {
+            removeAllValueChangeListeners(item);
+        }
         beanToItem.clear();
         fireItemSetChange();
         return true;
@@ -198,14 +264,40 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
 
     public boolean removeItem(Object itemId)
             throws UnsupportedOperationException {
-        if (list.contains(itemId)) {
-            beanToItem.remove(itemId);
-            list.remove(itemId);
-            fireItemSetChange();
-            return true;
-        } else {
+        if (!allItems.remove(itemId)) {
             return false;
         }
+        // detach listeners from Item
+        removeAllValueChangeListeners(beanToItem.get(itemId));
+        // remove item
+        beanToItem.remove(itemId);
+        list.remove(itemId);
+        fireItemSetChange();
+        return true;
+    }
+
+    private void addValueChangeListener(BeanItem beanItem, Object propertyId) {
+        Property property = beanItem.getItemProperty(propertyId);
+        if (property instanceof ValueChangeNotifier) {
+            // avoid multiple notifications for the same property if
+            // multiple filters are in use
+            ValueChangeNotifier notifier = (ValueChangeNotifier) property;
+            notifier.removeListener(this);
+            notifier.addListener(this);
+        }
+    }
+
+    private void removeValueChangeListener(BeanItem item, Object propertyId) {
+        Property property = item.getItemProperty(propertyId);
+        if (property instanceof ValueChangeNotifier) {
+            ((ValueChangeNotifier) property).removeListener(this);
+        }
+    }
+
+    private void removeAllValueChangeListeners(BeanItem item) {
+        for (Object propertyId : item.getItemPropertyIds()) {
+            removeValueChangeListener(item, propertyId);
+        }
     }
 
     public int size() {
@@ -227,7 +319,8 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
         for (int i = 0; i < ascending.length; i++) {
             final boolean asc = ascending[i];
             final Object property = propertyId[i];
-            Collections.sort(list, new Comparator<BT>() {
+            // sort allItems, then filter and notify
+            Collections.sort(allItems, new Comparator<BT>() {
                 @SuppressWarnings("unchecked")
                 public int compare(BT a, BT b) {
                     Comparable va = (Comparable) beanToItem.get(a)
@@ -239,6 +332,8 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
                 }
             });
         }
+        // notifies if anything changes in the filtered list, including order
+        filterAll();
     }
 
     public void addListener(ItemSetChangeListener listener) {
@@ -256,14 +351,13 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
 
     private void fireItemSetChange() {
         if (itemSetChangeListeners != null) {
-            final Object[] l = itemSetChangeListeners.toArray();
             final Container.ItemSetChangeEvent event = new Container.ItemSetChangeEvent() {
                 public Container getContainer() {
                     return BeanItemContainer.this;
                 }
             };
-            for (int i = 0; i < l.length; i++) {
-                ((ItemSetChangeListener) l[i]).containerItemSetChange(event);
+            for (ItemSetChangeListener listener : itemSetChangeListeners) {
+                listener.containerItemSetChange(event);
             }
         }
     }
@@ -289,10 +383,12 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
     @SuppressWarnings("unchecked")
     public void addContainerFilter(Object propertyId, String filterString,
             boolean ignoreCase, boolean onlyMatchPrefix) {
-        if (filters == null) {
-            allItems = list;
-            list = (ArrayList<BT>) list.clone();
-            filters = new HashSet<Filter>();
+        if (filters.isEmpty()) {
+            list = (ArrayList<BT>) allItems.clone();
+        }
+        // listen to change events to be able to update filtering
+        for (BeanItem item : beanToItem.values()) {
+            addValueChangeListener(item, propertyId);
         }
         Filter f = new Filter(propertyId, filterString, ignoreCase,
                 onlyMatchPrefix);
@@ -301,9 +397,34 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
         fireItemSetChange();
     }
 
+    /**
+     * Filter the view to recreate the visible item list from the unfiltered
+     * items, and send a notification if the set of visible items changed in any
+     * way.
+     */
+    @SuppressWarnings("unchecked")
+    protected void filterAll() {
+        // avoid notification if the filtering had no effect
+        List<BT> originalItems = list;
+        // it is somewhat inefficient to do a (shallow) clone() every time
+        list = (ArrayList<BT>) allItems.clone();
+        for (Filter f : filters) {
+            filter(f);
+        }
+        // check if exactly the same items are there after filtering to avoid
+        // unnecessary notifications
+        // this may be slow in some cases as it uses BT.equals()
+        if (!originalItems.equals(list)) {
+            fireItemSetChange();
+        }
+    }
+
     protected void filter(Filter f) {
-        for (Iterator<BT> iterator = list.iterator(); iterator.hasNext();) {
+        Iterator<BT> iterator = list.iterator();
+        while (iterator.hasNext()) {
             BT bean = iterator.next();
+            // TODO #2517: should not swallow exceptions - requires several
+            // checks
             try {
                 String value = getContainerProperty(bean, f.propertyId)
                         .getValue().toString();
@@ -326,16 +447,18 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
     }
 
     public void removeAllContainerFilters() {
-        if (filters != null) {
-            filters = null;
-            list = allItems;
-            fireItemSetChange();
+        if (!filters.isEmpty()) {
+            filters = new HashSet<Filter>();
+            // stop listening to change events for any property
+            for (BeanItem item : beanToItem.values()) {
+                removeAllValueChangeListeners(item);
+            }
+            filterAll();
         }
     }
 
-    @SuppressWarnings("unchecked")
     public void removeContainerFilters(Object propertyId) {
-        if (filters != null) {
+        if (!filters.isEmpty()) {
             for (Iterator<Filter> iterator = filters.iterator(); iterator
                     .hasNext();) {
                 Filter f = iterator.next();
@@ -343,12 +466,17 @@ public class BeanItemContainer<BT> implements Indexed, Sortable, Filterable,
                     iterator.remove();
                 }
             }
-            list = (ArrayList<BT>) list.clone();
-            for (Filter f : filters) {
-                filter(f);
+            // stop listening to change events for the property
+            for (BeanItem item : beanToItem.values()) {
+                removeValueChangeListener(item, propertyId);
             }
-            fireItemSetChange();
+            filterAll();
         }
     }
 
+    public void valueChange(ValueChangeEvent event) {
+        // if a property that is used in a filter is changed, refresh filtering
+        filterAll();
+    }
+
 }
diff --git a/src/com/itmill/toolkit/tests/containers/BeanItemContainerFilteringTest.java b/src/com/itmill/toolkit/tests/containers/BeanItemContainerFilteringTest.java
new file mode 100644
index 0000000000..be3f8b1861
--- /dev/null
+++ b/src/com/itmill/toolkit/tests/containers/BeanItemContainerFilteringTest.java
@@ -0,0 +1,172 @@
+package com.itmill.toolkit.tests.containers;

+

+import com.itmill.toolkit.data.Item;

+import com.itmill.toolkit.data.util.BeanItemContainer;

+import com.itmill.toolkit.terminal.Sizeable;

+import com.itmill.toolkit.tests.components.TestBase;

+import com.itmill.toolkit.ui.Button;

+import com.itmill.toolkit.ui.CheckBox;

+import com.itmill.toolkit.ui.Label;

+import com.itmill.toolkit.ui.Table;

+import com.itmill.toolkit.ui.TextField;

+import com.itmill.toolkit.ui.VerticalLayout;

+import com.itmill.toolkit.ui.Button.ClickEvent;

+

+public class BeanItemContainerFilteringTest extends TestBase {

+

+    private Table table;

+    private BeanItemContainer<TestBean> container;

+    private TextField filterString;

+    private TextField position;

+    private int nextToAdd = 1;

+    private Label nextLabel;

+

+    protected static class TestBean {

+        private String id;

+        private String value;

+

+        public TestBean() {

+        }

+

+        public TestBean(String id, String value) {

+            setId(id);

+            setValue(value);

+        }

+

+        public void setId(String id) {

+            this.id = id;

+        }

+

+        public String getId() {

+            return id;

+        }

+

+        public void setValue(String value) {

+            this.value = value;

+        }

+

+        public String getValue() {

+            return value;

+        }

+    }

+

+    @Override

+    protected String getDescription() {

+        return "Test adding items in a filtered BeanItemContainer.";

+    }

+

+    @Override

+    protected Integer getTicketNumber() {

+        return new Integer(1061);

+    }

+

+    @Override

+    protected void setup() {

+        table = new Table();

+        try {

+            container = new BeanItemContainer<TestBean>(TestBean.class);

+            table.setContainerDataSource(container);

+

+            table.setWidth(300, Sizeable.UNITS_PIXELS);

+            table.setSelectable(true);

+            table.setMultiSelect(false);

+            table.setEditable(true);

+            table.setImmediate(true);

+            // table.addContainerProperty("column1", String.class, "test");

+

+            for (int i = 0; i < 25; ++i) {

+                container.addItem(new TestBean("Item " + i, "Value for " + i));

+            }

+

+            VerticalLayout vl = new VerticalLayout();

+

+            // activate & deactivate filtering

+            filterString = new TextField("Filter string:", "1");

+            vl.addComponent(filterString);

+

+            final CheckBox cb = new CheckBox("Filter on value",

+                    new Button.ClickListener() {

+                        public void buttonClick(ClickEvent event) {

+                            container.removeAllContainerFilters();

+                            if (((CheckBox) event.getSource()).booleanValue()) {

+                                container.addContainerFilter("value",

+                                        filterString.getValue().toString(),

+                                        false, false);

+                            }

+                        }

+                    });

+            cb.setImmediate(true);

+            vl.addComponent(cb);

+

+            nextLabel = new Label();

+            nextLabel.setCaption("Next id: " + nextToAdd);

+            vl.addComponent(nextLabel);

+

+            // addItemAt(idx), addItemAfter(selection), addItem()

+

+            final Button addItemButton = new Button("addItem()",

+                    new Button.ClickListener() {

+                        public void buttonClick(ClickEvent event) {

+                            container.addItem(new TestBean("addItem() "

+                                    + nextToAdd, "value " + nextToAdd));

+                            nextToAdd++;

+                            nextLabel.setCaption("Next id: " + nextToAdd);

+                        }

+                    });

+            addItemButton.setImmediate(true);

+            vl.addComponent(addItemButton);

+

+            final Button addItemAfterButton = new Button("addItemAfter()",

+                    new Button.ClickListener() {

+                        public void buttonClick(ClickEvent event) {

+                            Object selection = table.getValue();

+                            if (selection == null) {

+                                return;

+                            }

+                            TestBean bean = new TestBean("addItemAfter() "

+                                    + nextToAdd, "value " + nextToAdd);

+                            Item item = container.addItemAfter(selection, bean);

+                            if (item == null) {

+                                getMainWindow().showNotification(

+                                        "Adding item after " + selection

+                                                + " failed");

+                            }

+                            nextToAdd++;

+                            nextLabel.setCaption("Next id: " + nextToAdd);

+                        }

+                    });

+            addItemAfterButton.setImmediate(true);

+            vl.addComponent(addItemAfterButton);

+

+            position = new TextField("Position:", "0");

+            vl.addComponent(position);

+

+            final Button addItemAtButton = new Button("addItemAt()",

+                    new Button.ClickListener() {

+                        public void buttonClick(ClickEvent event) {

+                            int index = Integer.parseInt(position.getValue()

+                                    .toString());

+                            TestBean bean = new TestBean("addItemAt() "

+                                    + nextToAdd, "value " + nextToAdd);

+                            Item item = container.addItemAt(index, bean);

+                            if (item == null) {

+                                getMainWindow().showNotification(

+                                        "Adding item at index "

+                                                + position.getValue()

+                                                + " failed");

+                            }

+                            nextToAdd++;

+                            nextLabel.setCaption("Next id: " + nextToAdd);

+                        }

+                    });

+            addItemAtButton.setImmediate(true);

+            vl.addComponent(addItemAtButton);

+

+            getLayout().addComponent(table);

+            getLayout().addComponent(vl);

+        } catch (Exception ex) {

+            ex.printStackTrace();

+        }

+    }

+

+}

diff --git a/src/com/itmill/toolkit/tests/containers/BeanItemContainerTest.java b/src/com/itmill/toolkit/tests/containers/BeanItemContainerTest.java
index 7cfb17041d..cc5b04a699 100644
--- a/src/com/itmill/toolkit/tests/containers/BeanItemContainerTest.java
+++ b/src/com/itmill/toolkit/tests/containers/BeanItemContainerTest.java
@@ -25,7 +25,7 @@ public class BeanItemContainerTest {
 
         c = new BeanItemContainer<Hello>(col);
 
-        System.out.print(c + " contains " + c.size() + "objects");
+        System.out.print(c + " contains " + c.size() + " objects");
 
     }