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

CSI 18t "Report Size In Characters" not working. #13944

Closed
PitWD opened this issue Sep 8, 2022 · 14 comments · Fixed by #15295
Closed

CSI 18t "Report Size In Characters" not working. #13944

PitWD opened this issue Sep 8, 2022 · 14 comments · Fixed by #15295
Labels
Area-VT Virtual Terminal sequence support good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Milestone

Comments

@PitWD
Copy link

PitWD commented Sep 8, 2022

Windows Terminal version

1.15.2282.0

Windows build number

10.0.19044.1889

Other Software

Own project.

Steps to reproduce

Start CMD or PS within Terminal.
Run app which activates VT-Modes for IN and OUT.
Send CSI 18 t to stdout.
Read (wait for) response on stdin.

Expected Behavior

A response from the Terminal a la:
CSI 'Decimal-Rows' ; 'Decimal-Columns' t

Actual Behavior

No response from the Terminal.
But i.e. sending CSI 6 n (Report Actual Cursor Position) results as expected in a response a la:
CSI 'Decimal-Row' ; 'Decimal-Column' R

@PitWD PitWD added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 8, 2022
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 8, 2022
@j4james
Copy link
Collaborator

j4james commented Sep 8, 2022

We just don't support any of the CSI t queries. I actually thought we already had an open issue for this, but I can't find it right now.

I should also say I'd recommend against relying on CSI t for querying the terminal dimensions if you're expecting your application to be widely used. There are quite a few terminals that don't support it, and DSR-CPR can serve the same purpose more reliably.

@PitWD
Copy link
Author

PitWD commented Sep 8, 2022

So, you mean that way:

  1. Save Actual CursorPos
  2. Place Cursor Outside Limits
  3. Read Resulted CursorPos ( CSI 6 n )
  4. Place Cursor Back To 1.

??

I couldn't find a Linux Terminal not supporting CSI t. On Monterey-Mac I only tested the "on board" terminal - working fine too.
Yes, the 4-steps method seems actually more cross-compatible - but just because of the MS-Terminal refuses CSI t - not because of one of the 10 most used Linix-Terminals or the OnBoard Mac-Terminal would struggle with CSI t.

@j4james
Copy link
Collaborator

j4james commented Sep 8, 2022

So, you mean that way:

1. Save Actual CursorPos

2. Place Cursor Outside Limits

3. Read Resulted CursorPos ( `CSI 6 n` )

4. Place Cursor Back To 1.

