Explicitly request timer repetition

One-shot timers are more common, so let's change the API a bit to make
that use case simpler. This API also makes it more clear what is
happening.

18 files changed, 63 insertions, 73 deletions

diff --git a/common/rfb/EncodeManager.cxx b/common/rfb/EncodeManager.cxx
index bc15e74a..c2658a70 100644
--- a/common/rfb/EncodeManager.cxx
+++ b/common/rfb/EncodeManager.cxx
@@ -297,7 +297,7 @@ void EncodeManager::writeLosslessRefresh(const Region& req, const PixelBuffer* p
            Region(), Point(), pb, renderedCursor);
 }
 
-bool EncodeManager::handleTimeout(Timer* t)
+void EncodeManager::handleTimeout(Timer* t)
 {
   if (t == &recentChangeTimer) {
     // Any lossy region that wasn't recently updated can
@@ -307,10 +307,8 @@ bool EncodeManager::handleTimeout(Timer* t)
 
     // Will there be more to do? (i.e. do we need another round)
     if (!lossyRegion.subtract(pendingRefreshRegion).is_empty())
-      return true;
+      t->repeat();
   }
-
-  return false;
 }
 
 void EncodeManager::doUpdate(bool allowLossy, const Region& changed_,
diff --git a/common/rfb/EncodeManager.h b/common/rfb/EncodeManager.h
index f2fd4ca4..33484db8 100644
--- a/common/rfb/EncodeManager.h
+++ b/common/rfb/EncodeManager.h
@@ -61,7 +61,7 @@ namespace rfb {
                               size_t maxUpdateSize);
 
   protected:
-    virtual bool handleTimeout(Timer* t);
+    virtual void handleTimeout(Timer* t);
 
     void doUpdate(bool allowLossy, const Region& changed,
                   const Region& copied, const Point& copy_delta,
diff --git a/common/rfb/SConnection.cxx b/common/rfb/SConnection.cxx
index 33b2d850..402b0c04 100644
--- a/common/rfb/SConnection.cxx
+++ b/common/rfb/SConnection.cxx
@@ -287,11 +287,11 @@ bool SConnection::processInitMsg()
   return reader_->readClientInit();
 }
 
-bool SConnection::handleAuthFailureTimeout(Timer* /*t*/)
+void SConnection::handleAuthFailureTimeout(Timer* /*t*/)
 {
   if (state_ != RFBSTATE_SECURITY_FAILURE) {
     close("SConnection::handleAuthFailureTimeout: invalid state");
-    return false;
+    return;
   }
 
   try {
@@ -304,12 +304,10 @@ bool SConnection::handleAuthFailureTimeout(Timer* /*t*/)
     os->flush();
   } catch (rdr::Exception& e) {
     close(e.str());
-    return false;
+    return;
   }
 
   close(authFailureMsg.c_str());
-
-  return false;
 }
 
 void SConnection::throwConnFailedException(const char* format, ...)
diff --git a/common/rfb/SConnection.h b/common/rfb/SConnection.h
index b163d627..0bd6afdb 100644
--- a/common/rfb/SConnection.h
+++ b/common/rfb/SConnection.h
@@ -245,7 +245,7 @@ namespace rfb {
     bool processSecurityFailure();
     bool processInitMsg();
 
-    bool handleAuthFailureTimeout(Timer* t);
+    void handleAuthFailureTimeout(Timer* t);
 
     int defaultMajorVersion, defaultMinorVersion;
 
diff --git a/common/rfb/Timer.cxx b/common/rfb/Timer.cxx
index dc17606e..37aaad83 100644
--- a/common/rfb/Timer.cxx
+++ b/common/rfb/Timer.cxx
@@ -66,37 +66,15 @@ int Timer::checkTimeouts() {
   gettimeofday(&start, 0);
   while (pending.front()->isBefore(start)) {
     Timer* timer;
-    timeval before, dueTime;
 
     timer = pending.front();
     pending.pop_front();
 
-    dueTime = timer->dueTime;
-    gettimeofday(&before, 0);
-    if (timer->cb->handleTimeout(timer)) {
-      timeval now;
+    timer->lastDueTime = timer->dueTime;
+    timer->cb->handleTimeout(timer);
 
-      if (msBetween(&dueTime, &timer->dueTime) != 0) {
-        vlog.error("Timer incorrectly modified whilst repeating");
-        timer->dueTime = dueTime;
-      }
-
-      gettimeofday(&now, 0);
-
-      timer->dueTime = addMillis(timer->dueTime, timer->timeoutMs);
-      if (timer->isBefore(now)) {
-        // Time has jumped forwards, or we're not getting enough
-        // CPU time for the timers
-
-        timer->dueTime = addMillis(before, timer->timeoutMs);
-        if (timer->isBefore(now))
-          timer->dueTime = now;
-      }
-
-      insertTimer(timer);
-    } else if (pending.empty()) {
+    if (pending.empty())
       return -1;
-    }
   }
   return getNextTimeout();
 }
@@ -144,6 +122,29 @@ void Timer::start(int timeoutMs_) {
   insertTimer(this);
 }
 
+void Timer::repeat() {
+  timeval now;
+
+  gettimeofday(&now, 0);
+
+  if (isStarted()) {
+    vlog.error("Incorrectly repeating already running timer");
+    stop();
+  }
+
+  if (msBetween(&lastDueTime, &dueTime) != 0)
+    vlog.error("Timer incorrectly modified whilst repeating");
+
+  dueTime = addMillis(lastDueTime, timeoutMs);
+  if (isBefore(now)) {
+    // Time has jumped forwards, or we're not getting enough
+    // CPU time for the timers
+    dueTime = now;
+  }
+
+  insertTimer(this);
+}
+
 void Timer::stop() {
   pending.remove(this);
 }
diff --git a/common/rfb/Timer.h b/common/rfb/Timer.h
index 3059a9b3..9775afd5 100644
--- a/common/rfb/Timer.h
+++ b/common/rfb/Timer.h
@@ -1,5 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2018 Pierre Ossman for Cendio AB
+ * Copyright 2018-2024 Pierre Ossman for Cendio AB
  * 
  * This is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -51,7 +51,7 @@ namespace rfb {
       //   running, causing another timeout after the appropriate
       //   interval.
       //   If the handler returns false then the Timer is cancelled.
-      virtual bool handleTimeout(Timer* t) = 0;
+      virtual void handleTimeout(Timer* t) = 0;
 
       virtual ~Callback() {}
     };
@@ -76,6 +76,12 @@ namespace rfb {
     //   be implicitly cancelled and re-started.
     void start(int timeoutMs_);
 
+    // repeat()
+    //   Restarts the timer in a way that repeats that last timeout.
+    //   This allows you to have a periodic timer without the risk of
+    //   accumulating drift caused by processing delays.
+    void repeat();
+
     // stop()
     //   Cancels the timer.
     void stop();
@@ -100,7 +106,7 @@ namespace rfb {
     bool isBefore(timeval other);
 
   protected:
-    timeval dueTime;
+    timeval dueTime, lastDueTime;
     int timeoutMs;
     Callback* cb;
 
@@ -113,14 +119,14 @@ namespace rfb {
   template<class T> class MethodTimer
     : public Timer, public Timer::Callback {
   public:
-    MethodTimer(T* obj_, bool (T::*cb_)(Timer*))
+    MethodTimer(T* obj_, void (T::*cb_)(Timer*))
       : Timer(this), obj(obj_), cb(cb_) {}
 
-    virtual bool handleTimeout(Timer* t) { return (obj->*cb)(t); }
+    virtual void handleTimeout(Timer* t) { (obj->*cb)(t); }
 
   private:
     T* obj;
-    bool (T::*cb)(Timer*);
+    void (T::*cb)(Timer*);
   };
 
 };
diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx
index 906f5f66..ffbf8be7 100644
--- a/common/rfb/VNCSConnectionST.cxx
+++ b/common/rfb/VNCSConnectionST.cxx
@@ -799,7 +799,7 @@ void VNCSConnectionST::supportsLEDState()
   writer()->writeLEDState();
 }
 
-bool VNCSConnectionST::handleTimeout(Timer* t)
+void VNCSConnectionST::handleTimeout(Timer* t)
 {
   try {
     if ((t == &congestionTimer) ||
@@ -811,8 +811,6 @@ bool VNCSConnectionST::handleTimeout(Timer* t)
 
   if (t == &idleTimer)
     close("Idle timeout");
-
-  return false;
 }
 
 bool VNCSConnectionST::isShiftPressed()
diff --git a/common/rfb/VNCSConnectionST.h b/common/rfb/VNCSConnectionST.h
index edc0391e..85bfd38f 100644
--- a/common/rfb/VNCSConnectionST.h
+++ b/common/rfb/VNCSConnectionST.h
@@ -140,7 +140,7 @@ namespace rfb {
     virtual void supportsLEDState();
 
     // Timer callbacks
-    virtual bool handleTimeout(Timer* t);
+    virtual void handleTimeout(Timer* t);
 
     // Internal methods
 
diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx
index 560a0ffa..bd1fecf3 100644
--- a/common/rfb/VNCServerST.cxx
+++ b/common/rfb/VNCServerST.cxx
@@ -623,22 +623,20 @@ SConnection* VNCServerST::getConnection(network::Socket* sock) {
   return 0;
 }
 
-bool VNCServerST::handleTimeout(Timer* t)
+void VNCServerST::handleTimeout(Timer* t)
 {
   if (t == &frameTimer) {
     // We keep running until we go a full interval without any updates
     if (comparer->is_empty())
-      return false;
+      return;
 
     writeUpdate();
 
     // If this is the first iteration then we need to adjust the timeout
-    if (frameTimer.getTimeoutMs() != 1000/rfb::Server::frameRate) {
+    if (frameTimer.getTimeoutMs() != 1000/rfb::Server::frameRate)
       frameTimer.start(1000/rfb::Server::frameRate);
-      return false;
-    }
-
-    return true;
+    else
+      frameTimer.repeat();
   } else if (t == &idleTimer) {
     slog.info("MaxIdleTime reached, exiting");
     desktop->terminate();
@@ -649,8 +647,6 @@ bool VNCServerST::handleTimeout(Timer* t)
     slog.info("MaxConnectionTime reached, exiting");
     desktop->terminate();
   }
-
-  return false;
 }
 
 void VNCServerST::queryConnection(VNCSConnectionST* client,
diff --git a/common/rfb/VNCServerST.h b/common/rfb/VNCServerST.h
index f688b317..55d0c889 100644
--- a/common/rfb/VNCServerST.h
+++ b/common/rfb/VNCServerST.h
@@ -155,7 +155,7 @@ namespace rfb {
   protected:
 
     // Timer callbacks
-    virtual bool handleTimeout(Timer* t);
+    virtual void handleTimeout(Timer* t);
 
     // - Internal methods
 
diff --git a/unix/vncconfig/QueryConnectDialog.cxx b/unix/vncconfig/QueryConnectDialog.cxx
index e13af34b..e725de7d 100644
--- a/unix/vncconfig/QueryConnectDialog.cxx
+++ b/unix/vncconfig/QueryConnectDialog.cxx
@@ -74,14 +74,13 @@ void QueryConnectDialog::buttonActivate(TXButton* b) {
     callback->queryRejected();
 }
   
-bool QueryConnectDialog::handleTimeout(rfb::Timer* /*t*/) {
+void QueryConnectDialog::handleTimeout(rfb::Timer* t) {
   if (timeUntilReject-- == 0) {
     unmap();
     callback->queryTimedOut();
-    return false;
   } else {
     refreshTimeout();
-    return true;
+    t->repeat();
   }
 }
 
diff --git a/unix/vncconfig/QueryConnectDialog.h b/unix/vncconfig/QueryConnectDialog.h
index f685dc34..dcf64e40 100644
--- a/unix/vncconfig/QueryConnectDialog.h
+++ b/unix/vncconfig/QueryConnectDialog.h
@@ -43,7 +43,7 @@ class QueryConnectDialog : public TXDialog, public TXEventHandler,
   void handleEvent(TXWindow*, XEvent* ) { }
   void deleteWindow(TXWindow*);
   void buttonActivate(TXButton* b);
-  bool handleTimeout(rfb::Timer* t);
+  void handleTimeout(rfb::Timer* t);
  private:
   void refreshTimeout();
   TXLabel addressLbl, address, userLbl, user, timeoutLbl, timeout;
diff --git a/unix/xserver/hw/vnc/XserverDesktop.cc b/unix/xserver/hw/vnc/XserverDesktop.cc
index 64b2e614..ab7f29d2 100644
--- a/unix/xserver/hw/vnc/XserverDesktop.cc
+++ b/unix/xserver/hw/vnc/XserverDesktop.cc
@@ -518,13 +518,11 @@ void XserverDesktop::keyEvent(uint32_t keysym, uint32_t keycode, bool down)
   vncKeyboardEvent(keysym, keycode, down);
 }
 
-bool XserverDesktop::handleTimeout(Timer* t)
+void XserverDesktop::handleTimeout(Timer* t)
 {
   if (t == &queryConnectTimer) {
     server->approveConnection(queryConnectSocket, false,
                               "The attempt to prompt the user to "
                               "accept the connection failed");
   }
-
-  return false;
 }
diff --git a/unix/xserver/hw/vnc/XserverDesktop.h b/unix/xserver/hw/vnc/XserverDesktop.h
index 677097a6..8aa97d97 100644
--- a/unix/xserver/hw/vnc/XserverDesktop.h
+++ b/unix/xserver/hw/vnc/XserverDesktop.h
@@ -112,7 +112,7 @@ protected:
                          network::SocketServer* sockserv,
                          bool read, bool write);
 
-  virtual bool handleTimeout(rfb::Timer* t);
+  virtual void handleTimeout(rfb::Timer* t);
 
 private:
 
diff --git a/vncviewer/EmulateMB.cxx b/vncviewer/EmulateMB.cxx
index 72335eb8..cc680df4 100644
--- a/vncviewer/EmulateMB.cxx
+++ b/vncviewer/EmulateMB.cxx
@@ -277,13 +277,13 @@ void EmulateMB::filterPointerEvent(const rfb::Point& pos, int buttonMask)
   }
 }
 
-bool EmulateMB::handleTimeout(rfb::Timer *t)
+void EmulateMB::handleTimeout(rfb::Timer *t)
 {
   int action1, action2;
   int buttonMask;
 
   if (&timer != t)
-    return false;
+    return;
 
   if ((state > 10) || (state < 0))
     throw rfb::Exception(_("Invalid state for 3 button emulation"));
@@ -310,8 +310,6 @@ bool EmulateMB::handleTimeout(rfb::Timer *t)
   }
 
   state = stateTab[state][4][2];
-
-  return false;
 }
 
 void EmulateMB::sendAction(const rfb::Point& pos, int buttonMask, int action)
diff --git a/vncviewer/EmulateMB.h b/vncviewer/EmulateMB.h
index 132f44fe..77fdec66 100644
--- a/vncviewer/EmulateMB.h
+++ b/vncviewer/EmulateMB.h
@@ -31,7 +31,7 @@ public:
 protected:
   virtual void sendPointerEvent(const rfb::Point& pos, int buttonMask)=0;
 
-  virtual bool handleTimeout(rfb::Timer *t);
+  virtual void handleTimeout(rfb::Timer *t);
 
 private:
   void sendAction(const rfb::Point& pos, int buttonMask, int action);
diff --git a/vncviewer/GestureHandler.cxx b/vncviewer/GestureHandler.cxx
index c3cc1531..ed99555e 100644
--- a/vncviewer/GestureHandler.cxx
+++ b/vncviewer/GestureHandler.cxx
@@ -323,14 +323,12 @@ bool GestureHandler::hasDetectedGesture()
   return true;
 }
 
-bool GestureHandler::handleTimeout(rfb::Timer* t)
+void GestureHandler::handleTimeout(rfb::Timer* t)
 {
   if (t == &longpressTimer)
     longpressTimeout();
   else if (t == &twoTouchTimer)
     twoTouchTimeout();
-
-  return false;
 }
 
 void GestureHandler::longpressTimeout()
diff --git a/vncviewer/GestureHandler.h b/vncviewer/GestureHandler.h
index 372b7865..b07454df 100644
--- a/vncviewer/GestureHandler.h
+++ b/vncviewer/GestureHandler.h
@@ -42,7 +42,7 @@ class GestureHandler : public rfb::Timer::Callback {
   private:
     bool hasDetectedGesture();
 
-    virtual bool handleTimeout(rfb::Timer* t);
+    virtual void handleTimeout(rfb::Timer* t);
     void longpressTimeout();
     void twoTouchTimeout();