[dasher] Fix event dispatch loop

[Patrick Welche <pwelche src gnome org>]

commit 61644af05b8bdb4d9dacf253872a0538a909d785
Author: Patrick Welche <prlw1 cam ac uk>
Date:   Mon Feb 25 13:01:25 2013 +0000

    Fix event dispatch loop
    
    The loop was based on iterating through a deque of event handlers.
    The problem is that an event handler, such as is called when changing
    the alphabet, can cause new observer objects to be created, which
    register their handlers by adding them to the deque. Unfortunately
    according to the C++ standard
    
      23.3.3.4 deque modifiers
    
      Effects: An insertion in the middle of the deque invalidates all
      the iterators and references to elements of the deque. An insertion
      at either end of the deque invalidates all the iterators to the
      deque, but has no effect on the validity of references to elements
      of the deque.
    
    so the iterators become invalid during the event dispatch loop.
    
    g++ is extremely lenient, allowing push_back additions to the deque,
    but other compilers aren't - afterall I could implement push_back
    by malloc(oldsize + 1) and copy, so any pointers to elements in
    the old array would not point to current new array locations and
    be invalid.

 Src/DasherCore/DasherInterfaceBase.cpp |    2 +-
 Src/DasherCore/Observable.h            |   54 +++++++++++++++----------------
 2 files changed, 27 insertions(+), 29 deletions(-)
---
diff --git a/Src/DasherCore/DasherInterfaceBase.cpp b/Src/DasherCore/DasherInterfaceBase.cpp
index c4bcd2d..15cb781 100644
--- a/Src/DasherCore/DasherInterfaceBase.cpp
+++ b/Src/DasherCore/DasherInterfaceBase.cpp
@@ -87,7 +87,7 @@ static char THIS_FILE[] = __FILE__;
 
 CDasherInterfaceBase::CDasherInterfaceBase(CSettingsStore *pSettingsStore) : CSettingsUser(pSettingsStore), 
m_pSettingsStore(pSettingsStore), m_pDasherModel(new CDasherModel()), m_pFramerate(new CFrameRate(this)), 
m_pLockLabel(NULL) {
   
-  pSettingsStore->Register(this, true);
+  pSettingsStore->Register(this);
   
   // Ensure that pointers to 'owned' objects are set to NULL.
   m_DasherScreen = NULL;
diff --git a/Src/DasherCore/Observable.h b/Src/DasherCore/Observable.h
index ad20e24..b21f0a0 100644
--- a/Src/DasherCore/Observable.h
+++ b/Src/DasherCore/Observable.h
@@ -1,7 +1,7 @@
 #ifndef __eventhandler_h__
 #define __eventhandler_h__
 
-#include <deque>
+#include <list>
 #include <algorithm>
 
 template <typename  T> class Observable;
@@ -19,14 +19,15 @@ public:
 template <typename T> class Observable {
 public:
   Observable();
-  void Register(Observer<T> *pLstnr, bool bLast=false);
+  void Register(Observer<T> *pLstnr);
   void Unregister(Observer<T> *pLstnr);
 protected:
   void DispatchEvent(T t);
 private:
-  typedef typename std::deque< Observer<T>* > ListenerList;
+  typedef typename std::list< Observer<T>* > ListenerList;
   typedef typename ListenerList::iterator L_it;
   ListenerList m_vListeners;
+  ListenerList m_vListenersToAdd;
   int m_iInHandler;
 };
 
@@ -50,51 +51,48 @@ protected:
   Observable<T> *m_pEventHandler;
 };
 
-template <typename T> void Observable<T>::Register(Observer<T> *pListener, bool bLast) {
-  L_it it = std::find(m_vListeners.begin(), m_vListeners.end(), pListener);
-  if (it != m_vListeners.end()) {
-    //already in list...in ok place?
-    if (it == (bLast ? m_vListeners.end() : m_vListeners.begin())) return;
-    //put it first or last, these are the only places we can guarantee satisfy the constraint of bLast
-    if (m_iInHandler) *it=NULL; else m_vListeners.erase(it);
-  }
-  if (bLast) m_vListeners.push_back(pListener); else m_vListeners.insert(m_vListeners.begin(),pListener);
+template <typename T> void Observable<T>::Register(Observer<T> *pListener) {
+  if (m_iInHandler == 0)
+    m_vListeners.push_back(pListener);
+  else
+    m_vListenersToAdd.push_back(pListener);
 }
 
 template <typename T> void Observable<T>::Unregister(Observer<T> *pListener) {
-  L_it it = std::find(m_vListeners.begin(), m_vListeners.end(), pListener);
-  if (it==m_vListeners.end()) return;
-  if (m_iInHandler) {
-    //remove listener, but leave behind its slot, so as not to upset the in-progress iteration
-    *it=NULL;
-  } else {
-    m_vListeners.erase(it);
+  if (m_iInHandler == 0)
+    m_vListeners.remove(pListener);
+  else {
+    L_it it = std::find(m_vListeners.begin(), m_vListeners.end(), pListener);
+    if (it != m_vListeners.end())
+         *it = NULL;
   }
 }
 
 template <typename T> void Observable<T>::DispatchEvent(T evt) {
-  
+
   // Speed up start-up before any listeners are registered
   if (m_vListeners.empty()) return;
 
   // We may end up here recursively, so keep track of how far down we
   // are, and only permit new handlers to be registered after all
   // messages are processed.
-  
+
   // An alternative approach would be a message queue - this might actually be a bit more sensible
   ++m_iInHandler;
+
   // Loop through components and notify them of the event
   for(L_it I=m_vListeners.begin(), E=m_vListeners.end(); I!=E; I++) {
-    if (*I) //don't dispatch to NULLs (slots remaining from listeners removed during iteration)
+    if (*I != NULL) { // Listener not removed during iteration
       (*I)->HandleEvent(evt);
+    }
   }
-  
+
   --m_iInHandler;
-  
-  if(m_iInHandler == 0) {
-    for (L_it it=m_vListeners.begin(); it!=m_vListeners.end(); it++)
-      if (!(*it)) //slot remaining from listener removed during iteration. Shuffle up...
-        m_vListeners.erase(it--);
+
+  if (m_iInHandler == 0) {
+    m_vListeners.remove(NULL);
+    m_vListeners.splice(m_vListeners.end(), m_vListenersToAdd);
+    m_vListeners.unique();
   }
 }
 #endif