You make it sound like it's a complicated serious of operations, but it's essentially just a longer escape sequence. Something like \e7\e[999;999H\e[6n\e8.

I couldn't find a Linux Terminal not supporting CSI t.

Admittedly I don't always have the latest version of every terminal, and it's been a while since I last tested these sequences, so maybe things have improved a lot since then. At the time I think there were a half dozen or so that didn't support CSI t, or produced invalid results. But if you're happy to rely on CSI t based on your own research, that's fine. I was just offering my opinion.

@PitWD
Copy link
Author

PitWD commented Sep 8, 2022

Uh, please do not misunderstand me.
I am really thankful for what MS is doing with the Terminal/VS-Code/WSL etc. I'm also impressed getting that fast an answer - and will follow your advice.
I'm on a MicroChip 2560... serial connected to Terminals. The whole TUI-Logic is running on the µC. Terminal is just a screen and a keyboard. In best case I do not even need a binary on the Terminal side to get serial redirected to stdio of a VT-Mode terminal.
So yes, on the µC I'm glad about every byte I can save... in code and on the serial line.
On the other side... there are some more issues where very small OS-specific binaries would make life much easier...

@zadjii-msft
Copy link
Member

I'll toss this on the backlog. No arguments that we should support it, even if there is a viable (albeit, verbose) alternative.

@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. and removed Issue-Bug It either shouldn't be doing this or needs an investigation. labels Sep 8, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone Sep 8, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 8, 2022
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 8, 2022
@PitWD
Copy link
Author

PitWD commented Sep 8, 2022

No arguments that we should support it

For me it looks much more logical to call a GetScreenSize-Function if I want to know the ScreenSize, than to call some functions to be able to use GetCursorPos as a substitute for GetScreenSize.

I have to set a flag NextCurserPosIsAScreenSize and have to check on each incoming CursorPos if it's not a ScreenSize...

That's in coding really not a thing - but on a 2560 it's 1 byte global RAM, ~8 additional cpu cycles per incoming CursorPos, and ~30 Bytes program memory.
I have to waste resources of the µC - cause 34% of Users want to administrate them with Windows.

CSI t commands do not conflict with anything - I also can't see why it wouldn't make sense to get the MS-Terminal (without cnflicting riscs) again one step closer to Xterm-compatibility...

@zadjii-msft
Copy link
Member

zadjii-msft commented Sep 8, 2022

Note

Walkthrough

Implementation would be fairly trivial.

  • Add a enum value to DispatchTypes::WindowManipulationType for ReportWindowSizeInCharacter
  • Add a handler for ReportWindowSizeInCharacter in AdaptDispatch::WindowManipulation
  • Do a _api.ReturnResponse(...); with a string formatted with the viewport dimensions.

I'll mark this up as a good first issue, cause the implementation should be pretty straightforward.

@zadjii-msft zadjii-msft added the good first issue This is a fix that might be easier for someone to do as a first contribution label Sep 8, 2022
@michalnpl
Copy link
Contributor

michalnpl commented Apr 28, 2023

@zadjii-msft, @PitWD, @j4james I have a branch to support this ready for PR, but I am unsure about interoperability. My apprehension is about microsoft/terminal rendering escape characters when not read, which differs from Ubuntu (bash, xterm-256color). What's more, is that PowerShell and cmd.exe do render CSI 18t differently when not read (PowerShell does not render \033 while cmd.exe does render \033[8). Kindly please advise if this is the desired behavior.

For test code:

#include <stdio.h>

int main() {
    printf("\033[18t");
    return 0;
}

Ubuntu:
image

Terminal (both PowerShell and Conhost):
image

and for Ubuntu test code:

#include <stdio.h>
#include <unistd.h>
#include <termios.h>

int main() {
    struct termios old_termios, new_termios;
    tcgetattr(STDIN_FILENO, &old_termios);
    new_termios = old_termios;
    new_termios.c_lflag &= ~(ICANON | ECHO);
    tcsetattr(STDIN_FILENO, TCSANOW, &new_termios);
    printf("\033[18t");
    int rows, cols;
    scanf("\033[8;%d;%dt", &rows, &cols);
    tcsetattr(STDIN_FILENO, TCSANOW, &old_termios);
    printf("Terminal size: %d columns x %d rows\n", cols, rows);
    return 0;
}

image

and for microsoft/terminal test code:

#include <stdio.h>
#include <windows.h>

int main() {
    HANDLE hConsole = GetStdHandle(STD_INPUT_HANDLE);
    DWORD prev_mode;
    GetConsoleMode(hConsole, &prev_mode);
    DWORD new_mode = prev_mode & ~(ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT);
    SetConsoleMode(hConsole, new_mode);
    printf("\033[18t");
    int rows, cols;
    scanf_s("\033[8;%d;%dt", &rows, &cols);
    SetConsoleMode(hConsole, prev_mode);
    printf("Terminal size: %d columns x %d rows\n", cols, rows);
    return 0;
}

image

Kindly please advise if this is the desired behavior.

@zadjii-msft
Copy link
Member

Honestly that looks right to me. Every shell is gonna do something different with an escape character they didn't expect to read. I'm under the impression that if you're emitthing the sequence to query the terminal, you're also expected to read the response. There's other issues scattered around where like, vim will ask the terminal for something, then ssh dies, and now the terminal replies to a shell that wasn't expecting it, and you get the same effect.

@zadjii-msft zadjii-msft moved this to Walkthrough in issue in Terminal Walkthroughs Apr 28, 2023
@michalnpl
Copy link
Contributor

michalnpl commented Apr 28, 2023

@zadjii-msft cool, is it OK for me to propose PR for this one, then? BTW, should I write a TAEF test for this?

@zadjii-msft
Copy link
Member

Please do! I'd file the PR with or without tests (though, tests are always appreciated 😉) ScreenBufferTests.cpp, ConptyRoundtripTests.cpp, maybe adapterTests.cpp are all good places that would likely work for a test

@michalnpl
Copy link
Contributor

I'll try to figure out the tests before I submit the PR.

@j4james
Copy link
Collaborator

j4james commented Apr 28, 2023

@michalnpl I just want to note something regarding the dimensions that this sequence should be reporting. @zadjii-msft previously suggested you should be using the viewport dimensions, but I suspect that apps are more likely to expect the dimensions of the addressable cell range, which for us is determined by the buffer width combined with the viewport height. The viewport width can potentially be narrower than the buffer width, but that's a user-configurable UI constraint. The only thing an application should care about is the point at which the text will wrap, which is the buffer width.

@michalnpl
Copy link
Contributor

michalnpl commented May 4, 2023

@zadjii-msft , @j4james, my best effort

#15295

couldn't figure out TAEF tests; if you can add it to PR, I would be in debt.

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label May 11, 2023
@github-project-automation github-project-automation bot moved this from Walkthrough in issue to Done in Terminal Walkthroughs May 11, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 11, 2023
DHowett pushed a commit that referenced this issue May 11, 2023
Adds support for CSI 18t to report the buffer screen size in characters.

This pull request adds support for **CSI 18t**. When submitted to the
terminal, it will respond with **"\033[8;{A};{B}t"** where **A** is
equal to the **height** and **B** is equal to the **width** of the
screen buffer in the number of characters (not pixels).

## Validation Steps Performed
Manual tests against PowerShell 7 and ConHost.
Added adapterTest

Closes #13944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants