gwsw / less

Less - text pager
http://greenwoodsoftware.com/less
Other
537 stars 84 forks source link

Regression: text is shown at bottom of screen instead of top #512

Open jlombera opened 3 months ago

jlombera commented 3 months ago

less shows text at bottom of screen instead of top when text is less than one screen in size. This is a regression from previous, historical behavior.

System info

How to reproduce

$ clear ; echo hello | less

Diagnostics

Bisect showed regression was introduced by commit 98782c194f16 ("Move cursor to lower left at startup if we are using an alt screen."). Still reproducible in current HEAD (v654; fabdc1550680). Reverting that commit fixes the problem.

gwsw commented 3 months ago

Old versions of less (before 98782c194f16ba93088d1033702172c3c00f0a61) did not actually display text starting at the top of the screen. It displayed text without moving the cursor, so wherever the cursor was sitting before less was invoked would be where the text appeared. For example,

clear; echo; echo; echo hello | less

would display the text on line 3. While

clear; seq 10; echo hello | less

would display the text on line 11. Change 98782c194f16ba93088d1033702172c3c00f0a61 fixed this nondeterministic behavior by always displaying text starting at the bottom of the screen.

jlombera commented 3 months ago

(Sorry for the late reply, for some reason GitHub didn't notify me about your reply).

I see, thanks for the explanation. I didn't understand what the fix was for from the commit message alone (couldn't discern what an "alt screen" meant).

