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

When the Terminal is resized, don't remove lines from scrollback #4354

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
b5c8c85
let's first move reflowing to the text buffer
zadjii-msft Jan 13, 2020
9aec694
add a doc comment because I'm not a barbarian
zadjii-msft Jan 13, 2020
1a2654d
Try to wrap the line properly with conpty
zadjii-msft Jan 13, 2020
aae6ce6
This works, fixing the ECH at end of wrapped line bug
zadjii-msft Jan 14, 2020
416be46
This is some cleanup, almost ready for PR but I need to write tests w…
zadjii-msft Jan 14, 2020
4319589
start by authoring a simple test class
zadjii-msft Jan 15, 2020
399b002
This seems to fix it?
zadjii-msft Jan 15, 2020
38ebbb6
This is good, but when we resize down quickly, sometimes a line gets …
zadjii-msft Jan 16, 2020
c8c794f
Horrifyingly try to not increment the buffer in the middle of multipl…
zadjii-msft Jan 16, 2020
ee08a5b
Revert "Horrifyingly try to not increment the buffer in the middle of…
zadjii-msft Jan 16, 2020
95122e3
I thought I could make some conpty changes to fix this, but I'm not s…
zadjii-msft Jan 16, 2020
17d25ee
I thought maybe without this it would work better but it doesn't
zadjii-msft Jan 16, 2020
668bad3
I couldn't tell you want all I've done but it _works_
zadjii-msft Jan 16, 2020
9ccc3bc
Merge branch 'master' into dev/migrie/b/3490-resize-down
zadjii-msft Jan 17, 2020
d116c6b
cleanup some dead code, add comments
zadjii-msft Jan 17, 2020
3398b19
Merge remote-tracking branch 'origin/master' into dev/migrie/b/3490-r…
zadjii-msft Jan 17, 2020
2e9ebcc
initial roundtrip test draft
zadjii-msft Jan 17, 2020
c6a7e4e
Update the tests, but something's not right
zadjii-msft Jan 17, 2020
872f4c4
Stash this for the weekend
zadjii-msft Jan 17, 2020
f37ce01
move the cursor back down in the vt renderer, to keep it roughly alig…
zadjii-msft Jan 22, 2020
252f466
woo all the tests pass
zadjii-msft Jan 22, 2020
02445fd
lots of cleanup
zadjii-msft Jan 22, 2020
13c6171
add more notes, cleanup
zadjii-msft Jan 22, 2020
48de202
I think this fixes the bug, for maximizing, but not for the test?
zadjii-msft Jan 22, 2020
3c4d157
Merge remote-tracking branch 'origin/master' into dev/migrie/b/3490-r…
zadjii-msft Jan 22, 2020
c091471
okay this fixes the test to work too
zadjii-msft Jan 22, 2020
c44bc9e
Try a potentially simpler solution
zadjii-msft Jan 23, 2020
2fe6d8e
well this makes most of the tests pass
zadjii-msft Jan 23, 2020
cd82017
All the tests pass, it's finally happening
zadjii-msft Jan 24, 2020
26a16d4
some cleanup for the terminal core
zadjii-msft Jan 24, 2020
f561f7f
Okay this adds _more_ variables to the tests
zadjii-msft Jan 24, 2020
8d855ed
woo boy all these tests pass now
zadjii-msft Jan 24, 2020
7bae9ec
that's right, all 60 tests pass
zadjii-msft Jan 24, 2020
8c61481
I've got a crazy idea, we dont need to invalidate the entire view, ju…
zadjii-msft Jan 24, 2020
4248c87
Merge branch 'dev/migrie/f/conpty-wrapping-003' into dev/migrie/b/349…
zadjii-msft Jan 24, 2020
94559ed
Oh we need this too
zadjii-msft Jan 24, 2020
be66829
Revert "Merge branch 'dev/migrie/f/conpty-wrapping-003' into dev/migr…
zadjii-msft Jan 24, 2020
715014d
Outstanding cleanup for PR
zadjii-msft Jan 24, 2020
8aa42f4
This comment should be removed.
zadjii-msft Jan 24, 2020
191952f
Merge remote-tracking branch 'origin/master' into dev/migrie/b/3490-r…
zadjii-msft Jan 27, 2020
2d07816
Hopefully this should fix the SA build
zadjii-msft Jan 27, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,40 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting
return S_FALSE;
}

const auto dy = viewportSize.Y - oldDimensions.Y;

// We're going to attempt to "stick to the top" of where the old viewport was.
const auto oldTop = _mutableViewport.Top();

const short newBufferHeight = viewportSize.Y + _scrollbackLines;
COORD bufferSize{ viewportSize.X, newBufferHeight };
RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize));

auto proposedTop = oldTop;
const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);
// However conpty resizes a little oddly - if the height decreased, and
// there were blank lines at the bottom, those lines will get trimmed.
// If there's not blank lines, then the top will get "shifted down",
// moving the top line into scrollback.
// See GH#3490 for more details.

// If the final position in the buffer is on the bottom row of the new
// viewport, then we're going to need to move the top down. Otherwise,
// move the bottom up.
const COORD cOldCursorPos = _buffer->GetCursor().GetPosition();
Copy link
Member

Choose a reason for hiding this comment

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

Avoid hungarian

COORD cOldLastChar = cOldCursorPos;
try
{
cOldLastChar = _buffer->GetLastNonSpaceCharacter();
}
CATCH_LOG();

const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y);

const bool beforeLastRow = maxRow < bufferSize.Y - 1;
const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy);

auto proposedTop = oldTop + adjustment;
Copy link
Member

Choose a reason for hiding this comment

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

Is this math a candidate for the new base::ClampedAdd or base::CheckedAdd? Looks like it might be. If oldTop and adjustment are SHORT or you declare SHORT proposedTop, I think those could help you clamp/check it to be in bounds or set a reasonable default if it goes past and avoid the need to gsl::narrow_cast<SHORT> unchecked below.


const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);
const auto proposedBottom = newView.BottomExclusive();
// If the new bottom would be below the bottom of the buffer, then slide the
// top up so that we'll still fit within the buffer.
Expand All @@ -192,7 +218,8 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting
proposedTop -= (proposedBottom - bufferSize.Y);
}

_mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);
_mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);

_scrollOffset = 0;
_NotifyScrollEvent();

Expand Down
9 changes: 7 additions & 2 deletions src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ namespace Microsoft::Terminal::Core

// fwdecl unittest classes
#ifdef UNIT_TESTING
class ConptyRoundtripTests;
namespace TerminalCoreUnitTests
{
class TerminalBufferTests;
class ConptyRoundtripTests;
};
#endif

class Microsoft::Terminal::Core::Terminal final :
Expand Down Expand Up @@ -252,6 +256,7 @@ class Microsoft::Terminal::Core::Terminal final :
#pragma endregion

#ifdef UNIT_TESTING
friend class ::ConptyRoundtripTests;
friend class TerminalCoreUnitTests::TerminalBufferTests;
friend class TerminalCoreUnitTests::ConptyRoundtripTests;
#endif
};
Loading