GetConsoleScreenBufferInfo usage is likely incorrect

[j4james]

Apologies for not providing version information, but I haven't actually tried your Windows build yet - I'm just going by what I've seen in the code, which looks to me to incorrect.

To understand why I think this is wrong, you need to understand the layout of the Windows console buffer. There is the buffer itself, which is similar in function to the scrollback buffer in a Linux terminal, and there is the display window, which is the visible view of that buffer. The addressable area (from a VT point of view) spans the width of the buffer, but the height of the display window. See the diagram below:

+--------------------------+      ^
|                          |      |
|                          |      |
+-----+--------------+-----+ ^ w  | b
|XXXXX|XXXXXXXXXXXXXX|XXXXX| | i  | u
|XXXXX|XXXXXXXXXXXXXX|XXXXX| | n  | f
|XXXXX|XXXXXXXXXXXXXX|XXXXX| | d  | f
|XXXXX|XXXXXXXXXXXXXX|XXXXX| | o  | e
+-----+--------------+-----+ v w  | r
|                          |      |
|                          |      |
+--------------------------+      v

      <--- window --->

<--------- buffer --------->

There are a couple of ways in which this differs from most Linux terminals:

1. Because the buffer extends past the bottom of the display window, a user can potentially scroll down beyond what would normally be thought of as the end of the buffer (this probably won't affect you though).
2. Because the buffer can be wider than the display window, the user can scroll horizontally to view parts of the addressable area that aren't currently visible.

The important thing to understand about point 2, is that if you use CUP to move to column 1, it's column 1 of the buffer rather than the window (although the window may automatically scroll to bring that position into view). Similarly, when text wraps, it'll be at the rightmost column of the buffer rather than the window, and will wrap to column 1 of the buffer. Vertical movement is still constrained to the range of the display window though (at least using VT sequences).

The reason why this is a problem for your use of GetConsoleScreenBufferInfo, is that you seems to be assuming the addressable width is the display window width, which could potentially be too narrow. https://github.com/dankamongmen/notcurses/blob/409ee0884900e55cd5858ff4e5dfee409fb348d7/src/lib/notcurses.c#L305

The second problem is that the cursor position returned by GetConsoleScreenBufferInfo is the position in the buffer rather than the window. So to obtain the relative position in the addressable VT area, you'd need to adjust the Y offset relative to srWindow.Top in this code: https://github.com/dankamongmen/notcurses/blob/3abeb81cb8ff5df92e1a4b176783591d7de9c675/src/lib/termdesc.c#L1013

That said, on Windows Terminal the buffer is hard coded to be the exact same size as the display window, so your current implementation should be fine on Windows Terminal for now. It's not going to work in the old Windows console, though, and could potentially break in a future version of Windows Terminal.

To make matters worse, even if you do everything correctly, there are still edge cases where things could go wrong. For example, if the user happens to be scrolling through the scrollback, the display window returned from GetConsoleScreenBufferInfo represents their scroll position, which is not the same as the addressable area, making it impossible to calculate the relative Y position correctly.

Really the safest way to calculate the cursor position (in the VT sense) is with a standard CPR query. Obviously that's asynchronous, though, so maybe that's not practical for your needs.

[dankamongmen]

i'm definitely not looking to support the "conhost-style" old windows terminal (happy to use a better term), i.e. any prior to the escape sequence-driven pseudotty-based one. with that said, it sounds like i'm just straight-up misusing the function, and i'll want to address this.

the windows code is all pretty experimental still. it's improving, but i can't yet call it usable. i just got cursor location reports working last night.

[dankamongmen]

oh, and thanks for the detailed writeup! i really appreciate it!

[j4james]

i'm definitely not looking to support the "conhost-style" old windows terminal (happy to use a better term), i.e. any prior to the escape sequence-driven pseudotty-based one.

Maybe I'm misunderstanding you, but just to be clear, the old windows terminal is also escape sequence-driven - it's just not on by default for Windows console applications unless you set the ENABLE_VIRTUAL_TERMINAL_PROCESSING mode (which you're already doing). And the new Windows Terminal is essentially just a fancy UI on top of that - under the hood it's conhost that's doing most of the work (in terms of escape sequence processing).

[dankamongmen]

Maybe I'm misunderstanding you, but just to be clear, the old windows terminal is also escape sequence-driven - it's just not on by default for Windows console applications unless you set the ENABLE_VIRTUAL_TERMINAL_PROCESSING mode (which you're already doing). And the new Windows Terminal is essentially just a fancy UI on top of that - under the hood it's conhost that's doing most of the work (in terms of escape sequence processing).

nope, you were understanding me correctly, and i was misunderstanding the relationship between the two. i thought all escape stuff in the old terminal was ANSI.SYS, and that it had been removed in some dark period between DOS 6.22 (the last time i used a microsoft OS) and the dawn of new windows terminal

[j4james]

I can't remember when ANSI.SYS disappeared - I'm guessing around the time of Windows Vista. But support for VT sequences were added back to the conhost console in 2016. That was kind of essential for WSL to work. The new Windows Terminal only came much later (2019 I think), and is implemented as a conpty client on top of conhost.

What this means is that conhost does all the initial rendering into an internal buffer - this includes both legacy console API calls that write directly to the buffer, as well as VT sequences (if the appropriate mode is enabled). The conpty layer then translates the contents of that buffer into a new stream of VT sequences which get passed through a pipe to the client terminal.

The main consequence of this architecture is that all conpty clients on Windows are essentially limited to the capabilities of conhost. They never actually receive the original escape sequences - just a dumbed down representation of the conhost buffer after it has processed the sequences as best it can. In some cases it will pass through a sequence directly if it doesn't understand it, but the chances of that working correctly are slim.

There are plans to try and implement a pass-through architecture, which will allow third-party terminals to receive the original escape sequences directly, so you can benefit from features that they support which conhost doesn't, but that's probably still a long way off.

[dankamongmen]

2284 was very likely related

[dankamongmen]

There are plans to try and implement a pass-through architecture, which will allow third-party terminals to receive the original escape sequences directly, so you can benefit from features that they support which conhost doesn't, but that's probably still a long way off.

so we're using this, or at least considering using this, in two places:

• cursor location detection (currently commented out, see #2284)
• screen size detection (as opposed to an ioctl()-based approach), still enabled, see update_term_dimensions()

the former isn't that big a deal; we use u7 like we do everywhere else, and it's answered meaningfully, and life goes on. we could theoretically get cursor reports without a pty round-trip, but they're pretty infrequent, so whatever.

getting screen dimensions is another thing entirely. we do this right before each render, with the hope of rendering for the correct screen size (there's a fundamental race here, obviously). we don't want to introduce a roundtrip in this hot path. so even though there exist control sequences to get those values, i'd prefer to continue using GetConsoleScreenBufferInfo().

if we're going to put the time into auditing our use of it and coming to a definite understanding of it, we probably might as well use it for cursor location reports, too. either way, let's get this locked down now.

[dankamongmen]

@j4james it seems to me that this implementation has the property that your store has a fixed width, even though your view has a dynamic width. i.e. if i have a buffer 80 wide, and my window shrinks below that, the 80 are still there (and can be seen via horizontal scrolling). so two questions:

• if my display area grows larger than my buffer, does the buffer grow to match it?
• can my buffer's width change via any other user action?

[dankamongmen]

the other question would be how well this is all supported under ssh. if i ssh from a windows conpty client to a windows server and run a notcurses program, what happens? what about windows to unix? unix to windows?

[dankamongmen]

if we're going to put the time into auditing our use of it and coming to a definite understanding of it, we probably might as well use it for cursor location reports, too. either way, let's get this locked down now.

hrmmm, but...

To make matters worse, even if you do everything correctly, there are still edge cases where things could go wrong. For example, if the user happens to be scrolling through the scrollback, the display window returned from GetConsoleScreenBufferInfo represents their scroll position, which is not the same as the addressable area, making it impossible to calculate the relative Y position correctly.

well, shit. u7 it is then.

[dankamongmen]

The reason why this is a problem for your use of GetConsoleScreenBufferInfo, is that you seems to be assuming the addressable width is the display window width, which could potentially be too narrow.

well it could also be too wide, right (depending on the answer to my question above)? so the former case would manifest as not using all the available buffer (though i would use all the visible space). where the line ends doesn't matter, because i always hard-cup to different lines (i.e. i never rely on am). the latter case would manifest as writing overlong lines, presumably creating a mess.

[j4james]

if i have a buffer 80 wide, and my window shrinks below that, the 80 are still there (and can be seen via horizontal scrolling)

Yes, but this is an option. The user can also choose to have the buffer and window sizes match exactly (which is the default in conhost, and always the case in Windows Terminal).

if my display area grows larger than my buffer, does the buffer grow to match it?

Yes.

can my buffer's width change via any other user action?

The user can go into the options and set the buffer and window sizes to whatever values they want.

if i ssh from a windows conpty client to a windows server and run a notcurses program, what happens?

Once conpty enters the picture it's all a lot simpler. The buffer and window sizes should match exactly.

[j4james]

well it could also be too wide, right

No. Even if you try and set the window size wider than the buffer from within the settings, it'll automatically adjust the other value to be in range.

[dankamongmen]

Once conpty enters the picture it's all a lot simpler. The buffer and window sizes should match exactly.

just to check, this is the case in Windows Terminal, but only the case by default for conpty in general, right?

[dankamongmen]

alright, since the visual area can't exceed the buffer size, the only problem needing concern is visual area smaller than buffer size, which ought lead to nothing worse than incomplete use of the buffered area.

so let's try and get the buffer geometry (width anyway), though honestly if this is the worst outcome, i'm not too worried.

[dankamongmen]

i don't see anything in https://docs.microsoft.com/en-us/windows/console/getconsolescreenbufferinfo that gives us the buffer geometry. let me look around a bit.

[dankamongmen]

https://docs.microsoft.com/en-us/windows/console/console-screen-buffer-infoex doesn't seem like much of an improvement on this front

[j4james]

Once conpty enters the picture it's all a lot simpler. The buffer and window sizes should match exactly.

just to check, this is the case in Windows Terminal, but only the case by default for conpty in general, right?

Neither Windows Terminal nor conpty have any control over this - they always get a buffer that exactly matches the window size as far as I know. The only place that you can get these mismatched window and buffer sizes is in the legacy conhost console.

i don't see anything in https://docs.microsoft.com/en-us/windows/console/getconsolescreenbufferinfo that gives us the buffer geometry. let me look around a bit.

You need CONSOLE_SCREEN_BUFFER_INFOEX.

[dankamongmen]

wait, for both ...Buffer() and ...BufferEx(), we have:

dwSize A COORD structure that contains the size of the console screen buffer, in character columns and rows.

they say "console screen buffer", but you're sure it applies to the visual area?

[j4james]

they say "console screen buffer", but you're sure it applies to the visual area?

No. See my original picture at the top of this issue. It's the area marked "buffer". But what you care about is the addressable VT area (marked with Xs), which is a combination of the buffer width and the window height.

[dankamongmen]

ooooohhhh so i'm getting buffer width from CONSOLE_SCREEN_BUFFER_INFOEX (though i think the simple one would work, too; we just need dwSize right?) but i need the height from the visual area ("display window"). hrmmm. i don't see any way to get that, either; the srWindow tells us the oirigin of the display window....ahhhh so it's just total buffer height less origin of display area. got it!

[dankamongmen]

so that's why you were saying i need CONSOLE_SCREEN_BUFFER_INFOEX -- to get srWindow for that calculation.

[j4james]

I may be wrong actually. It looks like CONSOLE_SCREEN_BUFFER_INFO also has both buffer and window sizes.

[dankamongmen]

where are you seeing a window size? do you mean dwMaximumWindowSize? that's different

[j4james]

The srWindow structure in https://docs.microsoft.com/en-us/windows/console/console-screen-buffer-info-str.

[j4james]

Btw, if you want notification of size changes like ioctl, I think you get that that by monitoring the WINDOW_BUFFER_SIZE_EVENT (see https://docs.microsoft.com/en-us/windows/console/reading-input-buffer-events).

[dankamongmen]

  if(GetConsoleScreenBufferInfo(tcache->outhandle, &csbi)){                                                                         
    *cols = csbi.srWindow.Right - csbi.srWindow.Left + 1;                                                                           
    *rows = csbi.srWindow.Bottom - csbi.srWindow.Top + 1;                                                                           
  }else{                    

[dankamongmen]

oh i see how you mean it's geometry. srWindow is coordinates but, with all 4, we can trivially extract geometry. got it.

[dankamongmen]

so to sum this up, it looks like our calculations of screen geometry are correct, but our cursor location determination was flawed. i've moved to exclusively using u7, so that shouldn't be a problem anymore. i just committed a change to not fire off u7s unless none are outstanding, which ought eliminate the extra return we see in mintty. i think we're done here.

[j4james]

Unless I've missed something, it looks to me like the cols calculation is still wrong:

https://github.com/dankamongmen/notcurses/blob/58ca4c5fa372dcf23207f79a7558d68a15050614/src/lib/notcurses.c#L302

Look at my diagram in the top comment again. If you're trying to determine the addressable VT area, what you need is the buffer width and the window height. So your rows calculation is fine, but your cols calculation should be:

*cols = csbi.dwSize.X;

[dankamongmen]

Look at my diagram in the top comment again. If you're trying to determine the addressable VT area, what you need is the buffer width and the window height. So your rows calculation is fine, but your cols calculation should be:

ahhh of course, got it; thanks again for your patience and determination to see this work properly despite my strongest endeavors to screw it up =]