[PATCH xorg-gtest 09/16] environment: remove default settings

[Peter Hutterer]

On Tue, Jul 03, 2012 at 10:53:28AM -0700, Chase Douglas wrote:
> On 07/02/2012 11:44 PM, Peter Hutterer wrote:
> >Keep those in the server only, not the environment. And only override the
> >build-in ones when they've been set by main.
> >
> >Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >---
> >  src/environment.cpp |   22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> >diff --git a/src/environment.cpp b/src/environment.cpp
> >index b041236..01b2148 100644
> >--- a/src/environment.cpp
> >+++ b/src/environment.cpp
> >@@ -28,7 +28,6 @@
> >  #include "xorg/gtest/xorg-gtest-environment.h"
> >  #include "xorg/gtest/xorg-gtest-process.h"
> >  #include "xorg/gtest/xorg-gtest-xserver.h"
> >-#include "defines.h"
> >
> >  #include <sys/types.h>
> >  #include <unistd.h>
> >@@ -44,11 +43,8 @@
> >  #include <X11/Xlib.h>
> >
> >  struct xorg::testing::Environment::Private {
> >-  Private()
> >-      : path_to_conf(DUMMY_CONF_PATH), path_to_log_file(DEFAULT_XORG_LOGFILE),
> >-        path_to_server(DEFAULT_XORG_SERVER), display(DEFAULT_DISPLAY) {
> >+  Private() : display(-1) {
> >    }
> >-
> >    std::string path_to_conf;
> >    std::string path_to_log_file;
> >    std::string path_to_server;
> >@@ -103,15 +99,23 @@ int xorg::testing::Environment::display() const
> >  }
> >
> >  void xorg::testing::Environment::SetUp() {
> >+  unsigned int display_used;
> >    static char display_string[6];
> >    snprintf(display_string, 6, ":%d", d_->display);
> >
> >-  d_->server.SetDisplayNumber(d_->display);
> >-  d_->server.SetLogfilePath(d_->path_to_log_file);
> >-  d_->server.SetConfigPath(d_->path_to_conf);
> >-  d_->server.Start(d_->path_to_server);
> >+  if (d_->display >= 0)
> >+    d_->server.SetDisplayNumber(d_->display);
> >+  if (d_->path_to_log_file.length())
> >+    d_->server.SetLogfilePath(d_->path_to_log_file);
> >+  if (d_->path_to_conf.length())
> >+    d_->server.SetConfigPath(d_->path_to_conf);
> >+  if (d_->path_to_server.length())
> >+    display_used = d_->server.Start(d_->path_to_server);
> >+  else
> >+    display_used = d_->server.Start();
> >    d_->server.WaitForConnections();
> >
> >+  snprintf(display_string, 6, ":%d", display_used);
> >    Process::SetEnv("DISPLAY", display_string, true);
> >  }
> >
> >
> 
> Rather than store the values in two places (environment and server
> objects), we can simply remove the values from the environment and
> manipulate the server directly. Either:
> 
> * The environment has a method like:
> 
> XServer& Environment::XServer();
> 
> And then you modify things like:
> 
> environment.XServer().SetDisplayNumber(value);
> 
> * Provide pass through functions like:
> 
> void Environment::SetDisplayNumber(int value) {
>   d_->server.SetDisplayNumber(value);
> }
> 
> And then you modify things like:
> 
> environment.SetDisplayNumber(value);
> 
> The first approach is a bit easier to code (less boilerplate
> pass-through functions), but it violates the "Tell, don't ask"
> policy that many object-oriented APIs follow. I would prefer the
> second approach because I think it is easier for the end-user to
> follow and understand. They don't have to hunt around in multiple
> classes to do what they want to do.

works for me, but what is your need for preserving API atm? Environment and
XServer have two different naming conventions, I decided to stick with the
googletest CamelCase one, environment partially uses the latter.

I'm fine with replacing those and align them with XServer, but it may break
your other clients.

Cheers,
  Peter