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

[Chase Douglas]

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.

-- Chase