fix timer issues (esp clock not updating on multiple screens)

4 files changed, 58 insertions, 49 deletions

diff --git a/ChangeLog b/ChangeLog
index d7319a7..7a2b5f8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
  (Format: Year/Month/Day)
 Changes for 1.0rc2:
+*06/06/19:
+   * Fix bug #1507671, clock not updating on multiple screens (Simon)
+     - time() and gettimeofday() are inconsistent, only use gettimeofday
+     - clean up Timer stuff a bit, fixed some other bugs
+     FbTk/Timer.hh/cc ClockTool.cc
 *06/06/18:
    * Fixed bug #1507754 iconbar text was not updated to match titlebar (Henrik)
      WinClient.cc
diff --git a/src/ClockTool.cc b/src/ClockTool.cc
index 2c0ad46..efc0ffb 100644
--- a/src/ClockTool.cc
+++ b/src/ClockTool.cc
@@ -45,6 +45,7 @@
 #else
   #include <time.h>
 #endif
+#include <sys/time.h>
 #include <string>
 #include <typeinfo>
 
@@ -242,7 +243,9 @@ unsigned int ClockTool::height() const {
 
 void ClockTool::updateTime() {
     // update clock
-    time_t the_time = time(0);
+    timeval now;
+    gettimeofday(&now, 0);
+    time_t the_time = now.tv_sec;
 
     if (the_time != -1) {
         char time_string[255];
diff --git a/src/FbTk/Timer.cc b/src/FbTk/Timer.cc
index 15da0fb..9492ac6 100644
--- a/src/FbTk/Timer.cc
+++ b/src/FbTk/Timer.cc
@@ -98,6 +98,15 @@ void Timer::stop() {
     removeTimer(this); //remove us from the list
 }
 
+void Timer::makeEndTime(timeval &tm) const {
+    tm.tv_sec = m_start.tv_sec + m_timeout.tv_sec;
+    tm.tv_usec = m_start.tv_usec + m_timeout.tv_usec;
+    if (tm.tv_usec >= 1000000) {
+        tm.tv_usec -= 1000000;
+        tm.tv_sec++;
+    }
+}
+
 
 void Timer::fireTimeout() {
     if (*m_handler)
@@ -111,79 +120,69 @@ void Timer::updateTimers(int fd) {
     FD_ZERO(&rfds);
     FD_SET(fd, &rfds);
 
+    bool overdue = false;
   
     if (!m_timerlist.empty()) {
         gettimeofday(&now, 0);
 
-        tm.tv_sec = tm.tv_usec = 0l;
-
         Timer *timer = m_timerlist.front();
 
-        const timeval &start = timer->getStartTime();
-        const timeval &length = timer->getTimeout();
-        tm.tv_sec = start.tv_sec +
-            length.tv_sec - now.tv_sec;
-        tm.tv_usec = start.tv_usec +
-            length.tv_usec - now.tv_usec;
+        timer->makeEndTime(tm);
 
-        while (tm.tv_usec >= 1000000) {
-            tm.tv_sec++;
-            tm.tv_usec -= 1000000;
-        }
+        tm.tv_sec -= now.tv_sec;
+        tm.tv_usec -= now.tv_usec;
 
         while (tm.tv_usec < 0) {
             if (tm.tv_sec > 0) {
                 tm.tv_sec--;
                 tm.tv_usec += 1000000;
             } else {
+                overdue = true;
                 tm.tv_usec = 0;
                 break;
             }
         }
 
-        if (tm.tv_sec < 0) {
+        if (tm.tv_sec < 0) { // usec zero-ed above if negative
             tm.tv_sec = 0;
             tm.tv_usec = 0;
+            overdue = true;
         }
 
         timeout = &tm;
     }
 
-    if (select(fd + 1, &rfds, 0, 0, timeout) != 0)
+    if (!overdue && select(fd + 1, &rfds, 0, 0, timeout) != 0)
         // didn't time out! x events pending
         return;
 
     TimerList::iterator it;
 
+    // check for timer timeout
+    gettimeofday(&now, 0);
+
     // someone set the date of the machine BACK
     // so we have to adjust the start_time
-    static time_t last_time = time(0);
-    if (time(0) < last_time) {
+    static time_t last_time = 0;
+    if (now.tv_sec < last_time) {
     
-        time_t delta = time(0) - last_time;
+        time_t delta = last_time - now.tv_sec;
 
         for (it = m_timerlist.begin(); it != m_timerlist.end(); it++) {
-            (*it)->m_start.tv_sec += delta;
+            (*it)->m_start.tv_sec -= delta;
         }
     }
+    last_time = now.tv_sec;
 
 
-    // check for timer timeout
-    gettimeofday(&now, 0);
-
     //must check end ...the timer might remove
     //it self from the list (should be fixed in the future)
     for(it = m_timerlist.begin(); it != m_timerlist.end(); ) {
         //This is to make sure we don't get an invalid iterator
         //when we do fireTimeout
         Timer &t = *(*it);
-        const timeval &start = t.getStartTime();
-        const timeval &length = t.getTimeout();
 
-        tm.tv_sec = start.tv_sec +
-            length.tv_sec;
-        tm.tv_usec = start.tv_usec +
-            length.tv_usec;
+        t.makeEndTime(tm);
 
         if (((now.tv_sec < tm.tv_sec) ||
              (now.tv_sec == tm.tv_sec && now.tv_usec < tm.tv_usec)))
@@ -192,15 +191,10 @@ void Timer::updateTimers(int fd) {
         t.fireTimeout();
         // restart the current timer so that the start time is updated
         if (! t.doOnce()) {
-            if (t.getInterval() != 0) {
-                it = m_timerlist.erase(it);
-            }
+            // must erase so that it's put into the right place in the list
+            it = m_timerlist.erase(it);
+            t.m_timing = false;
             t.start();
-
-            // Note that this mustn't be done if we're deleting the
-            // entry from the list, so therefore it's not in the update
-            // section of the for loop
-            it++;
         } else {
             // Since the default stop behaviour results in the timer
             // being removed, we must remove it here, so that the iterator
@@ -211,15 +205,14 @@ void Timer::updateTimers(int fd) {
         }
     }
 
-    last_time = time(0);
 }
 
 void Timer::addTimer(Timer *timer) {
     assert(timer);
     int interval = timer->getInterval();
     // interval timers have their timeout change every time they are started!
+    timeval tm;
     if (interval != 0) {
-        timeval tm;
         tm.tv_sec = timer->getStartTime().tv_sec;
         tm.tv_usec = timer->getStartTime().tv_usec;
 
@@ -233,13 +226,19 @@ void Timer::addTimer(Timer *timer) {
         timer->setTimeout(tm);
     }
 
+    // set timeval to the time-of-trigger
+    timer->makeEndTime(tm);
+
+    // timer list is sorted by trigger time (i.e. start plus timeout)
     TimerList::iterator it = m_timerlist.begin();
     TimerList::iterator it_end = m_timerlist.end();
-    int index = 0;
-    for (; it != it_end; ++it, ++index) {
-        if (((*it)->getTimeout().tv_sec > timer->getTimeout().tv_sec) ||
-            (((*it)->getTimeout().tv_sec == timer->getTimeout().tv_sec) &&
-             ((*it)->getTimeout().tv_usec >= timer->getTimeout().tv_usec))) {
+    for (; it != it_end; ++it) {
+        timeval trig;
+        (*it)->makeEndTime(trig);
+
+        if ((trig.tv_sec > tm.tv_sec) ||
+            (trig.tv_sec == tm.tv_sec &&
+             trig.tv_usec >= tm.tv_usec)) {
             break;
         }
     }
diff --git a/src/FbTk/Timer.hh b/src/FbTk/Timer.hh
index 773d5b8..189c2af 100644
--- a/src/FbTk/Timer.hh
+++ b/src/FbTk/Timer.hh
@@ -74,12 +74,14 @@ public:
     /// update all timers
     static void updateTimers(int file_descriptor);
 
-    inline int isTiming() const { return m_timing; }
-    inline int getInterval() const { return m_interval; }
-    inline int doOnce() const { return m_once; }
+    int isTiming() const { return m_timing; }
+    int getInterval() const { return m_interval; }
     
-    inline const timeval &getTimeout() const { return m_timeout; }
-    inline const timeval &getStartTime() const { return m_start; }
+    int doOnce() const { return m_once; }
+    
+    const timeval &getTimeout() const { return m_timeout; }
+    const timeval &getStartTime() const { return m_start; }
+    void makeEndTime(timeval &tm) const;
 
 protected:
     /// force a timeout
@@ -92,7 +94,7 @@ private:
     static void removeTimer(Timer *timer);
         
     typedef std::list<Timer *> TimerList;
-    static TimerList m_timerlist; ///< list of all timers
+    static TimerList m_timerlist; ///< list of all timers, sorted by next trigger time (start + timeout)
         
     RefCount<Command> m_handler; ///< what to do on a timeout