Clean up the interface for VNCSConnectionST. Entry points are more apparent

and the data flow is now more strictly aimed towards this connection class.


git-svn-id: svn://svn.code.sf.net/p/tigervnc/code/trunk@4771 3789f03b-4d11-0410-bbf8-ca57d06f2519

8 files changed, 103 insertions, 97 deletions

diff --git a/common/rfb/SDesktop.h b/common/rfb/SDesktop.h
index 4a53d538..57ceb076 100644
--- a/common/rfb/SDesktop.h
+++ b/common/rfb/SDesktop.h
@@ -64,13 +64,6 @@ namespace rfb {
 
     virtual void stop() {}
 
-    // framebufferUpdateRequest() is called to let the desktop know that at
-    // least one client has become ready for an update.  Desktops can check
-    // whether there are clients ready at any time by calling the VNCServer's
-    // clientsReadyForUpdate() method.
-
-    virtual void framebufferUpdateRequest() {}
-
     // getFbSize() returns the current dimensions of the framebuffer.
     // This can be called even while the SDesktop is not start()ed.
 
diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx
index 1478ba5d..84fc2ca1 100644
--- a/common/rfb/VNCSConnectionST.cxx
+++ b/common/rfb/VNCSConnectionST.cxx
@@ -1,5 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2009 Pierre Ossman for Cendio AB
+ * Copyright 2009-2011 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
@@ -34,7 +34,7 @@ static LogWriter vlog("VNCSConnST");
 
 VNCSConnectionST::VNCSConnectionST(VNCServerST* server_, network::Socket *s,
                                    bool reverse)
-  : SConnection(reverse), sock(s), server(server_),
+  : SConnection(reverse), sock(s), inProcessMessages(false), server(server_),
     updates(false), image_getter(server->useEconomicTranslate),
     drawRenderedCursor(false), removeRenderedCursor(false),
     updateTimer(this), pointerEventTime(0),
@@ -111,22 +111,19 @@ void VNCSConnectionST::processMessages()
   try {
     // - Now set appropriate socket timeouts and process data
     setSocketTimeouts();
-    bool clientsReadyBefore = server->clientsReadyForUpdate();
+
+    inProcessMessages = true;
 
     while (getInStream()->checkNoWait(1)) {
       processMsg();
     }
 
-    // If there were update requests, try to send a framebuffer update.
-    // We don't send updates immediately on requests as this way, we
-    // give higher priority to user actions such as keyboard and
-    // pointer events.
-    if (!requested.is_empty()) {
-      writeFramebufferUpdate();
-    }
+    inProcessMessages = false;
 
-    if (!clientsReadyBefore && !requested.is_empty())
-      server->desktop->framebufferUpdateRequest();
+    // If there were anything requiring an update, try to send it here.
+    // We wait until now with this to aggregate responses and to give 
+    // higher priority to user actions such as keyboard and pointer events.
+    writeFramebufferUpdate();
   } catch (rdr::EndOfStream&) {
     close("Clean disconnection");
   } catch (rdr::Exception &e) {
@@ -134,15 +131,6 @@ void VNCSConnectionST::processMessages()
   }
 }
 
-void VNCSConnectionST::writeFramebufferUpdateOrClose()
-{
-  try {
-    writeFramebufferUpdate();
-  } catch(rdr::Exception &e) {
-    close(e.str());
-  }
-}
-
 void VNCSConnectionST::pixelBufferChange()
 {
   try {
@@ -186,27 +174,25 @@ void VNCSConnectionST::pixelBufferChange()
     updates.add_changed(server->pb->getRect());
     vlog.debug("pixel buffer changed - re-initialising image getter");
     image_getter.init(server->pb, cp.pf(), writer());
-    if (writer()->needFakeUpdate())
-      writeFramebufferUpdate();
+    writeFramebufferUpdate();
   } catch(rdr::Exception &e) {
     close(e.str());
   }
 }
 
-void VNCSConnectionST::screenLayoutChange(rdr::U16 reason)
+void VNCSConnectionST::writeFramebufferUpdateOrClose()
 {
   try {
-    if (!authenticated())
-      return;
-
-    cp.screenLayout = server->screenLayout;
-    if (state() == RFBSTATE_NORMAL) {
-      writer()->writeExtendedDesktopSize(reason, 0, cp.width, cp.height,
-                                         cp.screenLayout);
-    }
+    writeFramebufferUpdate();
+  } catch(rdr::Exception &e) {
+    close(e.str());
+  }
+}
 
-    if (writer()->needFakeUpdate())
-      writeFramebufferUpdate();
+void VNCSConnectionST::screenLayoutChangeOrClose(rdr::U16 reason)
+{
+  try {
+    screenLayoutChange(reason);
   } catch(rdr::Exception &e) {
     close(e.str());
   }
@@ -221,7 +207,7 @@ void VNCSConnectionST::setColourMapEntriesOrClose(int firstColour,int nColours)
   }
 }
 
-void VNCSConnectionST::bell()
+void VNCSConnectionST::bellOrClose()
 {
   try {
     if (state() == RFBSTATE_NORMAL) writer()->writeBell();
@@ -230,7 +216,7 @@ void VNCSConnectionST::bell()
   }
 }
 
-void VNCSConnectionST::serverCutText(const char *str, int len)
+void VNCSConnectionST::serverCutTextOrClose(const char *str, int len)
 {
   try {
     if (!(accessRights & AccessCutText)) return;
@@ -243,15 +229,10 @@ void VNCSConnectionST::serverCutText(const char *str, int len)
 }
 
 
-void VNCSConnectionST::setDesktopName(const char *name)
+void VNCSConnectionST::setDesktopNameOrClose(const char *name)
 {
-  cp.setName(name);
   try {
-    if (state() == RFBSTATE_NORMAL) {
-      if (!writer()->writeSetDesktopName()) {
-	fprintf(stderr, "Client does not support desktop rename\n");
-      }
-    }
+    setDesktopName(name);
   } catch(rdr::Exception& e) {
     close(e.str());
   }
@@ -547,8 +528,7 @@ void VNCSConnectionST::setDesktopSize(int fb_width, int fb_height,
   if (!layout.validate(fb_width, fb_height)) {
     writer()->writeExtendedDesktopSize(reasonClient, resultInvalid,
                                        fb_width, fb_height, layout);
-    if (writer()->needFakeUpdate())
-      writeFramebufferUpdate();
+    writeFramebufferUpdate();
     return;
   }
 
@@ -557,18 +537,19 @@ void VNCSConnectionST::setDesktopSize(int fb_width, int fb_height,
   // protocol-wise, but unnecessary.
   result = server->desktop->setScreenLayout(fb_width, fb_height, layout);
 
-  // Always send back a reply to the requesting client
   writer()->writeExtendedDesktopSize(reasonClient, result,
                                      fb_width, fb_height, layout);
-  if (writer()->needFakeUpdate())
-    writeFramebufferUpdate();
 
-  // But only notify other clients on success
+  // Only notify other clients on success
   if (result == resultSuccess) {
     if (server->screenLayout != layout)
         throw Exception("Desktop configured a different screen layout than requested");
     server->notifyScreenLayoutChange(this);
   }
+
+  // but always send back a reply to the requesting client
+  // (do this last as it might throw an exception on socket errors)
+  writeFramebufferUpdate();
 }
 
 void VNCSConnectionST::setInitialColourMap()
@@ -627,8 +608,12 @@ void VNCSConnectionST::writeSetCursorCallback()
 
 bool VNCSConnectionST::handleTimeout(Timer* t)
 {
-  if (t == &updateTimer)
-    writeFramebufferUpdateOrClose();
+  try {
+    if (t == &updateTimer)
+      writeFramebufferUpdate();
+  } catch (rdr::Exception& e) {
+    close(e.str());
+  }
 
   return false;
 }
@@ -647,6 +632,12 @@ void VNCSConnectionST::writeFramebufferUpdate()
 {
   updateTimer.stop();
 
+  // We try to aggregate responses, so don't send out anything whilst we
+  // still have incoming messages. processMessages() will give us another
+  // chance to run once things are idle.
+  if (inProcessMessages)
+    return;
+
   if (state() != RFBSTATE_NORMAL || requested.is_empty())
     return;
 
@@ -788,15 +779,33 @@ void VNCSConnectionST::writeRenderedCursorRect()
   drawRenderedCursor = false;
 }
 
+void VNCSConnectionST::screenLayoutChange(rdr::U16 reason)
+{
+  if (!authenticated())
+    return;
+
+  cp.screenLayout = server->screenLayout;
+
+  if (state() != RFBSTATE_NORMAL)
+    return;
+
+  writer()->writeExtendedDesktopSize(reason, 0, cp.width, cp.height,
+                                     cp.screenLayout);
+  writeFramebufferUpdate();
+}
+
 void VNCSConnectionST::setColourMapEntries(int firstColour, int nColours)
 {
-  if (!readyForSetColourMapEntries) return;
-  if (server->pb->getPF().trueColour) return;
+  if (!readyForSetColourMapEntries)
+    return;
+  if (server->pb->getPF().trueColour)
+    return;
 
   image_getter.setColourMapEntries(firstColour, nColours);
 
   if (cp.pf().trueColour) {
     updates.add_changed(server->pb->getRect());
+    writeFramebufferUpdate();
   }
 }
 
@@ -807,10 +816,28 @@ void VNCSConnectionST::setColourMapEntries(int firstColour, int nColours)
 
 void VNCSConnectionST::setCursor()
 {
-  if (state() != RFBSTATE_NORMAL || !cp.supportsLocalCursor) return;
+  if (state() != RFBSTATE_NORMAL)
+    return;
+  if (!cp.supportsLocalCursor)
+    return;
+
   writer()->cursorChange(this);
-  if (writer()->needFakeUpdate())
-    writeFramebufferUpdate();
+  writeFramebufferUpdate();
+}
+
+void VNCSConnectionST::setDesktopName(const char *name)
+{
+  cp.setName(name);
+
+  if (state() != RFBSTATE_NORMAL)
+    return;
+
+  if (!writer()->writeSetDesktopName()) {
+    fprintf(stderr, "Client does not support desktop rename\n");
+    return;
+  }
+
+  writeFramebufferUpdate();
 }
 
 void VNCSConnectionST::setSocketTimeouts()
diff --git a/common/rfb/VNCSConnectionST.h b/common/rfb/VNCSConnectionST.h
index abf43838..5a007ee9 100644
--- a/common/rfb/VNCSConnectionST.h
+++ b/common/rfb/VNCSConnectionST.h
@@ -1,5 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2009 Pierre Ossman for Cendio AB
+ * Copyright 2009-2011 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
@@ -63,14 +63,17 @@ namespace rfb {
     // Socket if an error occurs, via the close() call.
     void processMessages();
 
-    void writeFramebufferUpdateOrClose();
+    // Called when the underlying pixelbuffer is resized or replaced.
     void pixelBufferChange();
-    void screenLayoutChange(rdr::U16 reason);
+
+    // Wrappers to make these methods "safe" for VNCServerST.
+    void writeFramebufferUpdateOrClose();
+    void screenLayoutChangeOrClose(rdr::U16 reason);
     void setColourMapEntriesOrClose(int firstColour, int nColours);
-    void bell();
-    void serverCutText(const char *str, int len);
-    void setDesktopName(const char *name);
     void setCursorOrClose();
+    void bellOrClose();
+    void serverCutTextOrClose(const char *str, int len);
+    void setDesktopNameOrClose(const char *name);
 
     // checkIdleTimeout() returns the number of milliseconds left until the
     // idle timeout expires.  If it has expired, the connection is closed and
@@ -92,7 +95,6 @@ namespace rfb {
     bool needRenderedCursor();
 
     network::Socket* getSock() { return sock; }
-    bool readyForUpdate() { return !requested.is_empty(); }
     void add_changed(const Region& region) { updates.add_changed(region); }
     void add_copied(const Region& dest, const Point& delta) {
       updates.add_copied(dest, delta);
@@ -155,12 +157,17 @@ namespace rfb {
     void writeFramebufferUpdate();
 
     void writeRenderedCursorRect();
+    void screenLayoutChange(rdr::U16 reason);
     void setColourMapEntries(int firstColour, int nColours);
     void setCursor();
+    void setDesktopName(const char *name);
     void setSocketTimeouts();
 
     network::Socket* sock;
     CharArray peerEndpoint;
+
+    bool inProcessMessages;
+
     VNCServerST* server;
     SimpleUpdateTracker updates;
     TransImageGetter image_getter;
diff --git a/common/rfb/VNCServer.h b/common/rfb/VNCServer.h
index 7fa44c18..1c8d38e0 100644
--- a/common/rfb/VNCServer.h
+++ b/common/rfb/VNCServer.h
@@ -58,10 +58,6 @@ namespace rfb {
     // bell() tells the server that it should make all clients make a bell sound.
     virtual void bell() = 0;
 
-    // clientsReadyForUpdate() returns true if there is at least one client
-    // waiting for an update, false if no clients are ready.
-    virtual bool clientsReadyForUpdate() = 0;
-
     // - Close all currently-connected clients, by calling
     //   their close() method with the supplied reason.
     virtual void closeClients(const char* reason) = 0;
diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx
index 32be8625..eea6565d 100644
--- a/common/rfb/VNCServerST.cxx
+++ b/common/rfb/VNCServerST.cxx
@@ -330,7 +330,7 @@ void VNCServerST::setScreenLayout(const ScreenSet& layout)
   std::list<VNCSConnectionST*>::iterator ci, ci_next;
   for (ci=clients.begin();ci!=clients.end();ci=ci_next) {
     ci_next = ci; ci_next++;
-    (*ci)->screenLayoutChange(reasonServer);
+    (*ci)->screenLayoutChangeOrClose(reasonServer);
   }
 }
 
@@ -348,7 +348,7 @@ void VNCServerST::bell()
   std::list<VNCSConnectionST*>::iterator ci, ci_next;
   for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
     ci_next = ci; ci_next++;
-    (*ci)->bell();
+    (*ci)->bellOrClose();
   }
 }
 
@@ -357,7 +357,7 @@ void VNCServerST::serverCutText(const char* str, int len)
   std::list<VNCSConnectionST*>::iterator ci, ci_next;
   for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
     ci_next = ci; ci_next++;
-    (*ci)->serverCutText(str, len);
+    (*ci)->serverCutTextOrClose(str, len);
   }
 }
 
@@ -367,7 +367,7 @@ void VNCServerST::setName(const char* name_)
   std::list<VNCSConnectionST*>::iterator ci, ci_next;
   for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
     ci_next = ci; ci_next++;
-    (*ci)->setDesktopName(name_);
+    (*ci)->setDesktopNameOrClose(name_);
   }
 }
 
@@ -385,16 +385,6 @@ void VNCServerST::add_copied(const Region& dest, const Point& delta)
   }
 }
 
