Optin to VT100 style terminal codes on Windows #3004
Conversation
| public: | ||
| // Store the streambuf from cout up-front because | ||
| // cout may get redirected when running tests | ||
| CoutStream() : m_os( Catch::cout().rdbuf() ) {} | ||
| CoutStream() : m_os( Catch::cout().rdbuf() ) { | ||
| m_isatty = true; |
There was a problem hiding this comment.
Too much Copy&Paste for my taste - and since Catch::cout() can be reimplemented, this is quite possibly probing the wrong file handles...
|
|
||
| public: // IStream | ||
| std::ostream& stream() override { return m_os; } | ||
| bool isConsole() const override { return true; } | ||
| bool isConsole() const override { return m_isatty; } |
There was a problem hiding this comment.
This does not pass the tests. Now isConsole reports false in all of the unit tests when run in the CI - but its doing so rightfully.
There was a problem hiding this comment.
Two options - injecting a proper terminal emulator for testing or changing the expected results?
| m_isatty = m_isatty && enableVirtualTerminalSupport( STD_ERROR_HANDLE ); | ||
| #endif | ||
| #if defined( CATCH_PLATFORM_MAC ) || defined( CATCH_PLATFORM_IPHONE ) | ||
| m_isatty = m_isatty && !isDebuggerActive(); |
There was a problem hiding this comment.
Copy&Paste from original ANSIColourImpl code - unfortunately no comments, so what is this supposed to do? Is the debugger specifically not compatible with VT100 style sequences? Or does it not behave like a console in some other sense?
There was a problem hiding this comment.
Does it even apply to stderr or is stderr not usable with debuggers on Mac/Iphone?
| @@ -115,10 +114,13 @@ namespace { | |||
| } | |||
|
|
|||
| static bool useImplementationForStream(IStream const& stream) { | |||
There was a problem hiding this comment.
We might just kick this one out of the auto-selection. Even the version check here is wonky, because it's easily thrown off guard by manifests or compatibility mode.
Ext3h
force-pushed
the
ansi_colour_on_windows
branch
from
fd44c79 to
43f577c
Compare
| } | ||
|
|
||
| private: | ||
| void use( Colour::Code _colourCode ) const override { | ||
| auto setColour = [&out = | ||
| m_stream->stream()]( char const* escapeCode ) { | ||
| // The escape sequence must be flushed to console, otherwise |
There was a problem hiding this comment.
That comment is partially a lie. std::cout, std::cerr and std::cin are all tied together. But they are (by default) not chained up correctly!
Problem is that while std::cerr and std::cin can cause an (implicit) flush of std::cout, std::cin doesn't do so on std::cerr, and neither does std::cout imply of flush of std::cerr.
Then again - it doesn't matter, because the terminal shouldn't apply escape sequences received on stderr to stdin in the first place, so this appears to be entirely incorrect after all?
And there's also std::flush explicitly throughout the entire codebase wherever necessary.
There was a problem hiding this comment.
Meanwhile, there are no tests actually setting up a stream-buffering that would have verified the necessity / correctness of the flushing in the first place.
I.e.:
setvbuf(stdout, NULL, _IOFBF, 3072);
setvbuf(stderr, NULL, _IOFBF, 3072);That would quickly show where buffering is still broken.
The case where it does still break, is when configuring Catch2 to use std::cout as the output channel, while the code under test is utilizing std::cerr / stderr (but without explicit flushing and with output buffering explicitly enabled - which is NOT the default for stderr even when redirected!). But in that edge case, flushing std::cout still doesn't do anything about the un-flushed stderr.
Ext3h
force-pushed
the
ansi_colour_on_windows
branch
from
fd44c79 to
43f577c
Compare
Description
Enable VT100 terminal color codes on Windows too (if available) and use them by default.
GitHub Issues
Closes #3003