[PATCH xorg-gtest v2 2/8] xserver: store config, logfile, binary paths in the XServer object

[Peter Hutterer]

On Wed, Jul 11, 2012 at 11:45:59AM -0700, Chase Douglas wrote:
> On 07/10/2012 08:28 PM, Peter Hutterer wrote:
> >And provide a SetOption call for the various commandline options that the
> >server may take.
> >
> >Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >---
> >Changes to v1:
> >- drop separate SetConfigPath(), SetLogfilePath()
> >
> >  include/xorg/gtest/xorg-gtest-xserver.h |   16 ++++++++++++++++
> >  src/environment.cpp                     |    2 ++
> >  src/xserver.cpp                         |   27 +++++++++++++++++++++++++++
> >  3 files changed, 45 insertions(+)
> >
> >diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
> >index 56dacf6..58fdc53 100644
> >--- a/include/xorg/gtest/xorg-gtest-xserver.h
> >+++ b/include/xorg/gtest/xorg-gtest-xserver.h
> >@@ -56,6 +56,11 @@ class XServer : public xorg::testing::Process {
> >      void SetDisplayNumber(unsigned int display_number);
> >
> >      /**
> >+     * @param [in] path_to_server The path to the binary
> >+     */
> 
> Missing method documentation.
> 
> >+    void SetServerPath(std::string &path_to_server);
> 
> The argument should be const.
> 
> >+
> >+    /**
> >       * Get the display number from this server. If the server was not
> >       * started yet, this function returns the display number the server will
> >       * be started on.
> >@@ -73,6 +78,17 @@ class XServer : public xorg::testing::Process {
> >      const std::string& GetDisplayString(void);
> >
> >      /**
> >+     * Set startup options for the server.
> >+     *
> >+     * For arguments that do not take/need a value, use the empty string as
> >+     * value.
> >+     *
> >+     * @param [in] key Commandline option
> >+     * @param [in] value Option value (if any)
> >+     */
> >+    void SetOption(std::string key, std::string value);
> 
> Pass in a const reference in order to make sure we aren't
> unnecessarily copying a string just to get it into the method:
> 
> void SetOption(const std::string& key, const std::string& value);

hah. the const did the trick, thanks. I had tried std::string& before but
then you can't just pass in a compile-time string, so the call to
option["-logfile"] = "foo" didn't work.

Cheers,
  Peter

> >+
> >+    /**
> >       * Wait for a specific device to be added to the server.
> >       *
> >       * @param [in] display The X display connection
> >diff --git a/src/environment.cpp b/src/environment.cpp
> >index 9a524ab..2f1c4e2 100644
> >--- a/src/environment.cpp
> >+++ b/src/environment.cpp
> >@@ -142,6 +142,8 @@ void xorg::testing::Environment::SetUp() {
> >    }
> >
> >    d_->server.SetDisplayNumber(d_->display);
> >+  d_->server.SetOption("-logfile", d_->path_to_log_file);
> >+  d_->server.SetOption("-config", d_->path_to_conf);
> >    d_->server.Start(d_->path_to_server, d_->path_to_server.c_str(),
> >                      display_string,
> >                      "-logverbose", "10",
> >diff --git a/src/xserver.cpp b/src/xserver.cpp
> >index c24cee0..f36bd10 100644
> >--- a/src/xserver.cpp
> >+++ b/src/xserver.cpp
> >@@ -27,6 +27,7 @@
> >   ******************************************************************************/
> >
> >  #include "xorg/gtest/xorg-gtest-xserver.h"
> >+#include "defines.h"
> >
> >  #include <sys/types.h>
> >  #include <sys/wait.h>
> >@@ -40,12 +41,30 @@
> >  #include <cstring>
> >  #include <stdexcept>
> >  #include <vector>
> >+#include <map>
> >
> >  #include <X11/extensions/XInput2.h>
> >
> >  struct xorg::testing::XServer::Private {
> >+  Private()
> >+      : display_number(DEFAULT_DISPLAY),
> >+        display_string(),
> 
> This ^^ isn't needed. The default constructor will be called if
> unspecified, which initializes to an empty string.
> 
> >+        path_to_server(DEFAULT_XORG_SERVER) {
> >+
> >+    std::string key, value;
> >+    key = std::string("-config");
> >+    value = std::string(DUMMY_CONF_PATH);
> >+    options[key] = value;
> >+
> >+    key = std::string("-logfile");
> >+    value = std::string(DEFAULT_XORG_LOGFILE);
> >+    options[key] = value;
> 
> The above can be simplified down to:
> 
> options["-config"] = DUMMY_CONF_PATH;
> options["-logfile"] = DEFAULT_XORG_LOGFILE;
> 
> >+  }
> >+
> >    unsigned int display_number;
> >    std::string display_string;
> >+  std::string path_to_server;
> >+  std::map<std::string, std::string> options;
> >  };
> >
> >  xorg::testing::XServer::XServer() : d_(new Private) {
> >@@ -68,6 +87,10 @@ const std::string& xorg::testing::XServer::GetDisplayString(void) {
> >    return d_->display_string;
> >  }
> >
> >+void xorg::testing::XServer::SetServerPath(std::string &path_to_server) {
> >+  d_->path_to_server = path_to_server;
> >+}
> >+
> >  bool xorg::testing::XServer::WaitForEvent(::Display *display, time_t timeout)
> >  {
> >      fd_set fds;
> >@@ -194,3 +217,7 @@ bool xorg::testing::XServer::WaitForDevice(::Display *display, const std::string
> >
> >      return false;
> >  }
> >+
> >+void xorg::testing::XServer::SetOption(std::string key, std::string value) {
> >+  d_->options[key] = value;
> >+}
> >
> 
> With the above fixes:
> 
> Reviewed-by: Chase Douglas <chase.douglas at canonical.com>