RFC: Use real terminal size for show (fix #7295 and #4513)

[simonster]

Replaces tty_rows()/tty_cols() with tty_size(), which (through Terminals) calls uv_tty_get_winsize() to get the terminal size.

The reason for the change is that uv_tty_get_winsize() gives both the rows and columns, so if we wanted to keep tty_rows()/tty_cols(), we'd have to call uv_tty_get_winsize() once for each, even though they are nearly always used together. AFAIK none of the methods whose signatures I've changed were ever exported, but this could still break some packages.

I am assuming that calling uv_tty_get_winsize() is not expensive enough that it's worth registering a SIGWINCH handler and only updating the numbers on window resize, as readline used to do. My Linux system can do >10^6 calls to tty_size() per second, which seems plenty fast to me. Given how cheap the call is, it may not even be worth replacing tty_rows()/tty_cols(). Of course other platforms may be different.

[Keno]

This is still using tty_cols. Is that intended?

[simonster]

Oops, looks like I didn't finish making those changes. I'll fix.

[vtjnash]

The only way to get this info for Mintty is through SIGWICH, or by sending a VT100 sequence to poll for it (currently we just hardcode a size)

[simonster]

AFAIK SIGWINCH only tells you that the terminal size has changed, not what it is. In any case, if uv_tty_get_winsize() is broken, then that seems like it should be fixed in libuv and not here. Using the real terminal size where uv_tty_get_winsize() works seems better than assuming all terminals are 80x25, which is what we do now.

[vtjnash]

Mintty isn't a tty, its a pipe. And the libuv folks have mostly ignored my PR so far

[ivarne]

Deprecation?

These functions are definitely used by packages, and printing methods are typically not well tested.

[simonster]

Added deprecation warnings (not using @deprecate, since that would export them) and removed the try, since I realized that if the libuv call errors the REPL would be broken in various ways anyway.

[johnmyleswhite]

This PR makes me so happy.

[johnmyleswhite]

FWIW, here's a place where you'd want to get just one dimension of the size: https://github.com/JuliaStats/DataFrames.jl/blob/master/src/dataframe/io.jl#L1079

[simonster]

I'm not entirely set on the API change, since I don't think performance matters here, but there is some logic to returning both the rows and the columns since that's what the syscall gives us.

[johnmyleswhite]

I'm not opposed to it. Just noting that there are cases where you only want 1 of the 2. Was originally thinking you'd support a Base.tty_size(1) style interface.

[simonster]

We could do that too (or even keep Base.tty_rows()/Base.tty_cols()), but Base.tty_size()[1] seems concise enough.

[JeffBezanson]

Couldn't this be done with

@deprecate tty_rows() tty_size()[1]

?

[simonster]

That was my first thought, but @deprecate calls export tty_rows, which seems undesirable since tty_rows was not previously exported.

[JeffBezanson]

Ok ready to merge I think?

[stevengj]

Does this still allow you to override the TTY size using the LINES and COLUMNS environment variables? That's important for IJulia (because the display is has nothing to do with the tty in which julia is running).

[vtjnash]

Shouldn't ijulia be making it's own Terminal type?

[stevengj]

@vtjnash, I'm not sure exactly what you're proposing. The IJulia interface is not really a "terminal" in the usual sense, and the current tty_size code seems oriented towards REPL.outstream returning a TTYTerminal, which doesn't seem applicable here.

Couldn't tty_size simply fall back to get(ENV, "LINES", 24), get(ENV, "COLUMNS", 80) rather than simply (24, 80) as it does now?

[simonster]

That would be fine with me.

[vtjnash]

ah, yes. tty_size should probably allow overloading in that case:

tty_size() = isdefined(Base, :active_repl) ? tty_size(REPL.outstream(Base.active_repl)) : tty_size(TextTerminals)
tty_size(os::Terminals.TTYTerminal) = size(os)
tty_size(::Any) = tty_size(TextTerminals)
tty_size(::Type{TextTerminals}) = (get(ENV, "LINES", 24), get(ENV, "COLUMNS", 80)

[StefanKarpinski]

Using the environment for that may not be the best approach in the longer term but for now that seems ok. I guess an advantage of using the environment is that it propagates to child processes.

[simonster]

As discussed in #4117/#5709, what we really want is to find a way to get the terminal size to show that isn't a hack. One possibility is to make tty_size operate on the IO with a fallback to 80x24, which is logical if the IO is a TTY, but then IJulia etc. need to implement their own IO to set tty_size. I'm also not sure it makes sense to tie the other output formatting parameters discussed in #5709 to an IO. We could also start passing show something that isn't an IO, but that would presumably break a lot of things.

[JeffBezanson]

There might be an argument for passing show a kind of "formatting object", which is an IO that wraps another IO object, but also holds info about how to print things. That may be the only way to tame the complexity of print settings (size, output limiting, compact/full, etc.) without either global state, or passing around extra arguments everywhere.