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

Add support for CSI 18t #15295

Merged
merged 15 commits into from
May 11, 2023
Merged

Conversation

michalnpl
Copy link
Contributor

@michalnpl michalnpl commented May 4, 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

@michalnpl

This comment was marked as resolved.

@michalnpl

This comment was marked as resolved.

@michalnpl michalnpl marked this pull request as ready for review May 4, 2023 21:34
@michalnpl
Copy link
Contributor Author

There are no TAEF tests. If you can add tests to this PR, I would greatly appreciate it so I can learn.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think we'll be swamped getting 1.18 out the door, so reviews might be slow.

You might be able to do something like

void ScreenBufferTests::TestGetWindowSizeInVt()
{
    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
    auto& si = gci.GetActiveOutputBuffer();
    auto& stateMachine = si.GetStateMachine();
    auto& cursor = si.GetTextBuffer().GetCursor();

    stateMachine.ProcessString(L"\x1b[18t");
    
    
    gci.pInputBuffer.Read(...);

and then check the Char of each of those key events.

Sorry I'm not more help ATM. Maybe after we sign off on 1.18 I can help more. We apparently didn't have tests like this, so I don't blame you for not writing a test for this one. I don't think there is prior art

@@ -3068,6 +3068,13 @@ bool AdaptDispatch::WindowManipulation(const DispatchTypes::WindowManipulationTy
case DispatchTypes::WindowManipulationType::ResizeWindowInCharacters:
_api.ResizeWindow(parameter2.value_or(0), parameter1.value_or(0));
return true;
case DispatchTypes::WindowManipulationType::ReportTextSizeInCharacters:
{
const til::rect viewport = _api.GetViewport();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to say that this is okay, but IIRC @j4james thought we should use the buffer width, not just the viewport width here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adressed.

@michalnpl
Copy link
Contributor Author

michalnpl commented May 5, 2023

@zadjii-msft @j4james I addressed:

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.

with and tested manually (still no TAEF):

image

@zadjii-msft
No worries; maybe by the time you get to this one, I can figure out the tests 🙃

@j4james
Copy link
Collaborator

j4james commented May 5, 2023

Tests for sequences like this are typically handled in AdapterTest. Just search for anything using ValidateInputEvent.

For example, this is a test case for the DECRQSS margin query, which first sets up the viewport height to the range it's expecting to see in the query response.

Log::Comment(L"Requesting DECSTBM margins (full screen).");
_testGetSet->PrepData();
// Set screen height to 25 - this will be the expected margin range.
_testGetSet->_viewport.bottom = _testGetSet->_viewport.top + 25;
_pDispatch->SetTopBottomScrollingMargins(0, 0);
requestSetting(L"r");
_testGetSet->ValidateInputEvent(L"\033P1$r1;25r\033\\");

That seems similar to the sort of thing you might want to do. And note that you can access the text buffer the same way as the viewport with _testGetSet->_textBuffer, although I'm not sure about resizing it. You could just assume a buffer width of 100 for your tests, since that's what the PrepData function initializes it with by default (see here).

@michalnpl
Copy link
Contributor Author

michalnpl commented May 8, 2023

I've added a TAEF test for this. Thank you, @zadjii-msft and @j4james, for guiding me in the right direction.

Comment on lines 3073 to 3074
const std::wstring textSize = fmt::format(L"\033[8;{};{}t", _api.GetViewport().height(), _api.GetTextBuffer().GetSize().Width());
_api.ReturnResponse(std::wstring_view(textSize));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think that std::wstring should automatically cast to std::wstring_view, so you shouldn't need an explicit cast on the textSize parameter. But with that out the way, you might as well embed the fmt::format call directly within the ReturnResponse call and avoid the textSize variable altogether. I suspect we're doing the same thing in a bunch of other places, though, so it's not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

Copy link
Collaborator

@j4james j4james left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't officially approve anything, but this looks good to me. Thanks.

@zadjii-msft zadjii-msft changed the title Michalnpl/csi 18t support Add support for CSI 18t May 10, 2023
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bearing with to write the test!

@zadjii-msft
Copy link
Member

Screenshots from body:

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.

@michalnpl
Copy link
Contributor Author

Going to need another approver to merge this, as @j4james mentioned he could not approve anything officially.

@@ -3058,6 +3058,9 @@ bool AdaptDispatch::WindowManipulation(const DispatchTypes::WindowManipulationTy
case DispatchTypes::WindowManipulationType::ResizeWindowInCharacters:
_api.ResizeWindow(parameter2.value_or(0), parameter1.value_or(0));
return true;
case DispatchTypes::WindowManipulationType::ReportTextSizeInCharacters:
_api.ReturnResponse(fmt::format(L"\033[8;{};{}t", _api.GetViewport().height(), _api.GetTextBuffer().GetSize().Width()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: all of the other uses of fmt in this file use FMT_COMPILE for the format string. That keeps the code size down when this is compiled into Windows' conhost.exe.

Would you mind doing the same? 😄

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 10, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 11, 2023
@zadjii-msft zadjii-msft requested a review from DHowett May 11, 2023 10:29
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I going crazy, or did Refined GitHub change the order of the signoff buttons?

@DHowett DHowett merged commit b2dd7fa into microsoft:main May 11, 2023
@michalnpl michalnpl deleted the michalnpl/CSI_18t_support branch May 15, 2023 22:28
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 Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSI 18t "Report Size In Characters" not working.
4 participants