This repository has been archived by the owner on Mar 5, 2022. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 533
Smarter colorization and better support for native terminals on Windows #277
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add STD_ERROR_HANDLE = -12 and repeat this process. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this and it turns out setting the mode flag for stdout makes stderr VT100-aware too. I know little about Windows, I guess it's because the two streams are sharing the same tty device (lacking Windows-specific terminology)? Like on *nix,
tcsetattr(3)
and friends from termios apply to the tty device and are not specific to any stream.I can't think of case where someone would split stdout and stderr on two ttys, so we're probably good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The messages sent to stderr through printerr will contain bad sequences, just like the original problem was with stdout. I guess it should not make a difference on non-Windows, but it sure makes it usable on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zmwangx and @guilt, do you guys understand that all these low-level hacks we are pulling out of our sleeves is because of the fact that we are trying to support a terminal that doesn't understand escape sequences in 2019?
We had an option to disable colors completely and there are terminal emulators on Windows which understand color codes.
Here's my take - we have spent quality time on this one. We are still discussing what we need to do to make it work on a terminal the source of which is not available to us.
Can we revert this patch and take any of the following paths:
-C
option?-C
if he is seeing ANSI escape codes-C
to true by default on Windows as the probability is high the users may be using those terminals which are provided to them. And we document that to have colors on Windows one has to set C to false.Any of these would be much more elegant than stuff we are wasting our time on now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guilt
Yeah, I’m saying after setting ENABLE_VIRTUAL_TERMINAL_PROCESSING on stdout, stderr understands ANSI color too, at least on my machine. The prompt use reverse video which works alright in both cmd and PowerShell. Do you have a problem there? (I’m just using tcsetattr, which I understand well, as a model to guess why it works. Both fd 1 and fd 2 eventually points to /dev/pts/something which tsetattr works upon, and I assume it works similarly on Windows.)
I made sure this patch is strictly improvements without breaking anything that was working previously, so I don’t see why it needs be reverted. No color by default or always on warning are too heavy-handed and I myself as a ConEmu/Cmder user (when I’m using Windows, that is) won’t appreciate — color was working fine the first time I ported googler to Windows, that was what, two to three years ago? Adding a note to README is fine, it’s already full of things like this.
Also, consider this: Windows 7 EOL is near, Windows 8 is universally hated and admitting to using it in public is a no no ;), Windows 10 auto updates so probably no one is using a pre-1607 version, so this patch really covers almost the entire supported base of Windows users. The rest are like Python 3.3 users and who cares.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, on second thought I’ll set the flag for stderr too since it doesn’t really hurt... At most we set 7 | 4 = 7 which is a noop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually 7= ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING
The reason I suggested stderr is because of lines like:
googler/googler
Line 2103 in 1d3f70d
I'm glad you folks are taking this forward. Really appreciate it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLE_PROCESSED_OUTPUT and ENABLE_WRAP_AT_EOL_OUTPUT are the default, so when you get stdout console mode you get 3, you set it to 3 | 4 = 7; then when you get stderr console mode, which is the same console, you get 7, so setting it to 7 | 4 = 7 is a noop. Again, it doesn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#280.