changeset 2816:3ec5209e2c07

RT-28790 Map/Set null values/keys not handled correctly by listeners
author Martin Sladecek <martin.sladecek@oracle.com>
date Wed, 06 Mar 2013 15:54:32 +0100
parents 7fa12f6c2e16
children df3ce8a3d2aa
files javafx-beans/src/com/sun/javafx/binding/MapExpressionHelper.java javafx-beans/src/com/sun/javafx/binding/SetExpressionHelper.java javafx-beans/src/com/sun/javafx/collections/ObservableMapWrapper.java javafx-beans/src/com/sun/javafx/collections/ObservableSetWrapper.java javafx-beans/test/javafx/collections/ObservableMapTestBase.java javafx-beans/test/javafx/collections/ObservableSetTestBase.java
diffstat 6 files changed, 153 insertions(+), 23 deletions(-) [+]
line wrap: on
line diff
--- a/javafx-beans/src/com/sun/javafx/binding/MapExpressionHelper.java	Wed Mar 06 13:08:40 2013 +0100
+++ b/javafx-beans/src/com/sun/javafx/binding/MapExpressionHelper.java	Wed Mar 06 15:54:32 2013 +0100
@@ -285,9 +285,9 @@
                     for (final Map.Entry<K, V> element : oldValue.entrySet()) {
                         final K key = element.getKey();
                         final V oldEntry = element.getValue();
-                        final V newEntry = currentValue.get(key);
-                        if (newEntry != null) {
-                            if (!newEntry.equals(oldEntry)) {
+                        if (currentValue.containsKey(key)) {
+                            final V newEntry = currentValue.get(key);
+                            if (oldEntry == null ? newEntry != null : !newEntry.equals(oldEntry)) {
                                 listener.onChanged(change.setPut(key, oldEntry, newEntry));
                             }
                         } else {
@@ -600,9 +600,9 @@
                                 for (final Map.Entry<K, V> element : oldValue.entrySet()) {
                                     final K key = element.getKey();
                                     final V oldEntry = element.getValue();
-                                    final V newEntry = currentValue.get(key);
-                                    if (newEntry != null) {
-                                        if (!newEntry.equals(oldEntry)) {
+                                    if (currentValue.containsKey(key)) {
+                                        final V newEntry = currentValue.get(key);
+                                        if (oldEntry == null ? newEntry != null : !newEntry.equals(oldEntry)) {
                                             change.setPut(key, oldEntry, newEntry);
                                             for (int i = 0; i < curListChangeSize; i++) {
                                                 curListChangeList[i].onChanged(change);
@@ -640,6 +640,8 @@
         private K key;
         private V old;
         private V added;
+        private boolean removeOp;
+        private boolean addOp;
 
         public SimpleChange(ObservableMap<K, V> set) {
             super(set);
@@ -650,12 +652,16 @@
             key = source.getKey();
             old = source.getValueRemoved();
             added = source.getValueAdded();
+            addOp = source.wasAdded();
+            removeOp = source.wasRemoved();
         }
 
         public SimpleChange<K, V> setRemoved(K key, V old) {
             this.key = key;
             this.old = old;
             this.added = null;
+            addOp = false;
+            removeOp = true;
             return this;
         }
 
@@ -663,6 +669,8 @@
             this.key = key;
             this.old = null;
             this.added = added;
+            addOp = true;
+            removeOp = false;
             return this;
         }
         
@@ -670,17 +678,19 @@
             this.key = key;
             this.old = old;
             this.added = added;
+            addOp = true;
+            removeOp = true;
             return this;
         }
 
         @Override
         public boolean wasAdded() {
-            return added != null;
+            return addOp;
         }
 
         @Override
         public boolean wasRemoved() {
-            return old != null;
+            return removeOp;
         }
 
         @Override
@@ -697,5 +707,21 @@
         public V getValueRemoved() {
             return old;
         }
+
+        @Override
+        public String toString() {
+            StringBuilder builder = new StringBuilder();
+            if (addOp) {
+                if (removeOp) {
+                    builder.append("replaced ").append(old).append("by ").append(added);
+                } else {
+                    builder.append("added ").append(added);
+                }
+            } else {
+                builder.append("removed ").append(old);
+            }
+            builder.append(" at key ").append(key);
+            return builder.toString();
+        }
     }
 }
--- a/javafx-beans/src/com/sun/javafx/binding/SetExpressionHelper.java	Wed Mar 06 13:08:40 2013 +0100
+++ b/javafx-beans/src/com/sun/javafx/binding/SetExpressionHelper.java	Wed Mar 06 15:54:32 2013 +0100
@@ -622,6 +622,7 @@
 
         private E old;
         private E added;
+        private boolean addOp;
 
         public SimpleChange(ObservableSet<E> set) {
             super(set);
@@ -631,28 +632,31 @@
             super(set);
             old = source.getElementRemoved();
             added = source.getElementAdded();
+            addOp = source.wasAdded();
         }
 
         public SimpleChange<E> setRemoved(E old) {
             this.old = old;
             this.added = null;
+            addOp = false;
             return this;
         }
 
         public SimpleChange<E> setAdded(E added) {
             this.old = null;
             this.added = added;
+            addOp = true;
             return this;
         }
 
         @Override
         public boolean wasAdded() {
-            return added != null;
+            return addOp;
         }
 
         @Override
         public boolean wasRemoved() {
-            return old != null;
+            return !addOp;
         }
 
         @Override
@@ -664,5 +668,11 @@
         public E getElementRemoved() {
             return old;
         }
+
+        @Override
+        public String toString() {
+            return addOp ? "added " + added : "removed " + old;
+        }
+        
     }
 }
--- a/javafx-beans/src/com/sun/javafx/collections/ObservableMapWrapper.java	Wed Mar 06 13:08:40 2013 +0100
+++ b/javafx-beans/src/com/sun/javafx/collections/ObservableMapWrapper.java	Wed Mar 06 15:54:32 2013 +0100
@@ -60,6 +60,7 @@
 
         public SimpleChange(K key, V old, V added, boolean wasAdded, boolean wasRemoved) {
             super(ObservableMapWrapper.this);
+            assert(wasAdded || wasRemoved);
             this.key = key;
             this.old = old;
             this.added = added;
@@ -92,6 +93,22 @@
             return old;
         }
 
+        @Override
+        public String toString() {
+            StringBuilder builder = new StringBuilder();
+            if (wasAdded) {
+                if (wasRemoved) {
+                    builder.append("replaced ").append(old).append("by ").append(added);
+                } else {
+                    builder.append("added ").append(added);
+                }
+            } else {
+                builder.append("removed ").append(old);
+            }
+            builder.append(" at key ").append(key);
+            return builder.toString();
+        }
+
     }
 
     protected void callObservers(MapChangeListener.Change<K,V> change) {
--- a/javafx-beans/src/com/sun/javafx/collections/ObservableSetWrapper.java	Wed Mar 06 13:08:40 2013 +0100
+++ b/javafx-beans/src/com/sun/javafx/collections/ObservableSetWrapper.java	Wed Mar 06 15:54:32 2013 +0100
@@ -52,25 +52,23 @@
         this.backingSet = set;
     }
 
-    private class SimpleChange extends SetChangeListener.Change<E> {
+    private class SimpleAddChange extends SetChangeListener.Change<E> {
 
-        private E old;
-        private E added;
+        private final E added;
 
-        public SimpleChange(E old, E added) {
+        public SimpleAddChange(E added) {
             super(ObservableSetWrapper.this);
-            this.old = old;
             this.added = added;
         }
 
         @Override
         public boolean wasAdded() {
-            return added != null;
+            return true;
         }
 
         @Override
         public boolean wasRemoved() {
-            return old != null;
+            return false;
         }
 
         @Override
@@ -80,8 +78,50 @@
 
         @Override
         public E getElementRemoved() {
-            return old;
+            return null;
         }
+
+        @Override
+        public String toString() {
+            return "added " + added;
+        }
+        
+    }
+    
+    private class SimpleRemoveChange extends SetChangeListener.Change<E> {
+
+        private final E removed;
+
+        public SimpleRemoveChange(E removed) {
+            super(ObservableSetWrapper.this);
+            this.removed = removed;
+        }
+
+        @Override
+        public boolean wasAdded() {
+            return false;
+        }
+
+        @Override
+        public boolean wasRemoved() {
+            return true;
+        }
+
+        @Override
+        public E getElementAdded() {
+            return null;
+        }
+
+        @Override
+        public E getElementRemoved() {
+            return removed;
+        }
+
+        @Override
+        public String toString() {
+            return "removed " + removed;
+        }
+        
     }
 
     private void callObservers(SetChangeListener.Change<E> change) {
@@ -183,7 +223,7 @@
             @Override
             public void remove() {
                 backingIt.remove();
-                callObservers(new SimpleChange(lastElement, null));
+                callObservers(new SimpleRemoveChange(lastElement));
             }
         };
     }
@@ -226,7 +266,7 @@
     public boolean add(E o) {
         boolean ret = backingSet.add(o);
         if (ret) {
-            callObservers(new SimpleChange(null, o));
+            callObservers(new SimpleAddChange(o));
         }
         return ret;
     }
@@ -244,7 +284,7 @@
     public boolean remove(Object o) {
         boolean ret = backingSet.remove(o);
         if (ret) {
-            callObservers(new SimpleChange((E)o, null));
+            callObservers(new SimpleRemoveChange((E)o));
         }
         return ret;
     }
@@ -314,7 +354,7 @@
             if (remove == c.contains(element)) {
                 removed = true;
                 i.remove();
-                callObservers(new SimpleChange(element, null));
+                callObservers(new SimpleRemoveChange(element));
             }
         }
         return removed;
@@ -330,7 +370,7 @@
         for (Iterator<E> i = backingSet.iterator(); i.hasNext(); ) {
             E element = i.next();
             i.remove();
-            callObservers(new SimpleChange(element, null));
+            callObservers(new SimpleRemoveChange(element));
         }
     }
 
--- a/javafx-beans/test/javafx/collections/ObservableMapTestBase.java	Wed Mar 06 13:08:40 2013 +0100
+++ b/javafx-beans/test/javafx/collections/ObservableMapTestBase.java	Wed Mar 06 15:54:32 2013 +0100
@@ -32,6 +32,7 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.TreeMap;
 import java.util.concurrent.ConcurrentHashMap;
 
 import static javafx.collections.MockMapObserver.Call.call;
@@ -113,6 +114,26 @@
 
         assertEquals(observer.getCallsNumber(), 6);
     }
+    
+    @Test
+    public void testPutRemove_NullKey() {
+        if (map instanceof ConcurrentHashMap || map instanceof TreeMap) {
+            return; // Do not perform on ConcurrentHashMap and TreeMap, as they doesn't accept null keys
+        }
+
+        observableMap.put(null, "abc");
+
+        assertEquals(4, observableMap.size());
+        
+        observableMap.remove(null);
+
+        assertEquals(3, observableMap.size());
+
+        observer.assertAdded(0, tup((String)null, "abc"));
+        observer.assertRemoved(1, tup((String)null, "abc"));
+
+        assertEquals(observer.getCallsNumber(), 2);
+    }
 
     @Test
     @SuppressWarnings("unchecked")
--- a/javafx-beans/test/javafx/collections/ObservableSetTestBase.java	Wed Mar 06 13:08:40 2013 +0100
+++ b/javafx-beans/test/javafx/collections/ObservableSetTestBase.java	Wed Mar 06 15:54:32 2013 +0100
@@ -29,6 +29,7 @@
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Set;
+import java.util.TreeSet;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -139,5 +140,20 @@
         assertTrue(observableSet.contains("foo"));
         assertFalse(observableSet.contains("bar"));
     }
+    
+    @Test
+    public void testNull() {
+        if (set instanceof TreeSet) {
+            return; // TreeSet doesn't accept nulls
+        }
+        observableSet.add(null);
+        assertEquals(4, observableSet.size());
+        
+        observer.assertAdded(tup((String)null));
+        
+        observableSet.remove(null);
+        assertEquals(3, observableSet.size());
+        observer.assertRemoved(tup((String)null));
+    }
 
 }