Simplifies and fix bugs in FbTk::Timer

* Calling Timer::setTimeout() from within Timer::start() might lead to ugly
  behavior (as experienced in bugs #3590078, #3600143, etc; see commit
  4d307dcd10af9d817ff5c05fc40ae7487564cb31, fixes the problem partially).

* Stop a timer first, then call the handler (via Timer::fireTimeout()). A
  given handler might call Timer::start() again, which (re)adds the Timer
  to the control list .. the following Timer::stop() would remove it again.

* Use 'm_start' as indicator if timer is running.

* Move the (now quite short) code of ::addTimer / ::removeTimer
  into the Timer::start() and Timer::stop() functions.

2 files changed, 36 insertions, 41 deletions

diff --git a/src/FbTk/Timer.cc b/src/FbTk/Timer.cc
index eff3af4..2404773 100644
--- a/src/FbTk/Timer.cc
+++ b/src/FbTk/Timer.cc
@@ -55,7 +55,6 @@
 #include <vector>
 #include <set>
 
-
 namespace {
 
 struct TimerCompare {
@@ -64,51 +63,30 @@ struct TimerCompare {
     }
 };
 typedef std::set<FbTk::Timer*, TimerCompare> TimerList;
-
 TimerList s_timerlist;
 
-
-/// add a timer to the static list
-void addTimer(FbTk::Timer *timer) {
-
-    assert(timer);
-    int interval = timer->getInterval();
-
-    // interval timers have their timeout change every time they are started!
-    if (interval != 0) {
-        timer->setTimeout(interval * FbTk::FbTime::IN_SECONDS);
-    }
-
-    s_timerlist.insert(timer);
-}
-
-/// remove a timer from the static list
-void removeTimer(FbTk::Timer *timer) {
-
-    assert(timer);
-    s_timerlist.erase(timer);
-}
-
-
 }
 
 
 namespace FbTk {
 
-Timer::Timer():m_timing(false), m_once(false), m_interval(0) {
+Timer::Timer() :
+    m_once(false),
+    m_interval(0),
+    m_start(0) {
 
 }
 
 Timer::Timer(const RefCount<Slot<void> > &handler):
     m_handler(handler),
-    m_timing(false),
     m_once(false),
-    m_interval(0) {
+    m_interval(0),
+    m_start(0) {
 }
 
 
 Timer::~Timer() {
-    if (isTiming()) stop();
+    stop();
 }
 
 
@@ -131,19 +109,31 @@ void Timer::setCommand(const RefCount<Slot<void> > &cmd) {
 
 void Timer::start() {
 
-    m_start = FbTk::FbTime::now();
-
     // only add Timers that actually DO something
-    if ((! m_timing || m_interval != 0) && m_handler) {
-        m_timing = true;
-        ::addTimer(this);
+    if ( ( ! isTiming() || m_interval > 0 ) && m_handler) {
+
+        // in case start() gets triggered on a started 
+        // timer with 'm_interval != 0' we have to remove
+        // it from s_timerlist before restarting it
+        stop();
+
+        m_start = FbTk::FbTime::now();
+
+        // interval timers have their timeout change every 
+        // time they are started!
+        if (m_interval != 0) {
+            m_timeout = m_interval * FbTk::FbTime::IN_SECONDS;
+        }
+        s_timerlist.insert(this);
     }
 }
 
 
 void Timer::stop() {
-    m_timing = false;
-    ::removeTimer(this);
+    if (isTiming()) {
+        s_timerlist.erase(this);
+        m_start = 0;
+    }
 }
 
 uint64_t Timer::getEndTime() const {
@@ -216,10 +206,16 @@ void Timer::updateTimers(int fd) {
 
         FbTk::Timer& t = *timeouts[i];
 
-        t.fireTimeout();
+        // first we stop the timer to remove it
+        // from s_timerlist
         t.stop();
 
-        if (! t.doOnce()) { // restart the current timer
+        // then we call the handler which might (re)start 't'
+        // on it's own
+        t.fireTimeout();
+
+        // restart 't' if needed
+        if (!t.doOnce() && !t.isTiming()) {
             t.start();
         }
     }
diff --git a/src/FbTk/Timer.hh b/src/FbTk/Timer.hh
index bdaf3b8..2e82f2a 100644
--- a/src/FbTk/Timer.hh
+++ b/src/FbTk/Timer.hh
@@ -61,7 +61,7 @@ public:
 
     static void updateTimers(int file_descriptor);
 
-    int isTiming() const { return m_timing; }
+    int isTiming() const { return (m_start > 0); }
     int getInterval() const { return m_interval; }
 
     int doOnce() const { return m_once; }
@@ -77,11 +77,10 @@ protected:
 private:
     RefCount<Slot<void> > m_handler; ///< what to do on a timeout
 
-    bool m_timing; ///< clock running?
     bool m_once;  ///< do timeout only once?
     int m_interval; ///< Is an interval-only timer (e.g. clock), in seconds
 
-    uint64_t m_start;   ///< start time in microseconds
+    uint64_t m_start;   ///< start time in microseconds, 0 if not running
     uint64_t m_timeout; ///< time length in microseconds
 };