-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Don't reflow the alt buffer on resize #12719
Conversation
Resizing is surely boned but this is 1000% percent better than nothing at all.
…n conpty. That didn't work, unfortunately
…f that we're supposed to do when entering/exiting the alt buffer
…t worked" because conpty magic.
…3-no-wrap-alt-buffer
…3-no-wrap-alt-buffer
This comment was marked as resolved.
This comment was marked as resolved.
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 inclined to trust the resizing code in conhost here, even though it scares me. I don't love that we have to reimplement part of ResizeWindow...
However, there's two concerning uses of _mutableViewport
after you've fully decoupled them:
mouse event clamping
_mutableViewport.ToOrigin().Clamp(clampedPos); |
this whole block in AdjustCursorPosition
terminal/src/cascadia/TerminalCore/Terminal.cpp
Lines 1117 to 1128 in 098f6e3
// our _scrollOffset | |
if (!_inAltBuffer() && (updatedViewport || newRows != 0)) | |
{ | |
const auto oldScrollOffset = _scrollOffset; | |
// scroll if... | |
// - no selection is active | |
// - viewport is already at the bottom | |
const bool scrollToOutput = !IsSelectionActive() && _scrollOffset == 0; | |
_scrollOffset = scrollToOutput ? 0 : _scrollOffset + scrollAmount + newRows; | |
Are those worth worrying about? I can imagine a world where we make the window wider in the alt buffer but mouse events are clamped to the smaller main buffer and fail to work properly after resize.
(Do we have tests covering this?)
const COORD origin{ 0, gsl::narrow<short>(_VisibleStartIndex()) }; | ||
const COORD size{ _inAltBuffer() ? _altBufferSize.to_win32_coord() : |
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.
This is almost aching for another helper ... or, seems like you could use the Dimensions off the GetMutableViewport helper's return value? I'd prefer concentrating the magic in one place. :)
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.
Can't we make a til::rect _viewports[2]
and index into it with a bool _altBufferInUse
?
Like... _viewports[_altBufferInUse].to_win32_coord()
or something like that...
(I didn't say anything since we're already stretched thin as is.)
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.
Bear in mind that we'll probably need to support more buffers than just main and alt in the long term. So I'd expect properties like this would ultimately be part of the text buffer itself, and then you could just do something like _activeBuffer().mutableViewport
. But we also need to get the architecture synced up with conhost, which currently manages this sort of state in the SCREEN_INFORMATION
class. Not that I'm saying this is essential for now, but if you're trying to clean up the code, it's best to also consider how we are likely to extend this in the future.
|
||
// If we happened to move the top of the window past | ||
// the 0th row (first row in the buffer) | ||
if (srNewViewport.Top < 0) |
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.
Don't totally understand why this disappeared
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 removed that code while debugging the crash issue.
Check out my commit here: 4c1404d
(#12719)
Line 1227-1231 it says:
if (srNewViewport.Top < 0)
{
srNewViewport.Bottom -= srNewViewport.Top;
srNewViewport.Top = 0;
}
That if condition makes the removed code with its fail-fast, etc. redundant.
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 didn't look at that block you introduced below as being related since the comment said "off the right" instead of "off the bottom"!
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentazurewebsites Consolescreen css cxcy DCompile debolden deconstructed devicefamily GETKEYSTATE guardxfg LLVM LPCHARSETINFO MAPVIRTUALKEY MSDL ned newcursor NOWAIT pgorepro pgort PGU redistributable Timeline timelines unintense UWA UWAs VKKEYSCAN WResult xfgTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the [email protected]:microsoft/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentazurewebsites Consolescreen css cxcy DCompile debolden deconstructed devicefamily GETKEYSTATE guardxfg LLVM LPCHARSETINFO MAPVIRTUALKEY MSDL ned newcursor NOWAIT pgorepro pgort PGU redistributable Timeline timelines unintense UWA UWAs VKKEYSCAN WResult xfgTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the [email protected]:microsoft/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
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.
Spellbot hates it, but I think I love it.
More importantly, it doesn't build on x86 :D |
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
/azp run |
Azure Pipelines failed to run 1 pipeline(s). |
Did we break CI with the Code Thing PR? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
🎉 Handy links: |
VTE only rewraps the contents of the (normal screen + its scrollback
buffer) on a resize event. It doesn't rewrap the contents of the
alternate screen. The alternate screen is used by applications which
repaint it after a resize event. So, it doesn't really matter. However,
in that short time window, after resizing the terminal but before the
application catches up, this prevents vertical lines
It was really hard to get a gif of this where it happened and was small
enough to upload to GH, but there is one in #12719.
There's something in this branch that fixes a scrolling issue in the
parent PR. I'm partially filing this so I can look at the diffs here and
try and figure out what that is. I kinda want to just take all 3 alt
buffer PRs as a single atomic unit, but splitting them up made sense
from a review standpoint.
Closes #3493