Don't reflow the alt buffer on resize by zadjii-msft · Pull Request #12719 · microsoft/terminal

zadjii-msft · 2022-03-18T13:07:04Z

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

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.

…er-terminal

…3-no-wrap-alt-buffer

…er-terminal

…3-no-wrap-alt-buffer

DHowett

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

terminal/src/cascadia/TerminalCore/Terminal.cpp

Line 696 in 098f6e3

_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?)

DHowett · 2022-04-13T23:25:09Z

src/cascadia/TerminalCore/Terminal.cpp

+    // viewport's size hasn't been updated yet. In that case, use the
+    // temporarily stashed _altBufferSize instead.
    const COORD origin{ 0, gsl::narrow<short>(_VisibleStartIndex()) };
+    const COORD size{ _inAltBuffer() ? _altBufferSize.to_win32_coord() :


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. :)

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.)

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.

DHowett · 2022-04-13T23:27:14Z

src/host/screenInfo.cpp

-
-                // If we happened to move the top of the window past
-                // the 0th row (first row in the buffer)
-                if (srNewViewport.Top < 0)


Don't totally understand why this disappeared

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.

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"!

…wrap-alt-buffer

github-actions · 2022-04-14T19:36:28Z

@check-spelling-bot Report

Unrecognized words, please review:

pullrequestreview
whech

Previously acknowledged words that are now absent

azurewebsites 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 xfg

To 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
on the dev/migrie/b/3493-no-wrap-alt-buffer branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/7f5caa1ba7888c6b1cde37041cb59af2d3e8984b.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/1099558967" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By 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.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 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 garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

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 string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

github-actions · 2022-04-14T19:38:23Z

@check-spelling-bot Report

Unrecognized words, please review:

pullrequestreview
whech

Previously acknowledged words that are now absent

azurewebsites 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 xfg

To 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
on the dev/migrie/b/3493-no-wrap-alt-buffer branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/7f5caa1ba7888c6b1cde37041cb59af2d3e8984b.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/1099560278" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By 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.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 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 garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

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 string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

DHowett

Spellbot hates it, but I think I love it.

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

DHowett · 2022-04-14T20:54:07Z

More importantly, it doesn't build on x86 :D

ghost · 2022-04-14T21:59:09Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

zadjii-msft · 2022-04-14T23:02:02Z

/azp run

azure-pipelines · 2022-04-14T23:02:12Z

Azure Pipelines failed to run 1 pipeline(s).

DHowett · 2022-04-15T00:09:43Z

Did we break CI with the Code Thing PR?

DHowett · 2022-04-15T01:57:48Z

/azp run

azure-pipelines · 2022-04-15T01:58:01Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2022-05-24T17:08:26Z

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 23 commits February 23, 2022 11:20

I think this is the conhost half of the changes

83466a4

This quite nearly implements everything for the Terminal half

e94e08c

Resizing is surely boned but this is 1000% percent better than nothing at all.

Most of the remaining todos, comments

dd702c7

one last comment

1209fa4

more comments are always good

bf24cdd

spel

dff1b94

fix tests

60d2c2e

I think this does the whole thing

57280d8

defer main buffer resizes till we exit, to try and prevent flashing i…

d08d216

…n conpty. That didn't work, unfortunately

deferred resizing both in the Terminal and the ConPTY

6529579

comments mostly

5d8c0d0

Stashing this for now. I think this does the save/restore cursor stuf…

54dc304

…f that we're supposed to do when entering/exiting the alt buffer

start writing tests

57094b7

make this test way more elaborate

69d0973

I thought this was a test for #12561 (comment), but I think that "jus…

0a1ed70

…t worked" because conpty magic.

More test cleanup. Make sure viewport doesnt move

9453aa5

Merge remote-tracking branch 'origin/main' into dev/migrie/b/alt-buff…

e658431

…er-terminal

Merge branch 'dev/migrie/b/alt-buffer-terminal' into dev/migrie/b/349…

4c96fc0

…3-no-wrap-alt-buffer

Merge remote-tracking branch 'origin/main' into dev/migrie/b/alt-buff…

0cfb463

…er-terminal

fine that's not a word

bf3d79e

thats not a word either

7237fce

notes from j4james

65b4571

Merge branch 'dev/migrie/b/alt-buffer-terminal' into dev/migrie/b/349…

c8f0a5e

…3-no-wrap-alt-buffer

This comment was marked as resolved.

Sign in to view

DHowett requested changes Apr 13, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 13, 2022

zadjii-msft added 7 commits April 14, 2022 08:50

Merge remote-tracking branch 'origin/main' into dev/migrie/b/3493-no-…

e14bba4

…wrap-alt-buffer

POC: You can test the input sent to the connection

6b89642

better, better, alright lets do this

bea906d

this is what you call test-driven-development

fe35515

this is the real test now

f6da747

tests work, which will make Dustin very happy

3360201

as discussed with dustin

23c6bf8

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 14, 2022

I actually had the wrong issue numbers

d157184

DHowett approved these changes Apr 14, 2022

View reviewed changes

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp Show resolved Hide resolved

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp Outdated Show resolved Hide resolved

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp Show resolved Hide resolved

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2022

it wouldn't be a mike PR without x86 breaks and spel check

93dbb9e

DHowett approved these changes Apr 14, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2022

ghost merged commit 41ef555 into main Apr 15, 2022

ghost deleted the dev/migrie/b/3493-no-wrap-alt-buffer branch April 15, 2022 02:33

ghost mentioned this pull request May 24, 2022

Consider not wrapping the alternate screen on resize #3493

Closed

j4james mentioned this pull request May 26, 2022

Terminal crashes when starting nano in bash when screen reader is enabled #13183

Closed

This pull request was closed.

	// 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;

Conversation

zadjii-msft commented Mar 18, 2022 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

DHowett left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

lhecker Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j4james Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

DHowett Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

lhecker Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

DHowett Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

@check-spelling-bot Report

Unrecognized words, please review:

Uh oh!

github-actions bot commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

@check-spelling-bot Report

Unrecognized words, please review:

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett commented Apr 14, 2022

Uh oh!

ghost commented Apr 14, 2022

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 (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

zadjii-msft commented Apr 14, 2022

Uh oh!

azure-pipelines bot commented Apr 14, 2022

Uh oh!

DHowett commented Apr 15, 2022

Uh oh!

DHowett commented Apr 15, 2022

Uh oh!

azure-pipelines bot commented Apr 15, 2022

Uh oh!

ghost commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zadjii-msft commented Mar 18, 2022 •

edited by DHowett

Loading

DHowett left a comment •

edited

Loading

lhecker Apr 13, 2022 •

edited

Loading

github-actions bot commented Apr 14, 2022 •

edited

Loading

github-actions bot commented Apr 14, 2022 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.