-bool VNCServerST::clientsReadyForUpdate()
-{
-  std::list<VNCSConnectionST*>::iterator ci;
-  for (ci = clients.begin(); ci != clients.end(); ci++) {
-    if ((*ci)->readyForUpdate())
-      return true;
-  }
-  return false;
-}
-
 void VNCServerST::tryUpdate()
 {
   std::list<VNCSConnectionST*>::iterator ci, ci_next;
@@ -613,6 +603,6 @@ void VNCServerST::notifyScreenLayoutChange(VNCSConnectionST* requester)
     ci_next = ci; ci_next++;
     if ((*ci) == requester)
       continue;
-    (*ci)->screenLayoutChange(reasonOtherClient);
+    (*ci)->screenLayoutChangeOrClose(reasonOtherClient);
   }
 }
diff --git a/common/rfb/VNCServerST.h b/common/rfb/VNCServerST.h
index aa9ade0f..c74be903 100644
--- a/common/rfb/VNCServerST.h
+++ b/common/rfb/VNCServerST.h
@@ -85,7 +85,6 @@ namespace rfb {
     virtual void serverCutText(const char* str, int len);
     virtual void add_changed(const Region &region);
     virtual void add_copied(const Region &dest, const Point &delta);
-    virtual bool clientsReadyForUpdate();
     virtual void tryUpdate();
     virtual void setCursor(int width, int height, const Point& hotspot,
                            void* cursorData, void* mask);
diff --git a/win/rfb_win32/SDisplay.cxx b/win/rfb_win32/SDisplay.cxx
index f14b49f1..05ddce34 100644
--- a/win/rfb_win32/SDisplay.cxx
+++ b/win/rfb_win32/SDisplay.cxx
@@ -316,11 +316,6 @@ void SDisplay::clientCutText(const char* text, int len) {
 }
 
 
-void SDisplay::framebufferUpdateRequest()
-{
-  SetEvent(updateEvent);
-}
-
 Point SDisplay::getFbSize() {
   bool startAndStop = !core;
 
@@ -382,7 +377,7 @@ SDisplay::processEvent(HANDLE event) {
     inputs->blockInputs(disableLocalInputs);
 
     // - Only process updates if the server is ready
-    if (server && server->clientsReadyForUpdate()) {
+    if (server) {
       bool try_update = false;
 
       // - Check that the SDesktop doesn't need restarting
diff --git a/win/rfb_win32/SDisplay.h b/win/rfb_win32/SDisplay.h
index 6dbb50a5..6aac59ae 100644
--- a/win/rfb_win32/SDisplay.h
+++ b/win/rfb_win32/SDisplay.h
@@ -68,7 +68,6 @@ namespace rfb {
       virtual void pointerEvent(const Point& pos, int buttonmask);
       virtual void keyEvent(rdr::U32 key, bool down);
       virtual void clientCutText(const char* str, int len);
-      virtual void framebufferUpdateRequest();
       virtual Point getFbSize();
 
       // -=- Clipboard