Ray sez:

- ObserverList shouldn't extend ArrayList; bad mojo.
- Policies should be an enum.
- Use CopyOnWriteArrayList instead of our hand-rolled snapshotting.
MDB addends:
- ObserverList should be an interface (turned out to be easier to make it an
abstract class).
- WeakObserverList should be simpler, and turns out to be so, once we simplify
ObserverList public interface and remove ArraListness.

This should all be source compatible with previous usage (modulo deprecation of
the ObserverList notification constants, which most code is probably not using,
since the nice factory methods are much more concise).

src/main/java/com/samskivert/util/ObserverList.java

@@ -23,6 +23,9 @@ package com.samskivert.util;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import static com.samskivert.Log.log;
 
@@ -86,7 +89,7 @@ import static com.samskivert.Log.log;
  * <p> Other usage patterns are most certainly conceivable, and hopefully these two will give you a
  * useful starting point for determining what is the most appropriate usage for your needs.
  */
-public class ObserverList<T> extends ArrayList<T>
+public abstract class ObserverList<T>
 {
     /**
      * Instances of this interface are used to apply methods to all observers in a list.
@@ -102,17 +105,26 @@ public class ObserverList<T> extends ArrayList<T>
         public boolean apply (T observer);
     }
 
+    /** Notification policies. */
+    public enum Policy {
         /** A notification ordering policy indicating that the observers should be notified in the
          * order they were added and that the notification should be done on a snapshot of the
          * array. */
-    public static final int SAFE_IN_ORDER_NOTIFY = 1;
+        SAFE_IN_ORDER,
 
         /** A notification ordering policy wherein the observers are notified last to first so that
          * they can be removed during the notification process and new observers added will not
-     * inadvertently be notified as well, but no copy of the observer list need be made. This will
+         * inadvertently be notified as well, but no copy of the observer list need be made. This
-     * not work if observers are added or removed from arbitrary positions in the list during a
+         * will not work if observers are added or removed from arbitrary positions in the list
-     * notification call. */
+         * during a notification call. */
-    public static final int FAST_UNSAFE_NOTIFY = 2;
+        FAST_UNSAFE;
+    };
+
+    /** @deprecated Use {@link Policy.SAFE_IN_ORDER}. */
+    @Deprecated public static final Policy SAFE_IN_ORDER_NOTIFY = Policy.SAFE_IN_ORDER;
+
+    /** @deprecated Use {@link Policy.FAST_UNSAFE}. */
+    @Deprecated public static final Policy FAST_UNSAFE_NOTIFY = Policy.FAST_UNSAFE;
 
     /**
      * A convenience method for creating an observer list that avoids duplicating the type
@@ -120,7 +132,7 @@ public class ObserverList<T> extends ArrayList<T>
      */
     public static <T> ObserverList<T> newSafeInOrder ()
     {
-        return new ObserverList<T>(SAFE_IN_ORDER_NOTIFY);
+        return newList(Policy.SAFE_IN_ORDER, false);
     }
 
     /**
@@ -129,161 +141,131 @@ public class ObserverList<T> extends ArrayList<T>
      */
     public static <T> ObserverList<T> newFastUnsafe ()
     {
-        return new ObserverList<T>(FAST_UNSAFE_NOTIFY);
+        return newList(Policy.FAST_UNSAFE, false);
     }
 
     /**
      * A convenience method for creating an observer list that avoids duplicating the type
      * parameter on the right hand side.
      */
-    public static <T> ObserverList<T> newList (int notifyPolicy, boolean allowDups)
+    public static <T> ObserverList<T> newList (Policy notifyPolicy, boolean allowDups)
     {
-        return new ObserverList<T>(notifyPolicy, allowDups);
+        return new Impl<T>(notifyPolicy, allowDups);
     }
 
     /**
-     * Creates an empty observer list with the supplied notification policy and that only allows
+     * Adds an observer at the specified index.
-     * observers to observe the list once.
+     * @return true if the observer was added, false if it was already in the list.
-     *
-     * @param notifyPolicy Either {@link #SAFE_IN_ORDER_NOTIFY} or {@link #FAST_UNSAFE_NOTIFY}.
      */
-    public ObserverList (int notifyPolicy)
+    public abstract boolean add (int index, T element);
-    {
-        this(notifyPolicy, false);
-    }
 
     /**
-     * Creates an empty observer list with the supplied notification and duplicate observer policy.
+     * Adds an observer to the end of the list.
-     *
+     * @return true if the observer was added, false if it was already in the list.
-     * @param notifyPolicy Either {@link #SAFE_IN_ORDER_NOTIFY} or {@link #FAST_UNSAFE_NOTIFY}.
-     * @param allowDups whether to allow observers to be added to the list more than once.
      */
-    public ObserverList (int notifyPolicy, boolean allowDups)
+    public abstract boolean add (T element);
-    {
-        // make sure the policy is valid
-        if (notifyPolicy != SAFE_IN_ORDER_NOTIFY && notifyPolicy != FAST_UNSAFE_NOTIFY) {
-            throw new RuntimeException("Invalid notification policy [policy=" + notifyPolicy + "]");
-        }
 
-        _policy = notifyPolicy ;
+    /**
-        _allowDups = allowDups;
+     * Removes the specified observer from the list.
-    }
+     * @return true if the observer was located and removed, false if it does not exist.
-
+     */
-    @Override
+    public abstract boolean remove (T element);
-    public void add (int index, T element)
-    {
-        if (element == null) {
-            throw new NullPointerException("Null observers not allowed.");
-        }
-        if (!isDuplicate(element)) {
-            super.add(index, element);
-        }
-    }
-
-    @Override
-    public boolean add (T element)
-    {
-        if (element == null) {
-            throw new NullPointerException("Null observers not allowed.");
-        }
-        return isDuplicate(element) ? false : super.add(element);
-    }
-
-    @Override
-    public boolean addAll (Collection<? extends T> c)
-    {
-        throw new UnsupportedOperationException(
-            "Observers may only be added via ObserverList.add(Object).");
-    }
-
-    @Override
-    public boolean addAll (int index, Collection<? extends T> c)
-    {
-        throw new UnsupportedOperationException(
-            "Observers may only be added via ObserverList.add(Object).");
-    }
-
-    @Override
-    public int indexOf (Object element)
-    {
-        // indexOf and lastIndexOf are implemented using reference equality so that contains() also
-        // uses reference equality
-        for (int ii = 0, nn = size(); ii < nn; ii++) {
-            if (element == get(ii)) {
-                return ii;
-            }
-        }
-        return -1;
-    }
-
-    @Override
-    public int lastIndexOf (Object element)
-    {
-        // indexOf and lastIndexOf are implemented using reference equality so that contains() also
-        // uses reference equality
-        for (int ii = size() - 1; ii >= 0; ii--) {
-            if (element == get(ii)) {
-                return ii;
-            }
-        }
-        return -1;
-    }
-
-    @Override
-    public boolean remove (Object element)
-    {
-        int dex = indexOf(element);
-        if (dex == -1) {
-            return false; // not found
-        }
-        remove(dex);
-        return true;
-    }
 
     /**
      * Applies the supplied observer operation to all observers in the list in a manner conforming
      * to the notification ordering policy specified at construct time.
      */
-    @SuppressWarnings("unchecked")
+    public abstract void apply (ObserverOp<T> obop);
-    public void apply (ObserverOp<T> obop)
-    {
-        int ocount = size();
-        if (ocount == 0) {
-            return;
-        }
-        if (_policy == SAFE_IN_ORDER_NOTIFY) {
-            // if we have to notify our observers in order, we need to create a snapshot of the
-            // observer array at the time we start the notification to ensure that modifications to
-            // the array during notification don't hose us
-            if (_snap == null || _snap.length < ocount || _snap.length > (ocount << 3)) {
-                _snap = (T[])new Object[ocount];
-            }
-            toArray(_snap);
-            for (int ii = 0; ii < ocount; ii++) {
-                if (!checkedApply(obop, _snap[ii])) {
-                    remove(_snap[ii]);
-                }
-            }
-            // clear out the snapshot so its contents can be gc'd
-            Arrays.fill(_snap, null);
-
-        } else if (_policy == FAST_UNSAFE_NOTIFY) {
-            for (int ii = ocount-1; ii >= 0; ii--) {
-                if (!checkedApply(obop, get(ii))) {
-                    remove(ii);
-                }
-            }
-        }
-    }
 
     /**
-     * Returns true and issues a warning if this list does not allow duplicates and the supplied
+     * Returns the number of observers in this list.
-     * observer is already in the list. Returns false if the supplied observer is not a duplicate.
      */
-    protected boolean isDuplicate (T obs)
+    public abstract int size ();
-    {
+
+    /**
+     * Clears all observers from the list.
+     */
+    public abstract void clear ();
+
+    protected static class Impl<T> extends ObserverList<T> {
+        protected Impl (Policy notifyPolicy, boolean allowDups) {
+            _policy = notifyPolicy;
+            _allowDups = allowDups;
+            _list = (_policy == Policy.SAFE_IN_ORDER) ?
+                new CopyOnWriteArrayList<T>() : new ArrayList<T>();
+        }
+
+        @Override public boolean add (int index, T element) {
+            if (element == null) throw new NullPointerException("Null observers not allowed.");
+            if (isDuplicate(element)) return false;
+            _list.add(index, element);
+            return true;
+        }
+
+        @Override public boolean add (T element) {
+            if (element == null) throw new NullPointerException("Null observers not allowed.");
+            if (isDuplicate(element)) return false;
+            _list.add(element);
+            return true;
+        }
+
+        @Override public boolean remove (T element) {
+            int idx = indexOf(element);
+            if (idx < 0) return false;
+            _list.remove(idx);
+            return true;
+        }
+
+        @Override public void apply (ObserverOp<T> obop) {
+            switch (_policy) {
+            case SAFE_IN_ORDER:
+                // our copy on write array list will prevent us from getting hosed if modifications
+                // take place during iteration
+                Iterator<T> iter = _list.iterator();
+                for (int ii = 0; iter.hasNext(); ii++) {
+                    T elem = iter.next();
+                    if (!checkedApply(obop, elem)) {
+                        // can't remove via COWArrayList iterator, and to be totally safe (because
+                        // we don't know if any additions or removals have taken place while we
+                        // were iterating), we have to scan the whole list to locate this element
+                        // so that we can remove it
+                        remove(elem);
+                    }
+                }
+                break;
+
+            case FAST_UNSAFE:
+                for (int ii = _list.size()-1; ii >= 0; ii--) {
+                    if (!checkedApply(obop, _list.get(ii))) {
+                        _list.remove(ii);
+                    }
+                }
+                break;
+            }
+        }
+
+        @Override public int size () {
+            return _list.size();
+        }
+
+        @Override public void clear () {
+            _list.clear();
+        }
+
+        /** Used to determine whether an element is in the list. */
+        protected int indexOf (T element) {
+            for (int ii = 0, ll = _list.size(); ii < ll; ii++) {
+                if (_list.get(ii) == element) return ii;
+            }
+            return -1;
+        }
+
+        /** Returns true and issues a warning if this list does not allow duplicates and the
+         * supplied observer is already in the list. Returns false if the supplied observer is not
+         * a duplicate. */
+        protected boolean isDuplicate (T obs) {
             // make sure we're not violating the list constraints
-        if (!_allowDups && contains(obs)) {
+            if (!_allowDups && (indexOf(obs) >= 0)) {
                 log.warning("Observer attempted to observe list it's already observing!", "obs", obs,
                             new Exception());
                 return true;
@@ -291,6 +273,16 @@ public class ObserverList<T> extends ArrayList<T>
             return false;
         }
 
+        /** The notification policy. */
+        protected Policy _policy;
+
+        /** Whether to allow observers to observe more than once simultaneously. */
+        protected boolean _allowDups;
+
+        /** Our list of observers. */
+        protected List<T> _list;
+    }
+
     /**
      * Applies the operation to the observer, catching and logging any exceptions thrown in the
      * process.
@@ -305,14 +297,4 @@ public class ObserverList<T> extends ArrayList<T>
             return true;
         }
     }
-
-    /** The notification policy. */
-    protected int _policy;
-
-    /** Whether to allow observers to observe more than once simultaneously. */
-    protected boolean _allowDups;
-
-    /** Used to avoid creating a new snapshot array every time we notify our observers if the size
-     * has not changed. */
-    protected T[] _snap;
 }

src/main/java/com/samskivert/util/ResultListenerList.java

@@ -23,23 +23,23 @@ package com.samskivert.util;
 /**
  * Multiplexes ResultListener responses to multiple ResultListeners.
  */
-public class ResultListenerList<T> extends ObserverList<ResultListener<T>>
+public class ResultListenerList<T> extends ObserverList.Impl<ResultListener<T>>
     implements ResultListener<T>
 {
     /**
-     * Create a ResultListenerList with the FAST_UNSAFE_NOTIFY policy.
+     * Create a ResultListenerList with the FAST_UNSAFE notification policy.
      */
     public ResultListenerList ()
     {
-        super(FAST_UNSAFE_NOTIFY);
+        super(Policy.FAST_UNSAFE, false);
     }
 
     /**
      * Create a ResultListenerList with your own notifyPolicy.
      */
-    public ResultListenerList (int notifyPolicy)
+    public ResultListenerList (Policy notifyPolicy)
     {
-        super(notifyPolicy);
+        super(notifyPolicy, false);
     }
 
     /**

src/main/java/com/samskivert/util/WeakObserverList.java

@@ -30,7 +30,7 @@ import com.samskivert.util.ObserverList.ObserverOp;
  * An {@link ObserverList} equivalent that does not prevent added observers from being
  * garbage-collected.
  */
-public class WeakObserverList<T> extends AbstractList<T>
+public class WeakObserverList<T> extends ObserverList<T>
 {
     /**
      * A convenience method for creating an observer list that avoids duplicating the type
@@ -38,7 +38,7 @@ public class WeakObserverList<T> extends AbstractList<T>
      */
     public static <T> WeakObserverList<T> newSafeInOrder ()
     {
-        return new WeakObserverList<T>(ObserverList.SAFE_IN_ORDER_NOTIFY);
+        return newList(Policy.SAFE_IN_ORDER, false);
     }
 
     /**
@@ -47,111 +47,47 @@ public class WeakObserverList<T> extends AbstractList<T>
      */
     public static <T> WeakObserverList<T> newFastUnsafe ()
     {
-        return new WeakObserverList<T>(ObserverList.FAST_UNSAFE_NOTIFY);
+        return newList(Policy.FAST_UNSAFE, false);
     }
 
     /**
      * A convenience method for creating an observer list that avoids duplicating the type
      * parameter on the right hand side.
      */
-    public static <T> WeakObserverList<T> newList (int notifyPolicy, boolean allowDups)
+    public static <T> WeakObserverList<T> newList (Policy notifyPolicy, boolean allowDups)
     {
         return new WeakObserverList<T>(notifyPolicy, allowDups);
     }
 
-    /**
+    @Override public boolean add (int index, T element)
-     * Creates an empty observer list with the supplied notification
-     * policy and that only allows observers to observe the list once.
-     *
-     * @param notifyPolicy Either {@link ObserverList#SAFE_IN_ORDER_NOTIFY} or {@link
-     * ObserverList#FAST_UNSAFE_NOTIFY}.
-     */
-    public WeakObserverList (int notifyPolicy)
     {
-        this(notifyPolicy, false);
+        return _delegate.add(index, new WeakReference<T>(element));
     }
 
-    /**
+    @Override public boolean add (T element)
-     * Creates an empty observer list with the supplied notification and
-     * duplicate observer policy.
-     *
-     * @param notifyPolicy Either {@link ObserverList#SAFE_IN_ORDER_NOTIFY} or {@link
-     * ObserverList#FAST_UNSAFE_NOTIFY}.
-     * @param allowDups whether to allow observers to be added to the list
-     * more than once.
-     */
-    public WeakObserverList (int notifyPolicy, boolean allowDups)
     {
-        _wrappedList = new WrappedList<T>(notifyPolicy, allowDups);
+        return _delegate.add(new WeakReference<T>(element));
     }
 
-    @Override // documentation inherited
+    @Override public boolean remove (T element)
-    public int size ()
     {
-        return _wrappedList.size();
+        return _delegate.remove(new WeakReference<T>(element));
     }
 
-    @Override // documentation inherited
+    @Override public void apply (ObserverOp<T> obop)
-    public boolean contains (Object element)
-    {
-        @SuppressWarnings("unchecked") T value = (T)element;
-        return _wrappedList.contains(new WeakReference<T>(value));
-    }
-
-    @Override // documentation inherited
-    public boolean remove (Object element)
-    {
-        @SuppressWarnings("unchecked") T value = (T)element;
-        return _wrappedList.remove(new WeakReference<T>(value));
-    }
-
-    @Override // documentation inherited
-    public int indexOf (Object element)
-    {
-        @SuppressWarnings("unchecked") T value = (T)element;
-        return _wrappedList.indexOf(new WeakReference<T>(value));
-    }
-
-    @Override // documentation inherited
-    public int lastIndexOf (Object element)
-    {
-        @SuppressWarnings("unchecked") T value = (T)element;
-        return _wrappedList.lastIndexOf(new WeakReference<T>(value));
-    }
-
-    @Override // documentation inherited
-    public T get (int index)
-    {
-        return _wrappedList.get(index).get();
-    }
-
-    @Override // documentation inherited
-    public T set (int index, T element)
-    {
-        return _wrappedList.set(index, new WeakReference<T>(element)).get();
-    }
-
-    @Override // documentation inherited
-    public void add (int index, T element)
-    {
-        _wrappedList.add(index, new WeakReference<T>(element));
-    }
-
-    @Override // documentation inherited
-    public T remove (int index)
-    {
-        return _wrappedList.remove(index).get();
-    }
-
-    /**
-     * Applies the supplied observer operation to all observers in the
-     * list in a manner conforming to the notification ordering policy
-     * specified at construct time.
-     */
-    public void apply (ObserverOp<T> obop)
     {
         _derefOp.init(obop);
-        _wrappedList.apply(_derefOp);
+        _delegate.apply(_derefOp);
+    }
+
+    @Override public int size ()
+    {
+        return _delegate.size();
+    }
+
+    @Override public void clear ()
+    {
+        _delegate.clear();
     }
 
     /**
@@ -159,30 +95,31 @@ public class WeakObserverList<T> extends AbstractList<T>
      */
     public void prune ()
     {
-        for (int ii = _wrappedList.size() - 1; ii >= 0; ii--) {
+        // applying an op prunes collected observers, so just apply a NOOP op
-            if (_wrappedList.get(ii).get() == null) {
+        apply(new ObserverOp<T>() {
-                _wrappedList.remove(ii);
+            public boolean apply (T obs) {
+                return true;
             }
+        });
     }
+
+    protected WeakObserverList (Policy notifyPolicy, boolean allowDups)
+    {
+        _delegate = new WrappedList<T>(notifyPolicy, allowDups);
     }
 
     /**
      * An operation that resolves a reference and applies a wrapped op.
      */
-    protected static class DerefOp<T>
+    protected static class DerefOp<T> implements ObserverOp<WeakReference<T>>
-        implements ObserverOp<WeakReference<T>>
-    {
-        /**
-         * (Re)initializes this op with a reference to the wrapped op.
-         */
-        public void init (ObserverOp<T> op)
     {
+        /** (Re)initializes this op with a reference to the wrapped op. */
+        public void init (ObserverOp<T> op) {
             _op = op;
         }
 
         // documentation inherited from interface ObserverOp
-        public boolean apply (WeakReference<T> ref)
+        public boolean apply (WeakReference<T> ref) {
-        {
             T observer = ref.get();
             return observer != null && _op.apply(observer);
         }
@@ -194,37 +131,23 @@ public class WeakObserverList<T> extends AbstractList<T>
     /**
      * ObserverList extension that dereferences elements when searching for a value.
      */
-    protected static class WrappedList<T> extends ObserverList<WeakReference<T>>
+    protected static class WrappedList<T> extends ObserverList.Impl<WeakReference<T>>
     {
-        public WrappedList (int notifyPolicy, boolean allowDups) {
+        public WrappedList (Policy notifyPolicy, boolean allowDups) {
             super(notifyPolicy, allowDups);
         }
 
-        @Override public int indexOf (Object element) {
+        @Override protected int indexOf (WeakReference<T> ref) {
-            @SuppressWarnings("unchecked") WeakReference<T> ref = (WeakReference<T>)element;
             T value = ref.get();
-            for (int ii = 0, nn = size(); ii < nn; ii++) {
+            for (int ii = 0, ll = _list.size(); ii < ll; ii++) {
-                if (value == get(ii).get()) {
+                if (_list.get(ii).get() == value) return ii;
-                    return ii;
-                }
-            }
-            return -1;
-        }
-
-        @Override public int lastIndexOf (Object element) {
-            @SuppressWarnings("unchecked") WeakReference<T> ref = (WeakReference<T>)element;
-            T value = ref.get();
-            for (int ii = size() - 1; ii >= 0; ii--) {
-                if (value == get(ii).get()) {
-                    return ii;
-                }
             }
             return -1;
         }
     }
 
-    /** The wrapped list. */
+    /** A delegate list that contains weak reference wrapped elements. */
-    protected WrappedList<T> _wrappedList;
+    protected WrappedList<T> _delegate;
 
     /** The wrapper op. */
     protected DerefOp<T> _derefOp = new DerefOp<T>();