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

Conhost window is not refreshed after software scroll #15932

Closed
alabuzhev opened this issue Sep 5, 2023 · 3 comments · Fixed by #16334
Closed

Conhost window is not refreshed after software scroll #15932

alabuzhev opened this issue Sep 5, 2023 · 3 comments · Fixed by #16334
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase

Comments

@alabuzhev
Copy link
Contributor

alabuzhev commented Sep 5, 2023

Windows Terminal version

1.18.1462.0

Windows build number

10.0.19045.3324

Other Software

No response

Steps to reproduce

  1. Compile the code:
#include <iostream>
#include <stdexcept>
#include <string>

#include <windows.h>

void scroll(bool page, bool up)
{
	const auto out = GetStdHandle(STD_OUTPUT_HANDLE);
	CONSOLE_SCREEN_BUFFER_INFO csbi;
	if (!GetConsoleScreenBufferInfo(out, &csbi))
		throw std::runtime_error("GetConsoleScreenBufferInfo");

	if (up && !csbi.srWindow.Top)
		return;

	if (!up && csbi.srWindow.Bottom == csbi.dwSize.Y - 1)
		return;

	auto lines = page?
		csbi.srWindow.Bottom - csbi.srWindow.Top + 1:
		1;

	if (up)
		lines = -lines;

	csbi.srWindow.Top += lines;
	csbi.srWindow.Bottom += lines;

	if (csbi.srWindow.Top < 0)
	{
		csbi.srWindow.Bottom -= csbi.srWindow.Top;
		csbi.srWindow.Top = 0;
	}

	if (csbi.srWindow.Bottom >= csbi.dwSize.Y)
	{
		csbi.srWindow.Top -= (csbi.srWindow.Bottom - (csbi.dwSize.Y - 1));
		csbi.srWindow.Bottom = csbi.dwSize.Y - 1;
	}

	if (!SetConsoleWindowInfo(out, true, &csbi.srWindow))
		throw std::runtime_error("SetConsoleWindowInfo");
}

void run()
{
	const auto width = 80, height = 25;
	const auto screens = 5;
	const auto out = GetStdHandle(STD_OUTPUT_HANDLE);
	const SMALL_RECT Window{ 0, 0, width - 1, height - 1 };
	SetConsoleWindowInfo(out, true, &Window);
	SetConsoleScreenBufferSize(out, { width, height * screens });

	for (size_t i = 0; i != height * screens - 1; ++i)
	{
		const auto data = L"Line " + std::to_wstring(i) + L"\n";
		DWORD n;
		WriteConsole(out, data.data(), static_cast<DWORD>(data.size()), &n, {});
	}

	const auto in = GetStdHandle(STD_INPUT_HANDLE);
	for (;;)
	{
		INPUT_RECORD r;
		DWORD n;
		if (!ReadConsoleInput(in, &r, 1, &n))
			break;

		if (r.EventType != KEY_EVENT || !r.Event.KeyEvent.bKeyDown)
			continue;

		switch (const auto key = r.Event.KeyEvent.wVirtualKeyCode)
		{
		case VK_UP:
		case VK_DOWN:
		case VK_NEXT:
		case VK_PRIOR:
			scroll(key == VK_NEXT || key == VK_PRIOR, key == VK_UP || key == VK_PRIOR);
		}
	}
}

int main()
{
	try
	{
		run();
	}
	catch(const std::exception& e)
	{
		std::cerr << e.what();
	}
}
  1. Run it with the latest published OpenConsole, e.g. from WT 1.18.1462.
  2. Press Up and Down a few times.
  3. Press PgUp and PgDn a few times.

Expected Behavior

The viewport moves up or down, line by line or page by page.

Actual Behavior

Pressing Up or Down works as expected.
Pressing PgUp or PgDown doesn't.

When PgUp/PgDown are pressed, the scrollbar on the right moves appropriately, indicating that the viewport has moved, but the window is not refreshed and shows the very same content.

The issue occurs only if the number of lines to scroll is >= window height.

The original issue is described here:
FarGroup/FarManager#727
According to the comments, the default conhost in Windows 11 is also affected to some extent.

@alabuzhev alabuzhev 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 5, 2023
@alabuzhev alabuzhev changed the title Conhost window not in refreshed after software scroll Conhost window is not refreshed after software scroll Sep 5, 2023
@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) labels Sep 6, 2023
@carlos-zamora carlos-zamora removed Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 6, 2023
@zadjii-msft zadjii-msft added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 6, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.19 milestone Sep 6, 2023
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 6, 2023
@lhecker
Copy link
Member

lhecker commented Sep 6, 2023

FYI #15769 will be closed by #15935, but that's just because I'm effectively working around this bug. The timing of you filing this issue is great, because it just made me realize that the cause for #15769 is not SSH's handling of SIGWINCH directly, but rather SetConsoleWindowInfo itself.

@lhecker
Copy link
Member

lhecker commented Nov 17, 2023

This regression was caused by 81b7e54. Reverting it has no or negligible impact on performance at this point, likely due to the overall vastly better performance of conhost nowadays.

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Nov 17, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Nov 27, 2023
github-actions bot pushed a commit to johnterickson/terminal that referenced this issue Nov 27, 2023
81b7e54 caused a regression in `SetConsoleWindowInfo` and any other
function that used the `WriteToScreen` helper. This is because it
assumes that it can place the viewport anywhere randomly and it was
written at a time where `TriggerScroll` didn't exist yet (there was
no need for that (also not today, but that's being worked on)). 

Caching the viewport meant that `WriteToScreen`'s call to
`TriggerRedraw` would pick up the viewport from the last rendered
frame, which would cause the intersection of both to be potentially
empty and nothing to be drawn on the screen.

This commit reverts 81b7e54 as I found that it has no or negligible
impact on performance at this point, likely due to the overall
vastly better performance of conhost nowadays.

Closes microsoft#15932

## Validation Steps Performed
* Scroll the viewport by entire pages worth of content using
  `SetConsoleWindowInfo` - see microsoft#15932
* The screen and scrollbars update immediately ✅
DHowett pushed a commit that referenced this issue Dec 4, 2023
81b7e54 caused a regression in `SetConsoleWindowInfo` and any other
function that used the `WriteToScreen` helper. This is because it
assumes that it can place the viewport anywhere randomly and it was
written at a time where `TriggerScroll` didn't exist yet (there was
no need for that (also not today, but that's being worked on)).

Caching the viewport meant that `WriteToScreen`'s call to
`TriggerRedraw` would pick up the viewport from the last rendered
frame, which would cause the intersection of both to be potentially
empty and nothing to be drawn on the screen.

This commit reverts 81b7e54 as I found that it has no or negligible
impact on performance at this point, likely due to the overall
vastly better performance of conhost nowadays.

Closes #15932

## Validation Steps Performed
* Scroll the viewport by entire pages worth of content using
  `SetConsoleWindowInfo` - see #15932
* The screen and scrollbars update immediately ✅

(cherry picked from commit 7a1b6f9)
Service-Card-Id: 91152167
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants