e7a2732ffb
This is an attempt to simplify the SGR (Select Graphic Rendition) implementation in conhost, to cut down on the number of methods required in the `ConGetSet` interface, and pave the way for future improvements and bug fixes. It already fixes one bug that prevented SGR 0 from being correctly applied when combined with meta attributes. * This a first step towards fixing the conpty narrowing bugs in issue #2661 * I'm hoping the simplification of `ConGetSet` will also help with #3849. * Some of the `TextAttribute` refactoring in this PR overlaps with similar work in PR #1978. ## Detailed Description of the Pull Request / Additional comments The main point of this PR was to simplify the `AdaptDispatch::SetGraphicsRendition` implementation. So instead of having it call a half a dozen methods in the `ConGetSet` API, depending on what kinds of attributes needed to be set, there is now just one call to get current attributes, and another call to set the new value. All adjustments to the attributes are made in the `AdaptDispatch` class, in a simple switch statement. To help with this refactoring, I also made some change to the `TextAttribute` class to make it easier to work with. This included adding a set of methods for setting (and getting) the individual attribute flags, instead of having the calling code being exposed to the internal attribute structures and messing with bit manipulation. I've tried to get rid of any methods that were directly setting legacy, meta, and extended attributes. Other than the fix to the `SGR 0` bug, the `AdaptDispatch` refactoring mostly follows the behaviour of the original code. In particular, it still maps the `SGR 38/48` indexed colors to RGB instead of retaining the index, which is what we ultimately need it to do. Fixing that will first require the color tables to be unified (issue #1223), which I'm hoping to address in a followup PR. But for now, mapping the indexed colors to RGB values required adding an an additional `ConGetSet` API to lookup the color table entries. In the future that won't be necessary, but the API will still be useful for other color reporting operations that we may want to support. I've made this API, and the existing setter, standardise on index values being in the "Xterm" order, since that'll be essential for unifying the code with the terminal adapter one day. I should also point out one minor change to the `SGR 38/48` behavior, which is that out-of-range RGB colors are now ignored rather than being clamped, since that matches the way Xterm works. ## Validation Steps Performed This refactoring has obviously required corresponding changes to the unit tests, but most were just minor updates to use the new `TextAttribute` methods without any real change in behavior. However, the adapter tests did require significant changes to accommodate the new `ConGetSet` API. The basic structure of the tests remain the same, but the simpler API has meant fewer values needed to be checked in each test case. I think they are all still covering the areas there were intended to, though, and they are all still passing. Other than getting the unit tests to work, I've also done a bunch of manual testing of my own. I've made sure the color tests in Vttest all still work as well as they used to. And I've confirmed that the test case from issue #5341 is now working correctly. Closes #5341
48 lines
979 B
C++
48 lines
979 B
C++
// Copyright (c) Microsoft Corporation.
|
|
// Licensed under the MIT license.
|
|
|
|
#include "precomp.h"
|
|
#include "TextAttributeRun.hpp"
|
|
|
|
TextAttributeRun::TextAttributeRun() noexcept :
|
|
_cchLength(0)
|
|
{
|
|
SetAttributes(TextAttribute(0));
|
|
}
|
|
|
|
TextAttributeRun::TextAttributeRun(const size_t cchLength, const TextAttribute attr) noexcept :
|
|
_cchLength(cchLength)
|
|
{
|
|
SetAttributes(attr);
|
|
}
|
|
|
|
size_t TextAttributeRun::GetLength() const noexcept
|
|
{
|
|
return _cchLength;
|
|
}
|
|
|
|
void TextAttributeRun::SetLength(const size_t cchLength) noexcept
|
|
{
|
|
_cchLength = cchLength;
|
|
}
|
|
|
|
void TextAttributeRun::IncrementLength() noexcept
|
|
{
|
|
_cchLength++;
|
|
}
|
|
|
|
void TextAttributeRun::DecrementLength() noexcept
|
|
{
|
|
_cchLength--;
|
|
}
|
|
|
|
const TextAttribute& TextAttributeRun::GetAttributes() const noexcept
|
|
{
|
|
return _attributes;
|
|
}
|
|
|
|
void TextAttributeRun::SetAttributes(const TextAttribute textAttribute) noexcept
|
|
{
|
|
_attributes = textAttribute;
|
|
}
|