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

Support for colored output in Windows shell #21

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
2 participants
@mdietsch
Contributor

mdietsch commented Mar 11, 2016

Things work a little different on Windows ;)

Sorry for the bad commit message but I github ignored the entered text when committing from online editor :(

@mdietsch

This comment has been minimized.

Contributor

mdietsch commented Mar 11, 2016

The related tests do currently fail. I am not sure how to handle these... :(

Is color information in stringstreams even reasonable on Windows?

@@ -59,13 +108,16 @@ template<Debug::Color c> Debug::Modifier Debug::colorInternal() {
if(!noSpaceBefore) debug << Debug::nospace;
debug << code;
if(noSpaceBefore) debug << Debug::nospace;

This comment has been minimized.

@mosra

mosra Mar 12, 2016

Owner

I think putting space at proper place before/after space is important also for the Windows implementation to avoid visual artifacts.

Similarly for the bold implementation below.

This comment has been minimized.

@mdietsch

mdietsch Mar 14, 2016

Contributor

You are just backing up restoring the state of InternalFlag::NoSpaceBeforeNextValue which is getting reset in debug << code.
In the case of CORRADE_TARGET_WINDOWS this flag does not get touched and therefor does not need this backup/restore code.

This comment has been minimized.

@mosra

mosra Mar 14, 2016

Owner

Yup, you're right, sorry.

const bool noSpaceBefore = !!(debug._flags & InternalFlag::NoSpaceBeforeNextValue);
if(!noSpaceBefore) debug << Debug::nospace;
debug << "\033[0m";
if(noSpaceBefore) debug << Debug::nospace;
#endif

This comment has been minimized.

@mosra

mosra Mar 12, 2016

Owner

This looks like resetting color back to default is not working on Windows at all. Or am I overlooking something?

This comment has been minimized.

@mdietsch

mdietsch Mar 14, 2016

Contributor

Resetting is not implemented in Win32 API headers. I would have to store the previous state somewhere. I'll have a look into this.

/* Reset output color */
if(_flags & InternalFlag::ColorWritten)
*_output << "\033[0m";
#endif

This comment has been minimized.

@mosra

mosra Mar 12, 2016

Owner

Color resetting implicitly on Debug destruction (end of a line, for example) should work on Windows too. I think these are crucial features to avoid leaking state (broken console colors for subsequent commands).

This comment has been minimized.

@mdietsch

mdietsch Mar 14, 2016

Contributor

Same as above.

@@ -42,6 +49,46 @@ template<> inline void toStream<Implementation::DebugOstreamFallback>(std::ostre
value.apply(s);
}
#ifdef CORRADE_TARGET_WINDOWS
HANDLE getStdHandle(const std::ostream* s) {
if(!s) return INVALID_HANDLE_VALUE;

This comment has been minimized.

@mosra

mosra Mar 12, 2016

Owner

This line is not needed I think (no pointer dereferencing is done later so it's safe).

This comment has been minimized.

@mdietsch

mdietsch Mar 14, 2016

Contributor

You're right. The null-pointer case is implictitly handled. But sometimes I prefer expliciticity ;)

case Debug::Color::Magenta: return FOREGROUND_RED|FOREGROUND_BLUE;
case Debug::Color::Cyan: return FOREGROUND_GREEN|FOREGROUND_BLUE;
case Debug::Color::White: return FOREGROUND_RED|FOREGROUND_GREEN|FOREGROUND_BLUE;
case Debug::Color::Default: return 0;

This comment has been minimized.

@mosra

mosra Mar 12, 2016

Owner

I think it would be better to have these values directly in the enum definition instead of doing a runtime switch. But I don't like having winapi includes in the public headers due to namespace pollution so it would need to be done without including the header (copypasting the corresponding #defines from the winapi header, for example).

This comment has been minimized.

@mdietsch

mdietsch Mar 14, 2016

Contributor

This is not really a runtime switch. c is a template parameter and the compiler can optimize this switch easily to a single return statement.

I don't like copy&pasting from platform headers if not really required.

@mosra

This comment has been minimized.

Owner

mosra commented Mar 12, 2016

Hi, thank you a lot for your contribution!

I was reading about ANSI on Wikipedia and saw that DOS and pre-NT Windows supported ANSI escape codes so I thought this would be valid also for current Windows versions... I should have been reading more carefully, because

The Win32 console did not support ANSI escape sequences at all until Windows 10 "Threshold 2".

In fact, I had tested it on AppVeyor CI and it miraculously worked there (in contrast to the Win32 console) so I didn't expect any problem elsewhere :( However, it seems that AppVeyor doesn't and won't support the "proper" way (they say it's way too deep in Windows so it's impossible to capture the color info) so with your fix the colors are there no more :( It would be wonderful to have colors both on AppVeyor and normal Win32 console, but I guess that's kinda impossible.

The related tests do currently fail. I am not sure how to handle these... :(

Is color information in stringstreams even reasonable on Windows?

The test was there just to verify that I'm putting the ANSI escape codes at proper places. From the look of it, I assume there is no way to do an unit test for the Windows implementation?

@mdietsch

This comment has been minimized.

Contributor

mdietsch commented Mar 14, 2016

Yes, this whole Windows console color thing seems like a mess. :(
I also stumbled upon ansicon. But this whole DLL injection thing seems a bit too hacky for this very case here.

But nevertheless it seems that there are some consoles on Windows which support coloring with ANSI escape codes. So it would probably be better to not mutually exclude ANSI coloring and Win32.

One possible solution could be to always use Win32 API on windows and ANSI escape sequences dependant on the DisableColors flag. Then this flag should get renamed to DisableAnsiColors or something similar.

Or is there any other solution you would prefer?

@mosra

This comment has been minimized.

Owner

mosra commented Mar 14, 2016

(Sorry, got interrupted and wasn't able to write everything at once.)

Hm... thinking about this, the coloring will work only when the program prints directly to the console without anything in betweeen. Which makes it all a lot less useful, because for example when I run unit tests via ctest, they get piped through some additional stream and thus there is no possibility to affect the console color anymore. And that was like my main use case for having colored output.

:(

If you can then please have a look into resetting the colors into the previous state and I'll merge this. I will make the ANSI/WINAPI switch a compile-time option later so the user can select between two options with different levels of sanity. Having a runtime switch or DLL injection seems too hacky, yeah.

Also, just a coding style nitpick, can you please indent the preprocessor #ifdefs the same as the surrounding code? Thank you.

Eberhard Hackl
Support for coloring in Win32 console
* Remove unneeded null-pointer check
* Always reset console color to previous state
* Indent pre-processor directives like the surrounding code
* Expect the related tests to fail on Windows
@mdietsch

This comment has been minimized.

Contributor

mdietsch commented Mar 15, 2016

I did the requested changes.

But the more I think about this topic the more I think it would be a good idea to have both coloring methods (Win32 and ANSI codes) on Windows...

@mosra

This comment has been minimized.

Owner

mosra commented Mar 19, 2016

Squashed into one commit and merged in adbc617, thanks for the contribution!

Switching between Win32 and ANSI colors is now possible with UTILITY_USE_ANSI_COLORS CMake option (defaults to OFF).

@mosra mosra closed this Mar 19, 2016

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment