Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ANSI colour support and terminal detection #131

Closed
wants to merge 1 commit into from

Conversation

nanoant
Copy link
Contributor

@nanoant nanoant commented Oct 22, 2012

Provides POSIX/UNIX counterpart for Windows colours implementation using ANSI escape codes. It also detects if stdout is a tty and disables colours when using pipes or file output.

Provides POSIX/UNIX counterpart for Windows colours implementation using ANSI escape codes. It also detects if stdout is a tty and disables colours when using pipes or file output.
@philsquared
Copy link
Collaborator

Just a quick note to let you know I'm not ignoring you.
Thanks so much for supplying this - I've been hoping someone would for some time now.

I'm hoping to have a look at it in the next week to try and get it integrated (it may not merge smoothly as is as I've been doing some wide-ranging refactoring).

@nanoant
Copy link
Contributor Author

nanoant commented Oct 29, 2012

Phil, no worries. If you prefer I can rebase & update these changes once you got yours pushed to master. This way we will be sure they actually work well after source base refactoring.

@philsquared
Copy link
Collaborator

Finally got to look at it this evening. It was actually very straightforward.
There was no clash with what I'd been doing - but I merged it manually anyway as I have guarded it by the CATCH_CONFIG_USE_POSIX_COLOUR_CODES identifier.

In fact if you define that it will use POSIX colour codes even on Windows (e.g. for targeting Cygwin).

Thanks again for contributing it. It's now committed on the Integration branch.

@nanoant
Copy link
Contributor Author

nanoant commented Oct 29, 2012

Well, if I can put my 2 cents. CATCH_CONFIG_USE_POSIX_COLOUR_CODES is undefined by default, right?
Can we make it defined by default on non-Windows platforms?

The reason being is that Windows always gets colours, and Unix not, which makes Catch inconsistent across platforms which is IMHO bad.

Please note isatty( fileno(stdout) ) check that ensures that colours actually only get emitted when using real terminal, not inside script or file output.

Btw. IMHO CATCH_CONFIG_USE_ANSI_COLOUR_CODES is more adequate name.

P.S. Can you have a look at my other patch (regarding Clang compiler flag misuse)?

@philsquared
Copy link
Collaborator

Yes, CATCH_CONFIG_USE_POSIX_COLOUR_CODES (now CATCH_CONFIG_USE_ANSI_COLOUR_CODES) is undefined by default, yes.
The reason is that I don't, yet, know how to unambiguously detect that they are available.
If you have a suggestion here I'll certainly consider it.

I might start making it the default for cases where I do know for sure (e.g. when running on a Mac) - however I also have a few other reservations:

  1. Developers on Unix-based platforms tend to be a lot more sensitive to tools using coloured terminal output by default.
  2. AFAICS there is no way to tell what the colour was before we started changing it - so we set it back to white when we're done - that may not be what it was - so we have hijacked the current colour.
  3. Although you can do so on Windows too I think it is more common for devs to change their terminal background colour (or set it to semi-transparent or even, in some cases, use a background image!)
  4. I believe the actual colours those codes map to can be remapped (certainly the Mac Terminal allows you do this).

In fact I've been thinking of making it not the default even on Windows (or least disable-able).

@nanoant
Copy link
Contributor Author

nanoant commented Oct 31, 2012

  1. Agreed, but colors do exists in Unix, i.e. Git, Vim, mc, GoogleTest, recent distro Linux bash commands, even clang compiler gives color output by default, anyway ANSI is a standard, so there's no way it won't work unless you are back in 70ties;) or not running tty, but this is what isatty check s for
  2. Nope, that's not true, ANSI escape 0m, one I used in the patch, resets the colour to default (whatever it is set by the user) that may be white but may be not, so there is no way we ruin console, otherwise other tools (above) would need to have magic capabilities, but they use same means as we do.
  3. All colour sequences are by design ought to be visible on both light and dark background, except when using explicit white and black (not used in the patch)
  4. Indeed all terminals, Mac, Linux, PuTTy colors such as red or green are nicknames for what is defined by the user.

The ultimate question is for how many users these colors will serve great value and how many will be annoyed by these.
P.S. Sorry for possible typos, responding from iPhone with tiny text size;)

@philsquared
Copy link
Collaborator

  1. Interesting. I don't tend to see colour output from any of the Bash tools I use in the OS X terminal (including Git). As for whether it will work what I meant was detecting that isatty() (or, more generally, <unistd.h>) is available. Obviously I am conditional on Windows, but I don't know if that is enough.
  2. That's useful to know - thanks. I'll change the respective comment back to reflect that ;-)
  3. Yes - they should be usable - but they may not reflect the intention (e.g. if you end up with red for passing test and green for failing tests it would get confusing). However I appreciate that's pretty unlikely - especially as the colour codes have nicknames that would make it obvious if you were doing that. I've not really played with that stuff much so just feeling my way around.
  4. See above

As for the "ultimate question" - for now the killer reason is (1) - making sure it doesn't break the compile on other platforms. Beyond that I'm still not sure how many people would get annoyed. I don't have first-hand experience but I've heard many times that it's a bit of a minefield.
Sounds like my other points (2 & 3) don't have a bearing.

Thanks for your feedback (and taking the time to type on an iPhone. I don't know about you but I'm finding it more difficult to type on my iPhone than it was five years ago - maybe I'm just getting old).

@nanoant nanoant deleted the ansi-colours branch April 22, 2014 15:57
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.

None yet

3 participants