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

scroll-out in secondary console buffer pollutes main scrollback buffer #17874

Open
avih opened this issue Sep 7, 2024 · 11 comments
Open

scroll-out in secondary console buffer pollutes main scrollback buffer #17874

avih opened this issue Sep 7, 2024 · 11 comments
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting

Comments

@avih
Copy link

avih commented Sep 7, 2024

Windows Terminal version

1.22.240823002-preview

Windows build number

10.0.19045.4780

Other Software

No response

Steps to reproduce

  • Ensure there's some content at the scrollback of the (main) terminal buffer.
  • Run an application which creates a new buffer using CreateConsoleScreenBuffer and then activates it using SetConsoleActiveScreenBuffer.
  • The application writes some lines to the new buffer until it starts scrolling (more lines than the window height can keep visible).
  • The application switches back to the main buffer and exits.

Test program which: prints 100 lines to the main buffer, then creates and activates a new screen buffer, then writes 40 lines to the new buffer, then activates the original buffer and exits:

scrollbuf.c
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <windows.h>


#define writecon(h, s) WriteConsole(h, (s), strlen(s), 0, 0)
#define ERR_EXIT(...) (fprintf(stderr, __VA_ARGS__), exit(1), 0)

void write_n_lines(HANDLE h, const char* prefix, int n)
{
	char buf[64];

	for (int i = 1; i <= n; ++i) {
		writecon(h, prefix);
		sprintf(buf, "%d\n", i);
		writecon(h, buf);
	}
}

int main(int argc, char **argv)
{
	int mode = argc > 1 ? atoi(argv[1]) : 0;

	HANDLE hcon = GetStdHandle(STD_OUTPUT_HANDLE);
	if (hcon == INVALID_HANDLE_VALUE)
		ERR_EXIT("can't get stdout handle\n");

	if (!writecon(hcon, "orig screen buffer\n"))
		ERR_EXIT("can't write to stdout console\n");

	write_n_lines(hcon, "origbuf ", 100);


	HANDLE hnewbuf = CreateConsoleScreenBuffer(
		GENERIC_WRITE, FILE_SHARE_WRITE,
		0, CONSOLE_TEXTMODE_BUFFER, 0);

	if (hnewbuf == INVALID_HANDLE_VALUE)
		ERR_EXIT("can't create new screen buf\n");

	if (!SetConsoleActiveScreenBuffer(hnewbuf))
		ERR_EXIT("can't activete new screen buf\n");

	if (!writecon(hnewbuf, "new screen buffer\n"))
		ERR_EXIT("can't write to new screen buf\n");

	write_n_lines(hnewbuf, "newbuf ", 40);

	Sleep(1000);

	if (!SetConsoleActiveScreenBuffer(hcon))
		ERR_EXIT("can't restore orig screen buf\n");
	CloseHandle(hnewbuf);

	return 0;
}

Expected Behavior

The original scrollback buffer is restored without traces of content which was printed to the new/secondary console buffer.

Actual Behavior

Lines which were printed to the new (secondary) buffer buf were scrolled out of view become part of the scrollback of the main buffer.

With the test program above (which prints 40 lines to the secondary buffer), if the terminal height is less than 40, for instance 30 lines, then the first 10 lines printed to the secondary buffer will be added to the scrollback of the main buffer.

After the program exits, one needs to scroll up (e.g. using the mouse wheel) to ovserve the lines from the secondary buffer.

Additional notes:

  • This does not seem to be a recent regression. I reproduced it also with Windows Terminal 1.15 (the oldest version I have locally), 1.22-preview, and git master artifacts build from a day or two ago (with Original console buffer is not restored when closing a secondary one #17817 already fixed).
  • There is no issue in OpenConsole.exe and also no issue in the native win10 conhost (i.e. the main scrollback buffer is unaffected by overflow at the secondary buffer).

EDIT:
The issue manifests when using less.exe for windows in Windows terminal, e.g. less some-file.c and then scrolling down using the down-arrow, then after less exits then the scrollback buffer has content which was scrolled out-of-view in less (but no issue in OpenConsole or conhost).

@avih avih added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 7, 2024
Copy link

We've found some similar issues:

If any of the above are duplicates, please consider closing this issue out and adding additional context in the original issue.

Note: You can give me feedback by 👍 or 👎 this comment.

@avih
Copy link
Author

avih commented Sep 7, 2024

The bot suggested #17669 might be similar, and it is similar, but not the same: #17669 is about the main buffer polluting the secondary buffer, while this is the other way around.

Also, this issue still exists, while #17669 is presumably already fixed.

@lhecker
Copy link
Member

lhecker commented Sep 8, 2024

No matter whether or how we fix it (pages perhaps? but that only works for WT), less should use the alternative screen buffer on Windows on versions that support it.

@avih
Copy link
Author

avih commented Sep 8, 2024

how we fix it (pages perhaps? but that only works for WT)

Now sure what "only works for WT" means. Do you mean it would be fixed for the windows-terminal but not for OpenConsole/conhost?

If yes, then as I noted above, OpenConsole/conhost don't have this issue. It only happens in windows-terminal.

less should use the alternative screen buffer on Windows on versions that support it.

Maybe, but that's work that someone will have to do, and with more code paths which have to be maintained and tested, and while code which uses CreateConsoleScreenBuffer already exists and not supposed to be broken.

(similar *nix code which uses alt-screen does exist in less, but can't be shared easily with windows code because it uses terminfo db for the sequences etc, and it's also hacky because using alt-screen is not bullet-proof like SGR sequences)

I believe the micro text editor does the same without switching to the alt screen buffer using VT escape sequences, and I'm sure there are many existing application which might not be very actively maintained and which do use CreateConsoleScreenBuffer in order to keep the main buffer (and its scrollback) clean of temporary full-screen TUI content.

@lhecker
Copy link
Member

lhecker commented Sep 8, 2024

FWIW the less shipped by msys2 (e.g. git-for-windows) works correctly, because it uses cygwin and so it uses the alternate screen buffer.


Now sure what "only works for WT" means.

I wrote it as a comment targeted to the other maintainers. We received an implementation for VT pages a while ago (#16615). It allows you to have multiple "pages" (= similar to console buffers, but more limited) at the same time, instead of just the two usual ones (main and alt buffer). That would allow us to improve our CreateConsoleScreenBuffer translation. However, most terminals don't support these sequences AFAIK and so we cannot always use them. That's what I meant with "only works for WT".

Maybe, but that's work that someone will have to do, and with more code paths which have to be maintained and tested

To be fair, that's also true for us. But I understand that we should bear the burden here.
However, unlike less, etc., we cannot make the VT translation perfect. That's something I'll get into more detail below.

I believe the micro text editor does the same without switching to the alt screen buffer using VT escape sequences

Similarly, it would be nice if micro could switch over to using the alt buffer. Windows had support for it for about as long as micro has existed.

and I'm sure there are many existing application which might not be very actively maintained and which do use CreateConsoleScreenBuffer in order to keep the main buffer (and its scrollback) clean of temporary full-screen TUI content.

Such existing applications should be translated to VT to the absolute best of our abilities of course. It's not their fault that Windows changed its focus from the old console APIs over to VT sequences over the last decade. However...


...our abilities to do that translation are limited. There are multiple problems:

  • Pages are not necessarily supported by your favorite terminal (e.g. wezterm).
  • The number of pages is finite, while the number of console screen buffers is infinite.
  • Each console buffer can specify a buffer size (dwSize) on top of a window size (srWindow). I.e. the application can specify your scrollback size, not you. That's a concept that (mostly?) doesn't exist in the VT world (certainly not outside of VT pages).
  • Each console buffer may have different size. We can translate that to CSI t (resize window sequence) but that may be ignored by the hosting terminal. That's definitely true for Windows Terminal: we don't want to allow hidden tabs to randomly resize your window. If the resize is ignored, console applications often break, because they don't check if the size actually changed. CSI t also doesn't address the problem that console buffers may have a custom scrollback size.

But most importantly:

SetConsoleActiveScreenBuffer of a console buffer with existing contents is a lossy operation and some of your screen contents may be permanently lost, because we cannot know what custom VT sequences your favorite terminal supports so we cannot save them either. Since SetConsoleActiveScreenBuffer is used by less (and micro?) to switch back to the main buffer, this issue will always affect you when using these applications.
We're expected to support complex VT, complex Unicode, and complex console APIs simultaneously. But the only thing Windows historically supported was the latter, while the former two are obviously incompatible with that: CHAR_INFO does not support complex VT attributes nor Unicode, buffer read-back doesn't exist in the VT world, and SetConsoleActiveScreenBuffer cannot be cleanly translated to any VT. There's a conflict here and the saying "You can't have your cake and eat it too" comes to my mind.
The solution appears to introduce a VT sequence that represents CreateConsoleScreenBuffer and SetConsoleActiveScreenBuffer perfectly, but then we need to get that new sequence supported by other terminals, including those on Linux (so you can SSH into Windows). At that point the question is: Why should all terminals support the new sequence as opposed to terminal applications not using ReadConsoleOutput and SetConsoleActiveScreenBuffer anymore?

In any case, we'll always try to do our absolute best, and perhaps we can improve the symptoms of your issue. But I don't believe it's fair to expect us to make two completely conflicting technologies fit perfectly.

@avih
Copy link
Author

avih commented Sep 8, 2024

Yeah, I totally understand the main issue and that's console buffer has many features which don't have an equivalent in the unix terminal world, or not enough terminals support enough features to implement many of those in the VT world, so that it can be expected to work fully inside some 3rd part terminal emulator.

That being said, I'd guess one goal of the windows terminal is to provide an environment in which windows console application can behave as if they run in conhost, so I'd think that console screen buffer would need to work somehow, including many/most/all of its features which don't have an equivalent in the VT world.

That may or may not extend to arbitrary conpty clients, but obviously prefably it does extend.

Specifically though, "less" for windows uses the console buffer like alt screen. I.e. it doesn't use a custom size (it does set size, but copies it from the main buffer), and it doesn't do read-back (that I know of, but I think it doesn't), and VT alt-screen also doesn't spill scrolled-out content into the main buffer scrollback, so at least at this specific use case, I don't think it requires features which "normal" alt-screen doesn't have. I.e. I'd think that it could be exposed as normal alt-screen to conpty clients and it would work as expected.

Admittedly I don't know most of the specifics about implementation details with the windows terminal, but I'm now getting the impression that it's a pure VT (conpty) app, probably like other 3rd party windows VT terminals (wezterm, alacritty, etc) and which operates mainly or exclussively using VT sequences?

I understand the issues this approach poses, but also think there's a non-negligible need to support existing console apps which might not get updated to use the VT interface.

Maybe conhost/OpenConsole can identify that a console app uses some features which translate to VT sequences which are not commonly supported by 3rd party terminals, and issue a warning (once-ish per app) that 3rd party terminals might not be compatible with this application (while the windows terminal would support it via extended sequences which emulate the console buffer fully, etc).

It's not my place to suggest solutions here and I'm unaware of the constraints you work with, but I do think that at least the windows terminal should be a good environment for legacy console apps, even if they use features which translate to VT sequences which are not universally supported.

Isn't that right?

@avih
Copy link
Author

avih commented Sep 15, 2024

Does this await resolution?

@lhecker
Copy link
Member

lhecker commented Sep 16, 2024

That being said, I'd guess one goal of the windows terminal is to provide an environment in which windows console application can behave as if they run in conhost [...]

I fully agree with this, which is why I made the following plan: https://github.com/microsoft/terminal/blob/main/doc/specs/%2313000%20-%20In-process%20ConPTY.md
That will need at least another year or so to be done though.

[...] but I'm now getting the impression that it's a pure VT (conpty) app, [...] which operates mainly or exclussively using VT sequences?

Yep, currently Windows Terminal operates exclusively via VT sequences. The above plan would change this: Windows Terminal would then be a native console server (like good old conhost), while we would still only use pure VT for windows applications run under WSL, as well as SSH and some other tools.

I think that approach has the best trade-off of being relatively easy to implement (by us and others), it's performant and robust. The only downside is that it'll take a while to get there.

@avih
Copy link
Author

avih commented Sep 16, 2024

I fully agree with this, which is why I made the following plan: https://github.com/microsoft/terminal/blob/main/doc/specs/%2313000%20-%20In-process%20ConPTY.md
That will need at least another year or so to be done though.

Can't say I entirely or even mostly grok it, but out of curiosity, would that mean that the windows terminal would be a good env for "legacy" console apps, but those same apps would run less good in 3rd party terminal emulators, because those work exclussively via VT sequences, and then some APIs can't be translated well enough (like is the case now with the windows terminal)?

And specifically about this issue, does that mean that it won't be fixed until this transition is complete?

Because:

Specifically though, "less" for windows uses the console buffer like alt screen. I.e. it doesn't use a custom size (it does set size, but copies it from the main buffer), and it doesn't do read-back (that I know of, but I think it doesn't), and VT alt-screen also doesn't spill scrolled-out content into the main buffer scrollback, so at least at this specific use case, I don't think it requires features which "normal" alt-screen doesn't have. I.e. I'd think that it could be exposed as normal alt-screen to conpty clients and it would work as expected.

I'd imagine that console buffer would be implemented/exposed using alt-screen, and as such, the issue shouldn't exist, because content which scrolls out of the alt-screen doesn't normally pollute the main scrollback buffer in any *nix terminal, right?

IOW, I'd think that this specific issue, depending on implementation details, does not actually need this planned architechture change?

@lhecker
Copy link
Member

lhecker commented Sep 16, 2024

Can't say I entirely or even mostly grok it, but out of curiosity, would that mean that the windows terminal would be a good env for "legacy" console apps, but those same apps would run less good in 3rd party terminal emulators, because those work exclussively via VT sequences, and then some APIs can't be translated well enough (like is the case now with the windows terminal)?

No, because over SSH these apps will be broken in Windows Terminal just as before, just like they're broken in conhost right now. Further "no", because the linked proposal can be implemented by any other terminal - it's not a proposal specific to Windows Terminal. And lastly, if we were to introduce a new sequence for translating console screen buffers instead, it would still take years for any other terminal adopt it. Given other precedent, what's most likely is that no other terminal will ever adopt it.

Just to clarify: Implementing the linked spec document above doesn't take a year. It takes maybe 1 month or so. It's just that I also need to work on a lot of other issues as well.

And specifically about this issue, does that mean that it won't be fixed until this transition is complete?

Unfortunately, yes. As I outlined above, "pages" VT sequences aren't supported by other terminals and also don't perfectly map console buffers, so even if we were to use them, someone would likely soon find another bug with them. Custom VT sequences would similarly not be supported by other terminals. Lastly, implementing a solution with VT sequences now will take just as long to implement as implementing the linked proposal above, because Windows Terminal currently doesn't support more than 2 text buffers (outside of VT pages), etc.

I'd imagine that console buffer would be implemented/exposed using alt-screen, and as such, the issue shouldn't exist, because content which scrolls out of the alt-screen doesn't normally pollute the main scrollback buffer in any *nix terminal, right?

I don't want to do any such translations at the moment because they're "risky". Any edge cases around console buffer switching would need to be fixed by me. Since Windows has had supported for the xterm alt buffer for almost a decade now, I think it's instead on these still fully maintained applications to switch over to using the alt buffer.

I would understand it, if this issue was a major one (like a crash), but it's really more of a really annoying edge case, wouldn't you agree? Basically, I just don't want to invest 50% of the time now to get a quick hack in that solves 10% of the problem, when I can also just wait 100% of the time and fix the problem 100%.
Personally, I'm just kind of tired of using software that's a hundred quick hacks glued together under a single trench coat (I'm sure you can imagine a lot of examples, particularly also from Microsoft, perhaps including this very application). For now, I just want to write some quality software. I hope you understand. 🙁

@avih
Copy link
Author

avih commented Sep 16, 2024

if we were to introduce a new sequence for translating console screen buffers instead, it would still take years for any other terminal adopt it. Given other precedent, what's most likely is that no other terminal will ever adopt it.

We're guessing, but my guess would be the same as yours.

Unfortunately, yes. As I outlined above, "pages" VT sequences aren't supported by other terminals and also don't perfectly map console buffers, so even if we were to use them, someone would likely soon find another bug with them. Custom VT sequences would similarly not be supported by other terminals.

I agree. I don't think solving it with custom or uncommon sequences is great.

I'd imagine that console buffer would be implemented/exposed using alt-screen, and as such, the issue shouldn't exist,

I don't want to do any such translations at the moment because they're "risky". Any edge cases around console buffer switching would need to be fixed by me. Since Windows has had supported for the xterm alt buffer for almost a decade now, I think it's instead on these still fully maintained applications to switch over to using the alt buffer.

Well, I was guessing that console buffers are already getting translated to alt-screen, and if that were the case then I thought the issue shouldn't exist already currently, but judging by this statement, that's not the case.

May I ask what happens currently, implementation-wise (but high level), which causes text which scrolls out of a (non-main) screen buffer to reach the terminal's scrollback?

I would understand it, if this issue was a major one (like a crash), but it's really more of a really annoying edge case, wouldn't you agree?

I do agree, but for me it's annoying enough to get back to conhost or openconsole, because I do use "less" a lot, and I do value my scrollback history, but if everytime I use "less" it adds pages of junk to my scrollback, then it will be too annoying.

(or I might implement alt-screen in "less" when VT is supported, because I already contribute to "less", but that's orthogonal to the issue here IMO)

In fact, I already did just that for a long while (using OpenConsole rather than windows terminal), and tried to get back to the terminal recently, and noticed some console buffer issues.

Believe it or not, my main issue with OpenConsole is that shift-mouse-select is unsupported (when quick-edit is disabled, e.g. because the application wants to capture mouse events). Other than that OpenConsole is quite great for me (though I do value the nice features of Windows Terminal too, but for me they're more nice to have than essential, maybe except scrollback search which is very useful).

Shall I open a feature request to support shift-mouse-select in OpenConsole to override disabled-quickedit?

Personally, I'm just kind of tired of using software that's a hundred quick hacks glued together

Completely agreed, though many times unavoidable in practice, because such is life. But if you do get the chance to avoid yet another hack, without great cost, I agree you should.

I just want to write some quality software

Not doing too bad so far, if I may say so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting
Projects
None yet
Development

No branches or pull requests

2 participants