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

Fix scrolling with SetConsoleWindowInfo #16334

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 17, 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

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Product-Conhost For issues in the Console codebase labels Nov 17, 2023
@@ -217,7 +217,7 @@ void Renderer::TriggerSystemRedraw(const til::rect* const prcDirtyClient)
// - <none>
void Renderer::TriggerRedraw(const Viewport& region)
{
auto view = _viewport;
Copy link
Member

Choose a reason for hiding this comment

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

HAH. But then, shouldn't we get rid of the entire cached viewport? We're just kicking the "torn state" can down the road...

Copy link
Member Author

@lhecker lhecker Nov 17, 2023

Choose a reason for hiding this comment

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

In the past scrolling was entirely implicit in the renderer as it computed the scroll and its invalidation just from the previous and current viewport. Nowadays we also got explicit scrolling, presumably for ConPTY edit: it was added with v2. Unfortunately (and similar to other parts here), the old code never got removed/unified and so we need to retain the behavior for both.

@zadjii-msft zadjii-msft merged commit 7a1b6f9 into main Nov 27, 2023
17 checks passed
@zadjii-msft zadjii-msft deleted the dev/lhecker/15932-SetConsoleWindowInfo-scrolling branch November 27, 2023 21:34
DHowett pushed a commit that referenced this pull request 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.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase
Projects
Development

Successfully merging this pull request may close these issues.

Conhost window is not refreshed after software scroll
3 participants