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

Support ANSI colors in msys terminals. See #2807 #11031

Closed
wants to merge 1 commit into from

Conversation

jhasse
Copy link
Contributor

@jhasse jhasse commented Dec 17, 2013

Enable ANSI colors if TERM is set to cygwin and terminfo is not available (msys terminal on Windows). See #2807

pub struct Terminal<T> {
priv num_colors: u16,
priv out: T,
priv ti: ~TermInfo
priv ti: Option<~TermInfo>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be cleaner to synthesize a TermInfo entry for ANSI terminals,- rather than hard-coding escapes into this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The escapes would have to be hard coded in this TermInfo entry then anyway, wouldn't they?

It would probably be a cleaner solution, but I don't know enough Rust to implement that (correctly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, TermInfo would be hard-coded, but at least all information would be in one place, not scattered across the Terminal class.
Also, this would open the door for other custom-detected terminal types, depending on the OS perhaps. For example, you could detect if Rust is running on top of libuv-based IO stack and have a TermInfo variant corresponding to escapes that libuv supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I have a (much shorter) patch ready which uses this approach.
I will push it tomorrow after some more testing ;)

@emberian
Copy link
Member

I'm more or less ok with this PR. But, could we convince upstream cygwin to include a terminfo entry? I'd like to remove this hack eventually. Hardcoding terminals is no good.

@jhasse
Copy link
Contributor Author

jhasse commented Dec 19, 2013

Agreed. Keep in my mind that this is msys only (cygwin has terminfo
support). The TERM variable is set to "cygwin", because msys is a fork of
an older cygwin version. So we would have to ask the msys/mingw devs for
terminfo support.

Also wait for tomorrow, my new patch (using TermInfo struct) is cleaner :)

@emberian
Copy link
Member

Ok, sounds good. Thanks for taking this up!

On Thu, Dec 19, 2013 at 10:23 AM, Jan Niklas Hasse <[email protected]

wrote:

Agreed. Keep in my mind that this is msys only (cygwin has terminfo
support). The TERM variable is set to "cygwin", because msys is a fork of
an older cygwin version. So we would have to ask the msys/mingw devs for
terminfo support.

Also wait for tomorrow, my new patch (using TermInfo struct) is cleaner :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/11031#issuecomment-30936797
.

@jhasse
Copy link
Contributor Author

jhasse commented Dec 20, 2013

Done :) This time I force pushed instead of creating a new PR.

This new patch also enables bold text and disables 16 colors (they were emulated using bold text).

bors added a commit that referenced this pull request Dec 21, 2013
Enable ANSI colors if TERM is set to cygwin and terminfo is not available (msys terminal on Windows). See #2807
@bors bors closed this Dec 21, 2013
@jhasse jhasse deleted the patch-msys-3 branch December 21, 2013 13:46
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
New lint [`needless_return_with_try`]

Closes rust-lang#10902

Rather than having a config option, this will just suggest removing the "return"; if `try_err` is used as well, then it'll be added again but without the `?`.

changelog: New lint [`needless_return_with_try`]
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.

5 participants