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

[Chase Douglas]

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.

> 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.

-- Chase