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 <[email protected]>
---
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
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
