-
Notifications
You must be signed in to change notification settings - Fork 8.9k
Fix #19390: Make snap-on-input conditional to prevent blocking programmatic scroll #19414
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
Conversation
|
@microsoft-github-policy-service agree |
lhecker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OOTL but this makes sense to me. 👍
src/host/screenInfo.hpp
Outdated
| void SetCursorType(const CursorType Type, const bool setMain = false) noexcept; | ||
|
|
||
| void SetSnapOnInputEnabled(const bool enabled) noexcept { _snapOnInputEnabled = enabled; } | ||
| [[nodiscard]] bool IsSnapOnInputEnabled() const noexcept { return _snapOnInputEnabled; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency I think these should go into the .cpp file (they aren't templates).
src/host/screenInfo.cpp
Outdated
|
|
||
| void SCREEN_INFORMATION::MakeCursorVisible(til::point position) | ||
| { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra.
src/host/input.cpp
Outdated
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra.
|
@lhecker qq about your thought process here - do you have a gut feeling about introducing new (possibly torn) state boolean rather than just the input mode directly? |
DHowett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(want to noodle about this a little bit, sorry!)
|
Oh right I didn't think of that, since the input mode is stored elsewhere. Regarding duplicating it: my thought was that if we were to add a user setting for this (hypothetically), I would make it a bitfield, similar to the new "cursor inhibitors". For now, I suppose it makes sense to just ask the input mode. |
|
I think I just chose the long way to do it. There can be another way, like this: Just change the file if (WI_IsFlagSet(buffer.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING))
{
buffer.SnapOnInput(keyEvent.wVirtualKeyCode);
} |
|
Do you want me to remove all of that and prefer this change... |
Yes, that's what Dustin meant with his comment. I think we should do that. |
beef806 to
0485501
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
(extracted from PR body) MicrosoftScreenFix.mp4 |
carlos-zamora
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and simple. Thanks! 🙂 Sorry for the delays!
Looks like the original concern was regarding a possible torn state (see #19414 (comment))
New implementation removed that issue
…mmatic scroll (#19414) ## Summary of the Pull Request This PR fixes a bug where programmatic scrolling would get stuck. The fix makes the "snap-on-input" feature conditional, activating it only for modern applications that use Virtual Terminal (VT) processing. This restores correct scrolling behavior for legacy applications without removing the feature for new ones. ## References and Relevant Issues Fixes #19390: OpenConsole: Cursor visibility prevents programmatic scrolling ## Detailed Description of the Pull Request / Additional comments The "snap-on-input" feature introduced in a previous PR caused an unintended side effect for older console programs that use the SetConsoleWindowInfo API to manage their own viewport. When such a program tried to scroll using a key press, the snap feature would immediately pull the view back to the cursor's position, causing the screen to flicker and get stuck. This fix makes the snap-on-input feature smarter by checking the application's mode first. ## Validation Steps Performed Compiled the minimal C++ reproduction case from issue #19390. Ran the test executable inside the newly built OpenConsole.exe. Confirmed that scrolling with the Up/Down arrow keys now works correctly, even with a visible cursor. The view no longer flickers or gets stuck when the cursor moves outside the viewport. Closes #19390 (cherry picked from commit c28610d) Service-Card-Id: PVTI_lADOAF3p4s4BBcTlzgfZNLg Service-Version: 1.24
Summary of the Pull Request
This PR fixes a bug where programmatic scrolling would get stuck. The fix makes the "snap-on-input" feature conditional, activating it only for modern applications that use Virtual Terminal (VT) processing. This restores correct scrolling behavior for legacy applications without removing the feature for new ones.
References and Relevant Issues
Fixes #19390: OpenConsole: Cursor visibility prevents programmatic scrolling
Detailed Description of the Pull Request / Additional comments
The "snap-on-input" feature introduced in a previous PR caused an unintended side effect for older console programs that use the SetConsoleWindowInfo API to manage their own viewport. When such a program tried to scroll using a key press, the snap feature would immediately pull the view back to the cursor's position, causing the screen to flicker and get stuck.
This fix makes the snap-on-input feature smarter by checking the application's mode first.
Validation Steps Performed
Compiled the minimal C++ reproduction case from issue #19390.
Ran the test executable inside the newly built OpenConsole.exe.
Confirmed that scrolling with the Up/Down arrow keys now works correctly, even with a visible cursor. The view no longer flickers or gets stuck when the cursor moves outside the viewport.
Closes #19390