That said, this fix is introducing a very inconvenient regression. When you open a file/pipe through less you might miss the text if it appears at the bottom (think of a long vertical screen), you might think the file/pipe is empty (it happened to me, that's how I got to this). Even if you notice the text at the bottom (after having to move your eye focus from the top all the way down to the bottom), you'll always wonder and have to verify whether the file/pipe is full of empty lines or is just less showing it at the bottom (e.g. try echo | less or less <empty_file>). Now every tool using less as pager would be affected by this[^1], some of which we might not even have a way to manually specify the pager command used (e.g. less -c). This might even be more confusing/inconvenient for people/tools that have to interact different versions of less that have different behavior.

If the original issue the commit was trying to fix really needs to be addressed (you could always work around the issue by explicitly clearing the screen before running less or passing -c), then I think the proper fix would be to always show the text at the top of the screen instead of the bottom (something like making -c the default (??)).

[^1]: E.g. when I inadvertently updated my system and less was upgraded, my TUI email client started showing messages this way, I was very confused, and assumed malformed emails at first, after manual inspection of the email and discarding this, I suspected my email client had some bug/regression, then I found out it was change in behavior in less.

gwsw commented 3 months ago

An "alt screen" terminal is one that switches to an alternate screen when you enter cursor-addressing mode, and then restores the original screen when you exit cursor-addressing mode. So on an alt-screen terminal, when you quit less, you return to seeing the screen that was displayed when you entered less. I assume this is what you're used to. Non-alt-screen terminals don't do that, so when you exit less, you see the the less output that was displayed while you were running less. Change 98782c194f16ba93088d1033702172c3c00f0a61 is supposed to affect only alt-screen terminals.

I recall that originally I tried to put the initial text at the top of the screen rather than the bottom, but ran into some issues. I don't recall exactly what those issues were. I think it may have been a problem with non-alt-screen terminals before I figured out a way to reliably distinguish between alt-screen and non-alt-screen terminals. I'll take another look at this after the current beta is completed.

jlombera commented 3 months ago

Thanks!

gwsw commented 3 months ago

I looked into this again and the issue is that it doesn't seem to be possible to reliably distinguish an alt-screen terminal from a non-alt-screen terminal. In theory, I believe that the "nrrmc" terminfo flag ("NR" in termcap) should do this, but it doesn't seem that the terminfo database uses this correctly. In fact, in every system I've looked at, this capability is not used by the terminfo description for any terminal. For example, xterm-utf8 uses an alt screen and xterm-256color does not, but neither one sets nrrmc.

It would be wrong to display text on a non-alt-screen terminal at the top of the screen, so I don't see a way to do it on alt-screen terminals.

jlombera commented 3 months ago

Does this mean that 98782c1 is maybe wrong because it used that terminfo capability to detect non-alt screens?

avih commented 3 months ago

It would be wrong to display text on a non-alt-screen terminal at the top of the screen

I presume wrong because you don't want to overwrite existing prior terminal content with the output of "less"?

the issue is that it doesn't seem to be possible to reliably distinguish an alt-screen terminal from a non-alt-screen terminal.

Change 98782c1 is supposed to affect only alt-screen terminals.

AFAICT that commit only adds lower_left() call when init/deinit are non-empty and the NR flag is unset?

I presume you still hold the opinion that maybe except for rare cases, it indeed only affects alt-screen?

How do these two statements agree? on one hand it's supposed to affect only alt screen, but OTOH you can't detect that reliably?

Does it use some sequence of events which print at the bottom with alt-screen, prints at the cursor without alt-screen, but without knowing which one it ended up doing?

Or maybe the detection is mostly correct, but not always, so lower_left() is less bad in case of misdetection than top_left(), because it wouldn't overwrite existing text?

Otherwise, if it can indeed know when it's alt screen, and that lower_left() indeed only happens with alt-screen, can't this call be replaced with top_left() (assuming it exists)?

As far as I can tell with minimal testing with echo 123 | TERM=vt100 less, it indeed affects only when using alt screen (which vt100 doesn't have AFAIK). I.e. the screen is not cleared, and the output starts at the cursor like cat, and adds inverted "END" after the content. (while echo 123 | TERM=xterm less does switch to alt-screen, clears the screen, and displays the content at the bottom).

(I feel like I am missing something, I'm just not sure what).

gwsw commented 3 months ago

I presume wrong because you don't want to overwrite existing prior terminal content with the output of "less"?

Yes, that is correct.

Or maybe the detection is mostly correct, but not always, so lower_left() is less bad in case of misdetection than top_left(), because it wouldn't overwrite existing text?

Basically correct, although I'd probably say "sometimes" rather than "mostly".

An alt-screen terminal MUST have ti/te strings (to switch to and from the alternate screen) and SHOULD NOT have the NR flag. A non-alt-screen terminal MAY have ti/te strings (to enter/exit cursor addressing mode) and SHOULD have the NR flag. What I'm finding is non-alt-screen terminals generally do not have the NR flag set (but I don't understand why; it seems wrong to me), so less is currently using only the existence of ti/te to distinguish them, which erroneously classifies any non-alt-screen terminal which does have ti/te as an alt-screen terminal. On such terminals, less starts by calling lower_left, which may be either a no-op (if the cursor is already at lower_left) or slightly suboptimal (it moves the cursor to lower left but doesn't overwrite any text). Replacing lower_left with goto_line(0) would overwrite existing terminal text on such terminals.

As examples of these three terminal types, on my Fedora system, xterm-utf8 uses an alt screen, xterm-vt52 does not use an alt screen and does not have ti/te, and xterm-256color does not use an alt screen but has ti/te (inconsistently, on my MacOS system, xterm-256color does not have ti/te). So on Fedora this displays at the top of the screen clear; seq 3 | TERM=xterm-vt52 less while this displays at the bottom of the screen clear; seq 3 | TERM=xterm-256color less.

BTW you can see whether a terminal has ti/te by using tput:

$ TERM=xterm-vt52 tput smcup | od -c
0000000

$ TERM=xterm-256color tput smcup | od -c
0000000 033   [   2   2   ;   0   ;   0   t
0000011

Perhaps there's a more reliable way to distinguish alt-screen from non-alt-screen but I haven't found it.

avih commented 3 months ago

xterm-utf8 uses an alt screen, xterm-vt52 does not use an alt screen and does not have ti/te, and xterm-256color does not use an alt screen but has ti/te (inconsistently, on my MacOS system, xterm-256color does not have ti/te)

On Ubuntu 22.04 and Alpine 3.19 xterm-256color does have alt-screen (and "less" does detect it correctly). I find it quite weird, to be honest, that such a common xterm TERM value (which other emulators frequently fake, like VTE terminals) doesn't support alt-screen on some systems...

(same output on Ubuntu and Alpine)

$ TERM=xterm-256color tput smcup | od -c
0000000 033   [   ?   1   0   4   9   h 033   [   2   2   ;   0   ;   0
0000020   t

EDIT: in Fedora 32 live dvd, xterm-256color also supports alt-screen for me, with the same output as above.

Here's what vim says about detecting alt-screen here:

Note: When 't_ti' is not empty, Vim assumes that it causes switching to the alternate screen. ... To avoid this use 't_TI' and 't_TE' (but make sure to add to them, not overwrite).

Maybe use something like that (or that exactly, or more strict if possible), while allowing some env var to list TERM values which have ti but don't have alt screen? something like LESS_NOALT=xterm-256color,foobar,...

And then you can use top_left() instead of bottom_left?

Though to be honest, at least for me, I don't think bottom-left is bad from a usability perspective, but top-left would indeed feel more "natural" for a viewer on a clean screen (alt-screen).

avih commented 3 months ago

Also, I don't know where alt-screen originated (it's not in vt100 or vt220, and I think also not in vt520), but if it was added by xterm, then I think it's extremely likely to be \033[?1049h (the xterm sequence to enter alt-screen) in 99% or more of the terminals, so maybe that can also be a test.

EDIT: a test for ?47 (old xterm alt screen) or ?1047 or ?1049 (newer xterm alt screen) in ti is likely to cover all sequences of alt-screen IMHO.

gwsw commented 2 months ago

Hm, I have a vague memory that I once used a hardware terminal (long before xterm existed) that had a save-screen and restore-screen feature accessed via an escape sequence. Maybe Tektronix? The terminfo for such a terminal might save the screen in smcup and restore it in rmcup.

Anyway, I'm leery of making an assumption that every terminal in the universe uses one of these two escape sequences. The result of writing into the top line because we've incorrectly detected a non-alt-screen terminal as alt-screen is a very bad regression: less will scribble over the user's screen, obliterating data that the user may have wanted to see. On the other hand, the existing behavior where we might incorrectly detect an alt-screen terminal as non-alt-screen is fairly benign; we just start the file at the bottom of the screen, which doesn't seem bad at all. So I'm inclined to leave things as they are.

avih commented 2 months ago

I'm leery of making an assumption that every terminal in the universe uses one of these two escape sequences

The assumption is that if ti includes CSI ?47 or ?1047 ?1049 (not necessarily as the sole CSI value I think) then it has alt-screen, and can display from the top rather than from the cursor.

If it does have alt screen but doesn't use one of those, then it's not terrible, because it would keep behaving like now (display from cursor).

The only "bad" case is when ti does include one of those sequences, but they do something other than alt-screen.

Not saying it can't happen, but I couldn't find anything like that in the ncurses terminfo database. As far as I can tell these sequences are always part of a meta value "does alt-screen like [some mode of] xterm".

The result of writing into the top line because we've incorrectly detected a non-alt-screen terminal as alt-screen is a very bad regression

FIrst of all, note to self and others, the difference in behavior between writing from the top or from the bottom with alt-screen only exists when the content is less than screen height, which is probably not the main use case of "less" - because it doesn't need paging to begin with.

If the content is big enough then the top of the content would always align with the top of the screen anyway, and there's no issue or difference (of initial display).

So it can be a regression indeed with small content, but it's also true that positioning the text at the top when the terminal can afford it (has an alt-screen) does have real value, and is arguably the expected behavior when the screen is cleared and only the content is displayed (alt-screen).

So if one has a useful feature but can't detect reliably whether it can be used, then I think these are the possible options:

Others already do that last option with alt-screen - like vim.

I think should be doing the last option above. I.e. the detection rate is good (as far as we know) and the result of misdetection is typically not terrible, so we can allow to misdetect, despite the fact that it would cause a regression on such case, but it can be overridden and because the most common case would be correct detection.

Defaults do matter, and autodetection should benefit the vast majority of users, while allowing the hypothetical misdetected systems to override manually.

avih commented 2 months ago

Additional notes:

While currently there's no override which explicitly does "start from the bottom/top/cursor", there are two flags which effectively do that:

The only "bad" case is when ti does include one of those sequences, but they do something other than alt-screen.

This can actually be split into two sub-cases, where potentialy only one is destructive: