Clean Table.propertyValueConverters if the property is removed (#8168)

Clean Table.propertyValueConverters if the property it attached to
is removed, or if the container is changed and the new container does
not contain a property with that identifier with a matching type.

Change-Id: I894ee6462ea7b9c1f9138a24fcb84db829165c7d

1 files changed, 65 insertions, 9 deletions

diff --git a/server/src/com/vaadin/ui/Table.java b/server/src/com/vaadin/ui/Table.java
index 06e82dedcb..00a7038afd 100644
--- a/server/src/com/vaadin/ui/Table.java
+++ b/server/src/com/vaadin/ui/Table.java
@@ -2647,6 +2647,10 @@ public class Table extends AbstractSelect implements Action.Container,
      * new container contains properties that are not meant to be shown you
      * should use {@link Table#setContainerDataSource(Container, Collection)}
      * instead, especially if the table is editable.
+     * <p>
+     * Keeps propertyValueConverters if the corresponding id exists in the new
+     * data source and is of a compatible type.
+     * </p>
      * 
      * @param newDataSource
      *            the new data source.
@@ -2681,9 +2685,14 @@ public class Table extends AbstractSelect implements Action.Container,
     /**
      * Sets the container data source and the columns that will be visible.
      * Columns are shown in the collection's iteration order.
+     * <p>
+     * Keeps propertyValueConverters if the corresponding id exists in the new
+     * data source and is of a compatible type.
+     * </p>
      * 
      * @see Table#setContainerDataSource(Container)
      * @see Table#setVisibleColumns(Object[])
+     * @see Table#setConverter(Object, Converter<String, ?>)
      * 
      * @param newDataSource
      *            the new data source.
@@ -2702,6 +2711,26 @@ public class Table extends AbstractSelect implements Action.Container,
             visibleIds = new ArrayList<Object>();
         }
 
+        // Retain propertyValueConverters if their corresponding ids are
+        // properties of the new
+        // data source and are of a compatible type
+        if (propertyValueConverters != null) {
+            Collection<?> newPropertyIds = newDataSource
+                    .getContainerPropertyIds();
+            LinkedList<Object> retainableValueConverters = new LinkedList<Object>();
+            for (Object propertyId : newPropertyIds) {
+                Converter<String, ?> converter = getConverter(propertyId);
+                if (converter != null) {
+                    if (typeIsCompatible(converter.getModelType(),
+                            newDataSource.getType(propertyId))) {
+                        retainableValueConverters.add(propertyId);
+                    }
+                }
+            }
+            propertyValueConverters.keySet().retainAll(
+                    retainableValueConverters);
+        }
+
         // Assures that the data source is ordered by making unordered
         // containers ordered by wrapping them
         if (newDataSource instanceof Container.Ordered) {
@@ -2738,6 +2767,34 @@ public class Table extends AbstractSelect implements Action.Container,
     }
 
     /**
+     * Checks if class b can be safely assigned to class a.
+     * 
+     * @param a
+     * @param b
+     * @return
+     */
+    private boolean typeIsCompatible(Class<?> a, Class<?> b) {
+        // TODO Implement this check properly
+        // Map<Class<?>, Class<?>> typemap = new HashMap<Class<?>, Class<?>>();
+        // typemap.put(byte.class, Byte.class);
+        // typemap.put(short.class, Short.class);
+        // typemap.put(int.class, Integer.class);
+        // typemap.put(long.class, Long.class);
+        // typemap.put(float.class, Float.class);
+        // typemap.put(double.class, Double.class);
+        // typemap.put(char.class, Character.class);
+        // typemap.put(boolean.class, Boolean.class);
+        // if (typemap.containsKey(a)) {
+        // a = typemap.get(a);
+        // }
+        // if (typemap.containsKey(b)) {
+        // b = typemap.get(b);
+        // }
+        // return a.isAssignableFrom(b);
+        return true;
+    }
+
+    /**
      * Gets items ids from a range of key values
      * 
      * @param startRowKey
@@ -4229,6 +4286,8 @@ public class Table extends AbstractSelect implements Action.Container,
         columnIcons.remove(propertyId);
         columnHeaders.remove(propertyId);
         columnFooters.remove(propertyId);
+        // If a propertyValueConverter was defined for the property, remove it.
+        propertyValueConverters.remove(propertyId);
 
         return super.removeContainerProperty(propertyId);
     }
@@ -5844,16 +5903,13 @@ public class Table extends AbstractSelect implements Action.Container,
             throw new IllegalArgumentException("PropertyId " + propertyId
                     + " must be in the container");
         }
-        // FIXME: This check should be here but primitive types like Boolean
-        // formatter for boolean property must be handled
 
-        // if (!converter.getSourceType().isAssignableFrom(getType(propertyId)))
-        // {
-        // throw new IllegalArgumentException("Property type ("
-        // + getType(propertyId)
-        // + ") must match converter source type ("
-        // + converter.getSourceType() + ")");
-        // }
+        if (!typeIsCompatible(converter.getModelType(), getType(propertyId))) {
+            throw new IllegalArgumentException("Property type ("
+                    + getType(propertyId)
+                    + ") must match converter source type ("
+                    + converter.getModelType() + ")");
+        }
         propertyValueConverters.put(propertyId,
                 (Converter<String, Object>) converter);
         refreshRowCache();