code review 186130: Tweaked the getDisplay function so, that the x11 packag...

Tweaked the getDisplay function so, that the x11 package works on OS X when x11 is installed.

[jhh, 2010-01-13 15:30:32]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao, 2010-01-14 12:28:59]

http://codereview.appspot.com/186130/diff/1003/1004
File src/pkg/exp/draw/x11/auth.go (right):

http://codereview.appspot.com/186130/diff/1003/1004#newcode14
src/pkg/exp/draw/x11/auth.go:14: // Reads the DISPLAY environment variable, and
returns ("12", "/tmp/foo/:") from "/tmp/foo/:12.0". If a path is missing (like
in ":12.0") the default "/tmp/.X11-unix/X" is returned.
It looks odd to me how the two return values are in opposite order from how they
appear in the full $DISPLAY.

What does a $DISPLAY look like on OS X? How can we distinguish it from something
like "localhost:0", for which the bit before the ':' is a host name, not a file
path? Can we look for a '/'?

Would a better API return (display, net, raddr string), the last two being the
right things to pass to net.Dial? For example, if $DISPLAY is ":12.0", it
returns "12", "unix", "/tmp/.X11-unix/X12".

[jhh, 2010-01-14 12:47:54]

http://codereview.appspot.com/186130/diff/1003/1004
File src/pkg/exp/draw/x11/auth.go (right):

http://codereview.appspot.com/186130/diff/1003/1004#newcode14
src/pkg/exp/draw/x11/auth.go:14: // Reads the DISPLAY environment variable, and
returns ("12", "/tmp/foo/:") from "/tmp/foo/:12.0". If a path is missing (like
in ":12.0") the default "/tmp/.X11-unix/X" is returned.
For me on OS X $DISPLAY is "/tmp/launch-2iR8Bm/:0" which is what I have to pass
to net.Dial. I found that out by trial and error, so I don't really have the
knowledge of how to interpret $DISPLAY.

So if $DISPLAY is like "host:0.0" what do we have to pass to net.Dial?

I agree on your commends and I'm happy to apply the changes once I understand
what to do :)

[jhh, 2010-01-14 21:45:30]

PTAL

[nigeltao, 2010-01-18 12:51:35]

http://codereview.appspot.com/186130/diff/1010/1011
File src/pkg/exp/draw/x11/auth.go (right):

http://codereview.appspot.com/186130/diff/1010/1011#newcode17
src/pkg/exp/draw/x11/auth.go:17: // If a path is missing (like in ":12.0") the
default "/tmp/.X11-unix/X" is used.
I think it would help if the doc comment had examples of the different DISPLAY
formats it could handle, and what the resultant (network, raddr, display) was.
For example:
:1.0
/tmp/launch-123456/:0
hostname:2.0

http://codereview.appspot.com/186130/diff/1010/1011#newcode26
src/pkg/exp/draw/x11/auth.go:26: if d[0] != ':' {
I think you can switch on d[0], and have a case for ':', a case for '/', and a
default case.

http://codereview.appspot.com/186130/diff/1010/1011#newcode79
src/pkg/exp/draw/x11/auth.go:79: func readAuthRetry(b []byte) (name, data
string, err os.Error) {
I don't think this retry loop belongs here. If the Xauthority cannot be found,
then the xgb library should simply let that bubble up to the app, and the app
can decide whether or not to retry, based on the os.Error.

Besides, you return early if readAuth fails, which is probably the opposite of
what you intended. If everything's fine, then you actually loop for 10 seconds
(and read the Xauthority file 10 times) unnecessarily.

But really, I'd take the retries out.

On a separate note, Go constants aren't named in ALL_CAPS unlike C
constants/macros. For examples, just grep src/pkg for "const".