Skip to content

Conversation

@sbinet
Copy link
Member

@sbinet sbinet commented Mar 27, 2015

Address issue #179 by translating the image to (0,-h) so (0,0) is at the
bottom-left corner for the X11 backend.

go test ./... will skip the X11 test if $DISPLAY is not defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No space after comma.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kortschak
Copy link
Member

I feel unsatisfied by the fact that the test is there but not run. It seems to me that a better approach would be to more heavily rely on vgimg (possibly extending it) to create the vgimg.Canvas and thereby trust the test for its creation in vg.

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

alternatively, I could try to have the travis-ci actually run the X11 test.

it seems to me like the approach you're advising would be more "testing vgimg" or "testing how xgbutil is using image" rather than actually testing how an X11 window is being displayed.

@sbinet sbinet force-pushed the issue-179-x11 branch 3 times, most recently from 461c3a9 to 69363f4 Compare March 28, 2015 09:04
@kortschak
Copy link
Member

kortschak commented Mar 28, 2015 via email

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

PTAL.

now the test is run if $DISPLAY is defined (which should work as a poor man's way to detect whether X11 is available. more refined ways could be devised if/when deemed necessary)

@kortschak
Copy link
Member

kortschak commented Mar 28, 2015 via email

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

you mean the !windows tag in the canvas_test.go file?
probably not.

@kortschak
Copy link
Member

kortschak commented Mar 28, 2015 via email

@kortschak
Copy link
Member

The !windows constraint does need to be there - not for us, but for people on Windows who do go test themselves.

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

the canvas_nox11.go file still needs to be there I think.
at least, when I tested on my linux+darwin machines with:
$ go test -v -tags=windows ./...
it would fail (w/o that canvas_nox11 file) complaining there were no source files for package vgx11 (paraphrasing.)

you are right though that even with the hasX11 function, the !windows build tag is still necessary (otherwise, compilation of the test binary would fail. d'oh!)

@kortschak
Copy link
Member

If there is only source for a non windows build environment the directory will be skipped when GOOS=windows.

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

ok. I wasn't indeed completely sure this wasn't an artifact of my unorthodox way of simulating a Windows environment.

@kortschak
Copy link
Member

Nope, you're right. That does not seem reasonable. But it is what it is.

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

ok. removed

@kortschak
Copy link
Member

I going to have a look at build and see why this is how it behaves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'd really like this to be "Package vgx11 implements X11 vg support." or words to that effect. The basis for the implementation is secondary and should not be in the first sentence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. (I was just following the other vg-foo backends descriptions)

@kortschak
Copy link
Member

I think it is a result of doing -tags=windows. When I change the build constraints to windows (without the bang), I get that error if the test is invoked in vgx11, but not if I invoke go test ./... in plot. I think we are good.

LGTM apart from minor comment. Please wait for @eaburns though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return os.Getenv("DISPLAY") != ""

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@eaburns
Copy link
Member

eaburns commented Mar 28, 2015

LGTM.
I assume that you two worked out all of the build issues.

@kortschak
Copy link
Member

I'm satisifed that it's correct.

@sbinet
Copy link
Member Author

sbinet commented Mar 28, 2015

I'll rebase, squash and make a nice commit message. (tomorrow)

- fixes gonum#179 for X11 too by translating the image to (0,-h) so (0,0)
  is at the bottom-left corner.
- 'go test ./...' will skip the X11 test if $DISPLAY is not defined.
- setup xvfb in travis-ci to test X11
@sbinet sbinet merged commit 4f1973d into gonum:master Mar 29, 2015
@sbinet sbinet deleted the issue-179-x11 branch March 29, 2015 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants