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

[Chase Douglas]

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);

> +
> +    /**
>        * 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>