More sane behavior if terminal size is zero

[tailhook]

In linux pseudo-terminals are created with dimensions of zero. If host application didn't initialize the correct size before start we treat zero size as 80 columns and
inifinite rows.

See containers/podman#351 and rust-cli/rexpect#17 for examples of such issues.

[tailhook]

The alternative is to set columns to usize::max_value() (effectively unlimited). But since we already use 80x25 in case the output is not a terminal, 80 columns make sense in this case too.

[bjorn3]

Suggested change

	let rows = if size.ws_row == 0 {
	0
	} else {
	usize::max_value()
	};
	let rows = if size.ws_row == 0 {
	usize::max_value()
	} else {
	size.ws_row as usize
	};

[bjorn3]

Also should it use 24 as fallback instead of usize::max_value()?

[tailhook]

This is a good question actually, my point is:

1. There are two cases where rows are used: when scrolling something (e.g. completions), and for vertical scroll once Vertical scroll #378 is landed
2. There is one primary case when terminal size is zero: when using some automation, like rexpect

So having no vertical scroll for automation is good idea.

In case there is a bug, like this one: containers/podman#351 This kind of degradation is good: if we have 24 rows and input is large enough that doesn't fit 24 rows, it maybe unclear for user why part of their input is hidden. And if their terminals is smaller than 24 rows (which may easily happen for split windows) no vertical scroll looks nicer than vertical scroll that is larger than an actual terminal.

[gwenn]

Maybe we should do like linenoise ?
https://github.com/antirez/linenoise/blob/master/linenoise.c#L308

[tailhook]

Maybe we should do like linenoise ?
https://github.com/antirez/linenoise/blob/master/linenoise.c#L308

I'll take a look if it works.

[tailhook]

Maybe we should do like linenoise ?
https://github.com/antirez/linenoise/blob/master/linenoise.c#L308

It works in the cases where terminal is forwarded (i.e. likely would work in the podman issue above). But rexpect doesn't answer on "get cursor" messages at all.

Also it looks like quite unreliable: move_cursor_at_leftmost uses 100ms to timeout on response. It may be not enough over SSH or if users machine is overloaded.

[gwenn]

https://github.com/AmokHuginnsson/replxx/blob/master/src/io.cxx#L143-L145

	// cols is 0 in certain circumstances like inside debugger, which creates
	// further issues
	return ( cols > 0 ) ? cols : 80;

https://github.com/prompt-toolkit/python-prompt-toolkit/blob/master/prompt_toolkit/output/vt100.py#L458-L471

            # If terminal (incorrectly) reports its size as 0, pick a
            # reasonable default.  See
            # https://github.com/ipython/ipython/issues/10071
            rows, columns = (None, None)

            # It is possible that `stdout` is no longer a TTY device at this
            # point. In that case we get an `OSError` in the ioctl call in
            # `get_size`. See:
            # https://github.com/prompt-toolkit/python-prompt-toolkit/pull/1021
            try:
                rows, columns = _get_size(stdout.fileno())
            except OSError:
                pass
            return Size(rows=rows or 24, columns=columns or 80)

https://github.com/troglobit/editline/blob/master/src/editline.c#L1328-L1336

#ifdef TIOCGWINSZ
        if (ioctl(el_outfd, TIOCGWINSZ, &W) >= 0 && W.ws_col > 0 && W.ws_row > 0) {
            tty_cols = (int)W.ws_col;
            tty_rows = (int)W.ws_row;
            return;
        }
#endif
        tty_cols = SCREEN_COLS;
        tty_rows = SCREEN_ROWS;

So they all agree with you except on the default rows.

[tailhook]

Well, I'm okay to make it 24 if my argumentation doesn't seem good enough for you. On the other hand, I'm not convinced we should copy what anybody else have done, because if they have some full-screen widgets it doesn't make sense with infinite height for them (or they might have copied that code from such place). But I'm not going to argue for death on this issue, as this use case is quite rare.

[Marwes]

This unfortunately breaks my rexpect tests (some of them hang forever) :( https://github.com/gluon-lang/gluon/blob/master/repl/tests/rexpect.rs (bisected to this commit)

[tailhook]

This unfortunately breaks my rexpect tests (some of them hang forever) :( https://github.com/gluon-lang/gluon/blob/master/repl/tests/rexpect.rs (bisected to this commit)

That's interesting. Do you have something that tests vertical scrolling? Or something wider than 80 chars?

[Marwes]

There are some tests print wider than 80 chars. But not all tests that hang test that, for instances "comments" hangs but that only prints some ~40 chars wide lines.

    let mut repl = REPL::new();

    repl.test("1 + 2 // Calls the + function on 1 and 2", Some("3"));
    repl.test("1 + 2 /* Calls the + function on 1 and 2 */", Some("3"));