-
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
Correct fill attributes when scrolling and erasing #3100
Merged
Merged
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
4c09e68
Add private scroll region API with support for standard erase attribu…
j4james 093b94f
Update scrolling operations to use the new API that sets the fill att…
j4james b41d2ff
Update existing screen buffer tests to check that the scrolling opera…
j4james 6f2ef8f
Update the line feed handling to set the fill attributes correctly fo…
j4james 6aa3e97
Update and add screen buffer tests to check that line feeds initializ…
j4james 32321a9
Add private fill region API with support for standard erase attributes.
j4james ba1e16f
Update the erase operations to use the new API that sets the fill att…
j4james a83fd64
Update the VtEraseAll implementation to use the correct fill attribut…
j4james 480a924
Update the EraseScrollback implementation to use the new APIs that se…
j4james d85c7e2
Replace the adapter tests with more appropriate screen buffer tests f…
j4james ff8a329
Remove the unneeded FillConsoleOutputCharacterW, FillConsoleOutputAtt…
j4james 655184c
Removed the unneeded hacks for handling legacy default colors in the …
j4james 11b6644
Only clear the meta attributes while incrementing the circular buffer…
j4james e5c273d
Initialize the alt buffer with the standard erase attributes.
j4james d03f24a
Generate an accessibility update event in the DoSrvPrivateFillRegion …
j4james 3099d1a
Make sure we're resetting both the extended and meta attributes for s…
j4james 786cc36
Add a SetStandardErase method to the TextAttribute class to simplify …
j4james 171a4f9
Update the comments to document the new argument in the IncrementCirc…
j4james File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -836,32 +836,6 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont | |
auto& buffer = context.GetActiveBuffer(); | ||
|
||
TextAttribute useThisAttr(fillAttribute); | ||
|
||
// Here we're being a little clever - similar to FillConsoleOutputAttributeImpl | ||
// Because RGB/default color can't roundtrip the API, certain VT | ||
// sequences will forget the RGB color because their first call to | ||
// GetScreenBufferInfo returned a legacy attr. | ||
// If they're calling this with the legacy attrs version of our current | ||
// attributes, they likely wanted to use the full version of | ||
// our current attributes, whether that be RGB or _default_ colored. | ||
// This could create a scenario where someone emitted RGB with VT, | ||
// THEN used the API to ScrollConsoleOutput with the legacy attrs, | ||
// and DIDN'T want the RGB color. As in FillConsoleOutputAttribute, | ||
// this scenario is highly unlikely, and we can reasonably do this | ||
// on their behalf. | ||
// see MSFT:19853701 | ||
|
||
if (buffer.InVTMode()) | ||
{ | ||
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); | ||
const auto currentAttributes = buffer.GetAttributes(); | ||
const auto bufferLegacy = gci.GenerateLegacyAttributes(currentAttributes); | ||
if (bufferLegacy == fillAttribute) | ||
{ | ||
useThisAttr = currentAttributes; | ||
} | ||
} | ||
|
||
ScrollRegion(buffer, source, clip, target, fillCharacter, useThisAttr); | ||
|
||
return S_OK; | ||
|
@@ -1422,25 +1396,12 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool | |
coordDestination.X = 0; | ||
coordDestination.Y = viewport.Top + 1; | ||
|
||
// Here we previously called to ScrollConsoleScreenBufferWImpl to | ||
// perform the scrolling operation. However, that function only | ||
// accepts a WORD for the fill attributes. That means we'd lose | ||
// 256/RGB fidelity for fill attributes. So instead, we'll just call | ||
// ScrollRegion ourselves, with the same params that | ||
// ScrollConsoleScreenBufferWImpl would have. | ||
// See microsoft/terminal#832, #2702 for more context. | ||
try | ||
{ | ||
LockConsole(); | ||
auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); | ||
ScrollRegion(screenInfo, | ||
srScroll, | ||
srScroll, | ||
coordDestination, | ||
UNICODE_SPACE, | ||
screenInfo.GetAttributes()); | ||
} | ||
CATCH_LOG(); | ||
// Note the revealed lines are filled with the standard erase attributes. | ||
Status = NTSTATUS_FROM_HRESULT(DoSrvPrivateScrollRegion(screenInfo, | ||
srScroll, | ||
srScroll, | ||
coordDestination, | ||
true)); | ||
} | ||
} | ||
return Status; | ||
|
@@ -2149,25 +2110,12 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert) | |
coordDestination.Y = (cursorPosition.Y) - gsl::narrow<short>(count); | ||
} | ||
|
||
// Here we previously called to ScrollConsoleScreenBufferWImpl to | ||
// perform the scrolling operation. However, that function only accepts | ||
// a WORD for the fill attributes. That means we'd lose 256/RGB fidelity | ||
// for fill attributes. So instead, we'll just call ScrollRegion | ||
// ourselves, with the same params that ScrollConsoleScreenBufferWImpl | ||
// would have. | ||
// See microsoft/terminal#832 for more context. | ||
try | ||
{ | ||
LockConsole(); | ||
auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); | ||
ScrollRegion(screenInfo, | ||
srScroll, | ||
srScroll, | ||
coordDestination, | ||
UNICODE_SPACE, | ||
screenInfo.GetAttributes()); | ||
} | ||
CATCH_LOG(); | ||
// Note the revealed lines are filled with the standard erase attributes. | ||
LOG_IF_FAILED(DoSrvPrivateScrollRegion(screenInfo, | ||
srScroll, | ||
srScroll, | ||
coordDestination, | ||
true)); | ||
|
||
// The IL and DL controls are also expected to move the cursor to the left margin. | ||
// For now this is just column 0, since we don't yet support DECSLRM. | ||
|
@@ -2291,3 +2239,101 @@ void DoSrvPrivateMoveToBottom(SCREEN_INFORMATION& screenInfo) | |
} | ||
CATCH_RETURN(); | ||
} | ||
|
||
// Routine Description: | ||
// - A private API call for filling a region of the screen buffer. | ||
// Arguments: | ||
// - screenInfo - Reference to screen buffer info. | ||
// - startPosition - The position to begin filling at. | ||
// - fillLength - The number of characters to fill. | ||
// - fillChar - Character to fill the target region with. | ||
// - standardFillAttrs - If true, fill with the standard erase attributes. | ||
// If false, fill with the default attributes. | ||
// Return value: | ||
// - S_OK or failure code from thrown exception | ||
[[nodiscard]] HRESULT DoSrvPrivateFillRegion(SCREEN_INFORMATION& screenInfo, | ||
const COORD startPosition, | ||
const size_t fillLength, | ||
const wchar_t fillChar, | ||
const bool standardFillAttrs) noexcept | ||
{ | ||
try | ||
{ | ||
if (fillLength == 0) | ||
{ | ||
return S_OK; | ||
} | ||
|
||
LockConsole(); | ||
auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); | ||
|
||
// For most VT erasing operations, the standard requires that the | ||
// erased area be filled with the current background color, but with | ||
// no additional meta attributes set. For all other cases, we just | ||
// fill with the default attributes. | ||
auto fillAttrs = TextAttribute{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ARGH i apparently wrote this comment eighteen months ago and never published it |
||
if (standardFillAttrs) | ||
{ | ||
fillAttrs = screenInfo.GetAttributes(); | ||
fillAttrs.SetExtendedAttributes(ExtendedAttributes::Normal); | ||
fillAttrs.SetMetaAttributes(0); | ||
zadjii-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
const auto fillData = OutputCellIterator{ fillChar, fillAttrs, fillLength }; | ||
screenInfo.Write(fillData, startPosition, false); | ||
|
||
// Notify accessibility | ||
auto endPosition = startPosition; | ||
const auto bufferSize = screenInfo.GetBufferSize(); | ||
bufferSize.MoveInBounds(fillLength - 1, endPosition); | ||
screenInfo.NotifyAccessibilityEventing(startPosition.X, startPosition.Y, endPosition.X, endPosition.Y); | ||
return S_OK; | ||
} | ||
CATCH_RETURN(); | ||
} | ||
|
||
// Routine Description: | ||
// - A private API call for moving a block of data in the screen buffer, | ||
// optionally limiting the effects of the move to a clipping rectangle. | ||
// Arguments: | ||
// - screenInfo - Reference to screen buffer info. | ||
// - scrollRect - Region to copy/move (source and size). | ||
// - clipRect - Optional clip region to contain buffer change effects. | ||
// - destinationOrigin - Upper left corner of target region. | ||
// - standardFillAttrs - If true, fill with the standard erase attributes. | ||
// If false, fill with the default attributes. | ||
// Return value: | ||
// - S_OK or failure code from thrown exception | ||
[[nodiscard]] HRESULT DoSrvPrivateScrollRegion(SCREEN_INFORMATION& screenInfo, | ||
const SMALL_RECT scrollRect, | ||
const std::optional<SMALL_RECT> clipRect, | ||
const COORD destinationOrigin, | ||
const bool standardFillAttrs) noexcept | ||
{ | ||
try | ||
{ | ||
LockConsole(); | ||
auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); | ||
|
||
// For most VT scrolling operations, the standard requires that the | ||
// erased area be filled with the current background color, but with | ||
// no additional meta attributes set. For all other cases, we just | ||
// fill with the default attributes. | ||
auto fillAttrs = TextAttribute{}; | ||
if (standardFillAttrs) | ||
{ | ||
fillAttrs = screenInfo.GetAttributes(); | ||
fillAttrs.SetExtendedAttributes(ExtendedAttributes::Normal); | ||
fillAttrs.SetMetaAttributes(0); | ||
} | ||
|
||
ScrollRegion(screenInfo, | ||
scrollRect, | ||
clipRect, | ||
destinationOrigin, | ||
UNICODE_SPACE, | ||
fillAttrs); | ||
return S_OK; | ||
} | ||
CATCH_RETURN(); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Arguments comment not updated.
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.
Sorry I totally overlooked that. Added in commit 171a4f9