Enable color output whenever stdout is TTY

[karan0299]

Fixes #34317 . Made a check if STDOUT is a TTY and color is not set explicitly by the user , then set global variable have_color to true.
In my opinion , there should be separate have_color variable for STDOUT and STDERR, since we can have separate TTY for STDOUT and STDERR

[JeffBezanson]

Welcome, and thanks for working on this!

In my opinion , there should be separate have_color variable for STDOUT and STDERR

That's a good point. For now, the easy thing would be to only enable color if stdout and stderr are both TTYs.

Looking at the code in more detail, we should really reduce use of the have_color global and instead keep a value per TTY object. Within julia the right way to check for color is get(iostream, :color, false), but for TTYs that is implemented using the global variable. Instead, it should be a field of the object that is set automatically and overridden for stdout/stderr by the --color command line option. What do you think?

[karan0299]

@JeffBezanson
Yeah , This is quite a better approach . There is a mutable struct TTY in base/stream.jl . I think if we add field color in that struct this initially set true and can be overridden by --color as you have already mentioned. This would solve our problem I think. Will try to get a PR on this.

[vtjnash]

Looking at the code in more detail, we should really reduce use of the have_color global and instead keep a value per TTY object

We have both because stdout could be assigned to any object. So there's a have_color global that reflects mostly just reflects the --color flag and applies to any IO object type. And then TTY also is supposed to have some auto-detection capabilities.

[JeffBezanson]

It looks to me like printstyled uses get(io, :color, false), and only TTYs implement :color (by returning the global have_color). I guess --color can continue to determine the default color setting for all TTYs, but ideally when --color is not specified each TTY should set its :color flag by autodetection (by running tput redirected to that object?)

[karan0299]

@JeffBezanson For now i have implemented code to enable color when stdout and stderr are both TTY's . What should be the next step ?

[karan0299]

@JeffBezanson Any updates on this one?

[JeffBezanson]

This seems OK for now. @ararslan requested a NEWS entry, so just add a line to NEWS.md saying that color now defaults to on when stdout and stderr are TTYs.

[karan0299]

I have made an entry in News.md with the purpose of this pr

[JeffBezanson]

Suggested change

	* Color nows defaults to on when stdout and stderr are TTYs ([#34347])
	* Color now defaults to on when stdout and stderr are TTYs ([#34347])

[JeffBezanson]

Thinking about it some more, this probably needs more discussion. In interactive mode, we try to check whether the terminal supports color. If checking is important, then it doesn't make much sense to skip the check in non-interactive mode. Before, we probably wanted to avoid the check to save startup time. I think there are 3 options:

1. Keep things as they are, default to color=no so we don't need any checks.
2. Add the hascolor check from stdlib/REPL/src/Terminals.jl to this PR; don't worry about the startup time cost.
3. Use this PR as-is, and remove the hascolor check from the REPL. Just assume color is available.

[StefanKarpinski]

Idea from discussion on triage: delay checking of the output stream (by calling tput, etc.) until we actually do something that would be different based on if there's color or not. So, for example, println("Hello, world") doesn't care if there's color or not, whereas logging does care, so the color check would happen the first time a log message is printed and then get cached after that.

[JeffBezanson]

Could be done this way:

1. Make the global have_color 3-state: true / false / nothing (nothing means not yet set)
2. The first time the TTY code queries the global, if it's nothing then call the detection function, which will have to be refactored a bit so you don't need a TTYTerminal object.

[karan0299]

@JeffBezanson I have understood your point but while implementing , I am not sure how I can do this without TTYTerminal object.
Thanks.

[tkf]

1. Make the global have_color 3-state

Do you mean for a "quick fix"? It would be really nice if this is a per-instance property. I have multiple packages opening non-stdio TTYs and relying on a global state is rather ugly for such usages.

[karan0299]

I have the implementation to remove Dependency of TTYTerminal object from hascolor function

function hascolor()
        term_type = get(ENV, "TERM", @static Sys.iswindows() ? "" : "dumb")
        startswith(term_type, "xterm") && return true
        try 
          @static if Sys.KERNEL === :FreeBSD  
                return success('tput AF 0')     
            else
                return success('tput setaf 0')  
            end
        catch
            return false
    end
end

@JeffBezanson please verify.

[JeffBezanson]

Yes looks OK to me.

[mkitti]

I think the commits 95c8ea3 59586de in #35240 may be useful to get this over the line.

[karan0299]

@mkitti yeah I think this is a concrete approach to settle the problem

[mkitti]

Remove lines 26 - 30

Suggested change

	<<<<<<< HEAD
	=======
	>>>>>>> 59586ded0da55b383c01b5d6e3538cd9680d79a3
	<<<<<<< HEAD
	=======
	>>>>>>> 59586ded0da55b383c01b5d6e3538cd9680d79a3

[mkitti]

@karan0299 You still have some merge conflict marks at the end of base/ttyhascolor.jl. Take those out.

[JeffBezanson]

Does this need to be its own file? Otherwise it seems like it makes sense to keep it with the implementation of TTY.

[karan0299]

I think ,we can shift this to stdlib/REPL/src/Terminal.jl

[mkitti]

@JeffBezanson The code in ttyhascolor.jl needs to come after process.jl due to the use of success. stream.jl is included before process.jl, and we were getting compilation errors because of this.

[JeffBezanson]

Ok, I figured it was something like that.

[JeffBezanson]

Suggested change

	--color=$(have_color === true ? "yes" : "no")
	--color=$(have_color === nothing ? "auto" : have_color ? "yes" : "no")

[mkitti]

I'm not sure why the one test failed for building on Mac. Looks like some kind of stochastic error. Could someone restart that test to see?

[karan0299]

All CI tests passed

[JeffBezanson]

Thanks for your persistence on this one. Ended up being more complicated than I would have thought! Glad to have this finally merged :)