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

[Peter Hutterer]

On Thu, Jul 05, 2012 at 02:17:30PM -0700, Chase Douglas wrote:
> On 07/04/2012 04:36 AM, Peter Hutterer wrote:
> >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.
> 
> The standard Google style guide says to use lowercase_underscore for
> variable and member names, and then to use set_variable() and
> variable() for setters and getters. This is one of my least favorite
> aspects of their style guide, but I tend to just try to conform
> rather than do my own thing, so I stuck with it.
> 
> Things obviously get murkier when your getter and setter aren't
> actually getting and setting direct members, but maybe are passing
> the value on to another object's getter or setter.

yes, this is just weird. I just looked at examples and they all had
CamelCase. For reference, the style guide is here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming_Rules

But here's me thinking the whole point of setters and getters is that you
don't have to care what variable is actually set (e.g.  SetDisplayNumber()
vs SetDisplayString(), we may change the backend storage).

> >I'm fine with replacing those and align them with XServer, but it may break
> >your other clients.
> 
> If we want to continue using the Google style, then we should follow
> its spirit and use set_display_number(), for example. If we want to
> break with it for getters and setters, then lets do it now and
> define new methods for the old ones, and call the new methods from
> them. I'm about 60-40 on the decision to keep with the current
> format, just because it's easier not to have a transition :). But I
> won't throw a fight if you want to change it.

I'd rather stick to CamelCaseOnly. Inconsistent function naming is bad
enough already, even worse so when encouraged by the style guide. I'll write
the wrappers.

Cheers,
  Peter