Skip to content

Fix interpretation of DECSTBM margin parameters #1881

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.

Sign up for GitHub

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

Merged
miniksa merged 3 commits into from
Jul 10, 2019
Merged

Fix interpretation of DECSTBM margin parameters #1881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc9a728
Fix DECSTBM parameter interpretation to ignore invalid ranges, and cl…
j4james Jul 6, 2019
6c8022c
Add additional scroll margin adapter tests to verify the parameter co…
j4james Jul 6, 2019
a3ad8c0
Fix scroll margin adapter tests that weren't actually verifying the c…
j4james Jul 6, 2019
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
49 changes: 23 additions & 26 deletions src/terminal/adapter/adaptDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,39 +1332,36 @@ bool AdaptDispatch::_DoSetTopBottomScrollingMargins(const SHORT sTopMargin,
SHORT sActualTop = sTopMargin;
SHORT sActualBottom = sBottomMargin;
SHORT sScreenHeight = csbiex.srWindow.Bottom - csbiex.srWindow.Top;
if (sActualTop == 0 && sActualBottom == 0)
// The default top margin is line 1
if (sActualTop == 0)
{
// Disable Margins
// This case is valid, and nothing changes.
sActualTop = 1;
}
else if (sActualBottom == 0)
// The default bottom margin is the screen height
if (sActualBottom == 0)
{
sActualBottom = sScreenHeight;
}
else if (sActualBottom < sActualTop)
{
fSuccess = false;
}
else if ((sActualTop == 0 || sActualTop == 1) && sActualBottom == sScreenHeight)
{
// Client requests setting margins to the entire screen
// - clear them instead of setting them.
// This is for apps like `apt` (NOT `apt-get` which set scroll
// margins, but don't use the alt buffer.)
// Some apps will use 0 as a top, some will use 1. Both should behave the same.
sActualBottom = 0;
}
// In VT, the origin is 1,1. For our array, it's 0,0. So subtract 1.
if (sActualTop > 0)
{
sActualTop -= 1;
}
if (sActualBottom > 0)
{
sActualBottom -= 1;
}
// The top margin must be less than the bottom margin, and the
// bottom margin must be less than or equal to the screen height
fSuccess = (sActualTop < sActualBottom && sActualBottom <= sScreenHeight);
if (fSuccess)
{
if (sActualTop == 1 && sActualBottom == sScreenHeight)
{
// Client requests setting margins to the entire screen
// - clear them instead of setting them.
// This is for apps like `apt` (NOT `apt-get` which set scroll
// margins, but don't use the alt buffer.)
sActualTop = 0;
sActualBottom = 0;
}
else
{
// In VT, the origin is 1,1. For our array, it's 0,0. So subtract 1.
sActualTop -= 1;
sActualBottom -= 1;
}
_srScrollMargins.Top = sActualTop;
_srScrollMargins.Bottom = sActualBottom;
fSuccess = !!_conApi->PrivateSetScrollingRegion(&_srScrollMargins);
Expand Down
38 changes: 34 additions & 4 deletions src/terminal/adapter/ut_adapter/adapterTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3173,6 +3173,7 @@ class AdapterTest
_testGetSet->_srViewport.Right = 8;
_testGetSet->_srViewport.Bottom = 8;
_testGetSet->_fGetConsoleScreenBufferInfoExResult = TRUE;
SHORT sScreenHeight = _testGetSet->_srViewport.Bottom - _testGetSet->_srViewport.Top;

Log::Comment(L"Test 1: Verify having both values is valid.");
_testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6);
Expand Down Expand Up @@ -3206,27 +3207,56 @@ class AdapterTest
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
VERIFY_IS_FALSE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));

Log::Comment(L"Test 6: Verify Setting margins to (0, height) clears them");
Log::Comment(L"Test 6: Verify setting margins to (0, height) clears them");
// First set,
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
_testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6);
VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));
// Then clear
_testGetSet->_SetMarginsHelper(&srTestMargins, 0, sScreenHeight);
_testGetSet->_srExpectedScrollRegion.Top = 0;
_testGetSet->_srExpectedScrollRegion.Bottom = 0;
_testGetSet->_SetMarginsHelper(&srTestMargins, 0, 7);
VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));

Log::Comment(L"Test 7: Verify Setting margins to (1, height) clears them");
Log::Comment(L"Test 7: Verify setting margins to (1, height) clears them");
// First set,
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
_testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6);
VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));
// Then clear
_testGetSet->_SetMarginsHelper(&srTestMargins, 1, sScreenHeight);
_testGetSet->_srExpectedScrollRegion.Top = 0;
_testGetSet->_srExpectedScrollRegion.Bottom = 0;
VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));

Log::Comment(L"Test 8: Verify setting margins to (1, 0) clears them");
// First set,
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
_testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6);
VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));
// Then clear
_testGetSet->_SetMarginsHelper(&srTestMargins, 1, 0);
_testGetSet->_srExpectedScrollRegion.Top = 0;
_testGetSet->_srExpectedScrollRegion.Bottom = 0;
_testGetSet->_SetMarginsHelper(&srTestMargins, 0, 7);
VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));

Log::Comment(L"Test 9: Verify having top and bottom margin the same is invalid.");

_testGetSet->_SetMarginsHelper(&srTestMargins, 4, 4);
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
VERIFY_IS_FALSE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));

Log::Comment(L"Test 10: Verify having top margin out of bounds is invalid.");

_testGetSet->_SetMarginsHelper(&srTestMargins, sScreenHeight + 1, sScreenHeight + 10);
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
VERIFY_IS_FALSE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));

Log::Comment(L"Test 11: Verify having bottom margin out of bounds is invalid.");

_testGetSet->_SetMarginsHelper(&srTestMargins, 1, sScreenHeight + 1);
_testGetSet->_fPrivateSetScrollingRegionResult = TRUE;
VERIFY_IS_FALSE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.Top, srTestMargins.Bottom));
}

TEST_METHOD(TabSetClearTests)
Expand Down