Add color support for Windows consoles #2804
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Note that this PR removes support for terminfo consoles on Windows. Ideally what this should do is first check for Windows console support, and then if that fails try terminfo, and only if that also fails fallback to no color. Also note that regardless of how this code is written, it is still fundamentally flawed due to Stebalien/term#62 but hopefully still an improvement over what we have currently. |
| }, | ||
| // This should never happens, since we checked that the creation | ||
| // would succeed above. | ||
| Err(e) => unreachable!("Could not create WinConsole: {:?}", e), |
There was a problem hiding this comment.
Could this punt the error upwards as even though the first one succeeded the second one may still have a small window where it could fail?
There was a problem hiding this comment.
That small window being someone calls FreeConsole between the first and second call to WinConsole::new. I can't think of any other way only the second call could fail.
There was a problem hiding this comment.
It is easy enough though - and If I replace the unreachable! with Shell::get_terminfo_term the previous check should be obsolete, it seems more elegant that way to me, actually.
Edit - after checking with @retep998, the problem is that creating a WinConsole consumes the writer - making anything but an error impossible. I will make the functions return a Result which, however opens up the question where the error can ultimately get handled properly
|
Thanks @skeleten! |
|
I added a commit - Letting the errors bubble up and added a |
| @@ -147,27 +147,61 @@ impl MultiShell { | |||
| } | |||
|
|
|||
| impl Shell { | |||
| pub fn create(out: Box<Write + Send>, config: ShellConfig) -> Shell { | |||
| pub fn create(out: Box<Write + Send>, config: ShellConfig) -> CargoResult<Shell> { | |||
There was a problem hiding this comment.
This is only ever really called in this one crate, so perhaps the logic of create_fallback could be folded into create itself?
That is, could we remove create_fallback?
There was a problem hiding this comment.
The problem there is, that the out is consumed - It'd needed to be newly created by the caller. As such I'm not really sure if that's at all possible.
If Rust were to support default arguments, it could be easily be one function, but without them I do not really see how at the moment.
Edit: I suppose the flag could be added the ShellConfig. Thoughts on that?
There was a problem hiding this comment.
Perhaps this could take FnMut() -> Box<Write + Send> and just call that multiple times if necessary? The actual handles are just stdout/stderr so they can be manufactured multiple tiems.
There was a problem hiding this comment.
That sounds like a great idea! I'll be rewriting it in such a manner straight away :)
|
Looks good to me, thanks! Can you squash the commits as well? |
|
📌 Commit 6edf535 has been approved by |
|
@skeleten You appear to have a merge commit in your PR. |
Add color support for Windows consoles Unfortunately, the `term` API makes this PR rather inelegant, any hints/pointers to make it better are highly appreciated! Fixes #2803
|
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
Unfortunately, the
termAPI makes this PR rather inelegant, any hints/pointers to make it better are highly appreciated!Fixes #2803