<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Adds a global setting, `experimental.detectHyperlinks`, that controls whether we automatically detect links and make them clickable. Default is set to true.
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#9981
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [x] Schema updated.
* [x] I work here
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
When `detectHyperlinks` is set to false, links do not underline on hover and are not clickable.
This commit fixes two issues:
1. We were pushing ConsoleWaitBlocks into the wait queue before they
were fully constructed. This resulted in the wait being called before
it even had an API message in it. [MSFT-24113101]
2. Drawing DBCS characters and resizing (a lot) would cause a crash
because of an invalid DBCS cell state. This hits in both Terminal and
conhost. [MSFT-17364373] [GH-4907]
The DBCS state check was promoted from an NT_ASSERT (which never fired
in release) to a FAIL_FAST in !1794053. The console kept chugging along
without failing in fre for all those years.
Fixes MSFT-24113101
Fixes MSFT-17364373
Fixes GH-4907 Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_wdx_dxp_windev 108e746630749aa7851dd813b19e013ae31ef0db
This PR adds support for the VT line rendition attributes, which allow
for double-width and double-height line renditions. These renditions are
enabled with the `DECDWL` (double-width line) and `DECDHL`
(double-height line) escape sequences. Both reset to the default
rendition with the `DECSWL` (single-width line) escape sequence. For now
this functionality is only supported by the GDI renderer in conhost.
There are a lot of changes, so this is just a general overview of the
main areas affected.
Previously it was safe to assume that the screen had a fixed width, at
least for a given point in time. But now we need to deal with the
possibility of different lines have different widths, so all the
functions that are constrained by the right border (text wrapping,
cursor movement operations, and sequences like `EL` and `ICH`) now need
to lookup the width of the active line in order to behave correctly.
Similarly it used to be safe to assume that buffer and screen
coordinates were the same thing, but that is no longer true. Lots of
places now need to translate back and forth between coordinate systems
dependent on the line rendition. This includes clipboard handling, the
conhost color selection and search, accessibility location tracking and
screen reading, IME editor positioning, "snapping" the viewport, and of
course all the rendering calculations.
For the rendering itself, I've had to introduce a new
`PrepareLineTransform` method that the render engines can use to setup
the necessary transform matrix for a given line rendition. This is also
now used to handle the horizontal viewport offset, since that could no
longer be achieved just by changing the target coordinates (on a double
width line, the viewport offset may be halfway through a character).
I've also had to change the renderer's existing `InvalidateCursor`
method to take a `SMALL_RECT` rather than a `COORD`, to allow for the
cursor being a variable width. Technically this was already a problem,
because the cursor could occupy two screen cells when over a
double-width character, but now it can be anything between one and four
screen cells (e.g. a double-width character on the double-width line).
In terms of architectural changes, there is now a new `lineRendition`
field in the `ROW` class that keeps track of the line rendition for each
row, and several new methods in the `ROW` and `TextBuffer` classes for
manipulating that state. This includes a few helper methods for handling
the various issues discussed above, e.g. position clamping and
translating between coordinate systems.
## Validation Steps Performed
I've manually confirmed all the double-width and double-height tests in
_Vttest_ are now working as expected, and the _VT100 Torture Test_ now
renders correctly (at least the line rendition aspects). I've also got
my own test scripts that check many of the line rendition boundary cases
and have confirmed that those are now passing.
I've manually tested as many areas of the conhost UI that I could think
of, that might be affected by line rendition, including things like
searching, selection, copying, and color highlighting. For
accessibility, I've confirmed that the _Magnifier_ and _Narrator_
correctly handle double-width lines. And I've also tested the Japanese
IME, which while not perfect, is at least useable.
Closes#7865
Moving things out of CharRow into ROW helps us hide it as an implementation detail.
This is part one of many.
### CharRow: Hide ClearCell, use ROW::ClearColumn
### CharRow: Hide GetText, use ROW::GetText
### CharRowBaseTests: remove dead file (never used!)
### CharRow: Move DoubleBytePadded into ROW
### CharRow: Move WrapForced into ROW
### Char/AttrRow: Hide Reset, use ROW::Reset
### Remove RowCellIterator (dead code)
RCI was unused; it was replaced by TextBufferCellIterator shortly after its creation
### Move AttrRowTests to ut_textbuffer from ut_host
It had no reliance on the host.
This is by no means comprehensive. It will be unmarked as draft when it
is more comprehensive.
This pull request adds some tests for resizing a TextBuffer and
reflowing its contents. Each test takes the form of an initial state and
a number of buffers of different sizes. The initial state is used to
seed the first TextBuffer, and the subsequent buffers are only used to
compare.
I manually reimplemented some of the DBCS logic to ensure that the
buffers contain _exactly_ what they're supposed to. I know this is
non-ideal. After some of the CharRow changes in #8446 land, this will
need to be updated.
There's a cool bit of TAEF gore in here: the IDataSource. An IDataSource
allows us to programmatically return test cases. It's a code-only
version of its support for parameterized tests of the form `Data:x = {0,
1, 2}` .
The only downsides are...
1. It looks like COM (it is not using COM under the hood, just the COM
ABI)
2. Property values must be returned as strings.
To best support rich test types, I used IDataSource to produce _a lit of
array indices_ and nothing more. The test is run once for array member,
and it is the test's responsibility to look up the object to which that
index refers.
Works great though! Each reflow test is its own unit, and a failure in
an earlier reflow test will not tank a later one.
Make a few changes to memory usage throughout the application to reduce transient allocations from the `big.txt` test from ~213,000 to ~53,000.
## PR Checklist
* [x] Supports #3075
* [x] I work here.
* [x] Tested manually and WPR'd. Test suite should still pass.
* [x] Am core contributor
## Detailed Description of the Pull Request / Additional comments
Transient allocations are those that are new'd, used, then delete'd. Going back and forth to the system allocator for things we're just going to throw away or use rapidly again is a performance detriment. Not only is it a bunch of time to go ask the system with a syscall, it also hits a whole bunch of locks on the allocators. This PR identifies a few places where we were accidentally allocating and didn't mean to or were allocating and freeing just to turn around and allocate again. I chose other strategies to avoid this.
## Validation Steps Performed
- Ran `big.txt` sample (~6MB file) before and after. Observed heap allocations with WPR.
I was looking at conhost/OpenConsole and noticed it was being pretty
inefficient with allocations due to some usages of std::deque and
std::vector that didn't need to be done quite that way.
So this uses std::vector for the TextBuffer's storage of ROW objects,
which allows one allocation to contiguously reserve space for all the
ROWs - on Desktop this is 9001 ROW objects which means it saves 9000
allocations that the std::deque would have done. Plus it has the
benefit of increasing locality of the ROW objects since deque is going
to chase pointers more often with its data structure.
Then, within each ROW there are CharRow and ATTR_ROW objects that use
std::vector today. This changes them to use Boost's small_vector, which
is a variation of vector that allows for the so-called "small string
optimization." Since we know the typical size of these vectors, we can
pre-reserve the right number of elements directly in the
CharRow/ATTR_ROW instances, avoiding any heap allocations at all for
constructing these objects.
There are a ton of variations on this "small_vector" concept out there
in the world - this one in Boost, LLVM has one called SmallVector,
Electronic Arts' STL has a small_vector, Facebook's folly library has
one...there are a silly number of these out there. But Boost seems like
it's by far the easiest to consume in terms of integration into this
repo, the CI/CD pipeline, licensing, and stuff like that, so I went with
the boost version.
In terms of numbers, I measured the startup path of OpenConsole.exe on
my dev box for Release x64 configuration. My box is an i7-6700k @ 4
Ghz, with 32 GB RAM, not that I think machine config matters much here:
| | Allocation count | Allocated bytes | CPU usage (ms) |
| ------ | ------------------- | ------------------ | -------------- |
| Before | 29,461 | 4,984,640 | 103 |
| After | 2,459 (-91%) | 4,853,931 (-2.6%) | 96 (-7%) |
Along the way, I also fixed a dynamic initializer I happened to spot in
the registry code, and updated some docs.
## Validation Steps Performed
- Ran "runut", "runft" and "runuia" locally and confirmed results are
the same as the main branch
- Profiled the before/after numbers in the Visual Studio profiler, for
the numbers shown in the table
Co-authored-by: Austin Lamb <austinl@microsoft.com>
There are two issue with copy to clipboard when block is selected:
* We don't add new lines for lines that were wrapped
* We remove trailing whitespaces which is not intuitive in block selection.
Fixed the copy logic to always add newlines and not to remove
whitespaces when block is selected.
Even if shift is pressed!
## Detailed Description of the Pull Request / Additional comments
* Added optional parameter to `TextBuffer::GetText`
that allows to apply formatting (includeCRLF / trimming)
to lines that were wrapped
* Changed `Terminal::RetrieveSelectedTextFromBuffer`
to apply the following parameters when block is selected:
* includeCRLF = true
* trimTrailingWhitespaces = false
* apply the formatting above to all rows, including the ones
that were wrapped
## Validation Steps Performed
* Manual tests for both block and standard selection
* Copy with both right-click and command
* Added UT
Closes#6740
This PR defines a series of `NOSOMETHING` macros in PCHs, in order to
prevent `windows.h` from bringing a lot of rarely used things into the
project.
Theoretically this should make PCH generation and overall complication
faster, but I didn't really benchmark the speed.
Another benefit would be reducing the symbol noises caused by
`windows.h`.
I added `enum class` to one thing and decided that that was quite enough
before disabling the `enum class` warning.
Looks like 16.8 made more map/vector operations noexcept, so we have to
re-annotate to remain compliant.
Many include statements use forward slashes, while others use backwards
slashes. This is inconsistent formatting. For this reason, I changed the
backward slashes to forward slashes since that is the standard.
Fix#8121
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
When calculating the position of the matched pattern, consider the width of the characters.
However, if there are some wide glyphs in the detected hyperlink(not possible for now, for the existing regex will not match wide-character?). The repeated character in the tooltip is not fixed by this PR.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes#8121
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
When calculating the coordinate of the match in #7691, it simply uses the `prefix.size()` as the total prefix width on the screen.
This PR fixes that behavior.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Manually Verified
This pull request is the initial implementation of hyperlink auto
detection
Overall design:
- Upon startup, TerminalCore gives the TextBuffer some patterns it
should know about
- Whenever something in the viewport changes (i.e. text
output/scrolling), TerminalControl tells TerminalCore (through a
throttled function for performance) to retrieve the visible pattern
locations from the TextBuffer
- When the renderer encounters a region that is associated with a
pattern, it paints that region differently
References #5001Closes#574
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Realized that we don't copy the current hyperlink id when we copy buffers, quick fix for that
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] I work here
It turns out that we missed part of the OSC 8 spec which indicated that
_hyperlinks with the same ID but different URIs are logically distinct._
> Character cells that have the same target URI and the same nonempty id
> are always underlined together on mouseover.
> The same id is only used for connecting character cells whose URIs is
> also the same. Character cells pointing to different URIs should never
> be underlined together when hovering over.
This pull request fixes that oversight by appending the (hashed) URI to
the generated ID.
When Terminal receives one of these links over ConPTY, it will hash the
URL a second time and therefore append a second hashed ID. This is taken
as an acceptable cost.
Fixes#7698
The `MovementAtExclusiveEnd` test was improperly authored for the
following reasons:
- it should have used `TEST_METHOD_PROPERTY` to cover all of the
TextUnits
- TextUnit::Document (arguably one of the most important) was ommitted
accidentally (`!= TextUnit_Document` was used instead of `<=`)
- The created range was not `EndExclusive`, but rather, the last cell in
the buffer (`EndInclusive`)
The first half of this PR fixes the test.
The second half of this PR expands the test and fixes any related issues
to make the test pass (i.e. #7771):
- `TEST_METHOD_PROPERTY` was added for it to be degenerate (start/end at
`EndExclusive`) or not (last cell of buffer)
- `utr->_start` is now also validated after moving backwards
NOTE: `utr->_start` was not validated when moving forwards because
moving forwards should always fail when at/past the last chell in the
buffer.
Closes#7771
This performs a minor refactor on `TextBuffer::MoveToNextWord` that
relies more heavily on `TextBuffer::GetWordEnd`. Now, the logic is
simplified and looks more like `MoveToPreviousWord`.
This refactor required me to move the `lastCharPos` optimization down to
`GetWordEnd`. So word expansion gets this optimization for free now.
### WPR Traces
The percentages below represent the weight that a function call had. The
test scenario included moving by word on the CMD welcome message until
the last word was reached. Inspect.exe was used to limit any additional
calls that are generally performed by a screen reader.
| function | current | branch |
| -- | -- | -- |
| `UIA:Move` | 34.55% | 29.52% |
There is an improvement of about 5% in a release build of ConHost.
NOTE: `UIA::Move` already calls `Expand` after a move operation is
performed. I'm using this data to represent a performance improvement
across both functions.
Contributes to #5243
This fixes a bug when moving backwards by word that resulted in #7742.
This also includes...
- a minor refactor that leverages `GetWordStart` in `MoveToPreviousWord`
- additional unit tests for movement by word
- a feature test comprised of the referenced bug report
`MoveToPreviousWord()` would...
- move backwards for each whitespace character
- then, move backwards for each regular character
This would actually result in moving to the beginning of the current "word" (as defined by a11y).
We actually need to do this process twice:
- the first time gets you to the beginning of the current word
- attempt to move back by one character
- the second time gets you to the beginning of the previous word
Rather than implementing 4 while loops, we leverage `GetWordStart()` to
attempt to move to the beginning of the previous word. We call it twice
(as described above). The logic is unchanged, but we instead reuse a
function that has already undergone more testing.
To make sure this works as expected, additional unit tests were
introduced covering "MoveByWord" in the TextBuffer.
## Validation Steps Performed
Added test for repro steps.
Added unit tests for movement by word.
Closes#7742
`EndExclusive` represents the end of the buffer. This is designed to not
point to any data on the buffer. UiaTextRange would point to this
`EndExclusive` and then attempt to move based on it. However, since it
does not point to any data, it could experience undefined behavior or
(inevitably) crash from running out of bounds.
This PR specifically checks for expansion and movement at that point,
and prevents us from moving beyond it. There are plans in the future to
define the "end" as the last character in the buffer. Until then, this
solution will suffice and provide correct behavior that doesn't crash.
## Validation Steps Performed
Performed the referenced bugs' repro steps and added test coverage.
Closes MSFT-20458595
Closes#7663Closes#7664
This PR adds support for the _blink_ graphic rendition attribute. When a
character is output with this attribute set, it "blinks" at a regular
interval, by cycling its color between the normal rendition and a dimmer
shade of that color.
The majority of the blinking mechanism is encapsulated in a new
`BlinkingState` class, which is shared between the Terminal and Conhost
implementations. This class keeps track of the position in the blinking
cycle, which determines whether characters are rendered as normal or
faint.
In Windows Terminal, the state is stored in the `Terminal` class, and in
Conhost it's stored in the `CONSOLE_INFORMATION` class. In both cases,
the `IsBlinkingFaint` method is used to determine the current blinking
rendition, and that is passed on as a parameter to the
`TextAttribute::CalculateRgbColors` method when these classes are
looking up attribute colors.
Prior to calculating the colors, the current attribute is also passed to
the `RecordBlinkingUsage` method, which keeps track of whether there are
actually any blink attributes in use. This is used to determine whether
the screen needs to be refreshed when the blinking cycle toggles between
the normal and faint renditions.
The refresh itself is handled by the `ToggleBlinkingRendition` method,
which is triggered by a timer. In Conhost this is just piggybacking on
the existing cursor blink timer, but in Windows Terminal it needs to
have its own separate timer, since the cursor timer is reset whenever a
key is pressed, which is not something we want for attribute blinking.
Although the `ToggleBlinkingRendition` is called at the same rate as the
cursor blinking, we actually only want the cells to blink at half that
frequency. We thus have a counter that cycles through four phases, and
blinking is rendered as faint for two of those four. Then every two
cycles - when the state changes - a redraw is triggered, but only if
there are actually blinking attributes in use (as previously recorded).
As mentioned earlier, the blinking frequency is based on the cursor
blink rate, so that means it'll automatically be disabled if a user has
set their cursor blink rate to none. It can also be disabled by turning
off the _Show animations in Windows_ option. In Conhost these settings
take effect immediately, but in Windows Terminal they only apply when a
new tab is opened.
This PR also adds partial support for the `SGR 6` _rapid blink_
attribute. This is not used by DEC terminals, but was defined in the
ECMA/ANSI standards. It's not widely supported, but many terminals just
it implement it as an alias for the regular `SGR 5` blink attribute, so
that's what I've done here too.
## Validation Steps Performed
I've checked the _Graphic rendition test pattern_ in Vttest, and
compared our representation of the blink attribute to that of an actual
DEC VT220 terminal as seen on [YouTube]. With the right color scheme
it's a reasonably close match.
[YouTube]: https://www.youtube.com/watch?v=03Pz5AmxbE4&t=1m55sCloses#7388
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Conhost can now support OSC8 sequences (as specified [here](https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda)). Terminal also supports those sequences and additionally hyperlinks can be opened by Ctrl+LeftClicking on them.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
#204
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [X] Closes#204
* [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
Added support to:
- parse OSC8 sequences and extract URIs from them (conhost and terminal)
- add hyperlink uri data to textbuffer/screeninformation, associated with a hyperlink id (conhost and terminal)
- attach hyperlink ids to text to allow for uri extraction from the textbuffer/screeninformation (conhost and terminal)
- process ctrl+leftclick to open a hyperlink in the clicked region if present
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Open up a PowerShell tab and type
```PowerShell
${ESC}=[char]27
Write-Host "${ESC}]8;;https://github.com/microsoft/terminal${ESC}\This is a link!${ESC}]8;;${ESC}\"
```
Ctrl+LeftClick on the link correctly brings you to the terminal page on github
New warnings were added in VS 16.7 and `std::map::erase` is now `noexcept`.
Update our code to be compatible with the new enforcement.
## PR Checklist
* [x] Closes broken audit in main after Agents updated over the weekend.
* [x] I work here.
* [x] Audit mode passes now
* [x] Am core contributor.
## Validation Steps Performed
* [x] Ran audit mode locally
This PR adds support for the ANSI _doubly underlined_ graphic rendition
attribute, which is enabled by the `SGR 21` escape sequence.
There was already an `ExtendedAttributes::DoublyUnderlined` flag in the
`TextAttribute` class, but I needed to add `SetDoublyUnderlined` and
`IsDoublyUnderlined` methods to access that flag, and update the
`SetGraphicsRendition` methods of the two dispatchers to set the
attribute on receipt of the `SGR 21` sequence. I also had to update the
existing `SGR 24` handler to reset _DoublyUnderlined_ in addition to
_Underlined_, since they share the same reset sequence.
For the rendering, I've added a new grid line type, which essentially
just draws an additional line with the same thickness as the regular
underline, but slightly below it - I found a gap of around 0.05 "em"
between the lines looked best. If there isn't enough space in the cell
for that gap, the second line will be clamped to overlap the first, so
you then just get a thicker line. If there isn't even enough space below
for a thicker line, we move the offset _above_ the first line, but just
enough to make it thicker.
The only other complication was the update of the `Xterm256Engine` in
the VT renderer. As mentioned above, the two underline attributes share
the same reset sequence, so to forward that state over conpty we require
a slightly more complicated process than with most other attributes
(similar to _Bold_ and _Faint_). We first check whether either underline
attribute needs to be turned off to send the reset sequence, and then
check individually if each of them needs to be turned back on again.
## Validation Steps Performed
For testing, I've extended the existing attribute tests in
`AdapterTest`, `VTRendererTest`, and `ScreenBufferTests`, to make sure
we're covering both the _Underlined_ and _DoublyUnderlined_ attributes.
I've also manually tested the `SGR 21` sequence in conhost and Windows
Terminal, with a variety of fonts and font sizes, to make sure the
rendering was reasonably distinguishable from a single underline.
Closes#2916
This PR updates the rendering of the _underlined_ graphic rendition
attribute, using the style specified in the active font, instead of just
reusing the grid line at the bottom of the character cell.
* Support for drawing the correct underline effect in the grid line
renderer was added in #7107.
There was already an `ExtendedAttributes` flag defined for the
underlined state, but I needed to update the `SetUnderlined` and
`IsUnderlined` methods in the `TextAttribute` class to use that flag
now in place of the legacy `LVB_UNDERSCORE` attribute. This enables
underlines set via a VT sequence to be tracked separately from
`LVB_UNDERSCORE` grid lines set via the console API.
I then needed to update the `Renderer::s_GetGridlines` method to
activate the `GridLines::Underline` style when the `Underlined`
attribute was set. The `GridLines::Bottom` style is still triggered by
the `LVB_UNDERSCORE` attribute to produce the bottom grid line effect.
Validation
----------
Because this is a change from the existing behaviour, certain unit tests
that were expecting the `LVB_UNDERSCORE` to be toggled by `SGR 4` and
`SGR 24` have now had to be updated to check the `Underlined` flag
instead.
There were also some UI Automation tests that were checking for `SGR 4`
mapping to `LVB_UNDERSCORE` attribute, which I've now substituted with a
test of the `SGR 53` overline attribute mapping to
`LVB_GRID_HORIZONTAL`. These tests only work with legacy attributes, so
they can't access the extended underline state, and I thought a
replacement test that covered similar ground would be better than
dropping the tests altogether.
As far as the visual rendering is concerned, I've manually confirmed
that the VT underline sequences now draw the underline in the correct
position and style, while grid lines output via the console API are
still displayed in their original form.
Closes#2915
The act of calling `InsertAttrRuns` is relatively slow. Instead of
calling it a bunch of times to meddle with colors one cell at a time,
we'll accumulate a length of color and call it to make it act all at
once. This is great for when one color full line is getting replaced
with another color full line OR when a line is being replaced with the
same color all at once. There's significantly fewer checks to be made
inside `InsertAttrRuns` if we can help it out by accumulating the length
of each color before asking it to stitch it into the storage.
Validation
----------
- Run `time cat big.txt` and `time cat ls.txt` under VS Performance
Profiler.
## Summary of the Pull Request
Text can have various attributes, such as "bold", "italic", "underlined", etc. The TextAttribute class embodies this. It has methods to set/query these attributes.
This change tweaks a few of the method names to make them match. I.e. for an imaginary text property "Foo", we should have methods along the lines of:
```
IsFoo
SetFoo(bool isFoo)
```
And variations should match: we should have "Foo" and "OverFoo", not "Fooey" and "OverFoo".
I chose to standardize on the adjective form, since that's what we are closest to already. The attributes I attacked here are:
SetItalic**s** --> SetItalic
SetUnderline --> SetUnderline**d**
SetOverline --> SetOverline**d**
("italic" is an adjective; "italics" is a plural noun, representing letters or words in an italic typeface)
And I also added methods for "DoublyUnderlined" for good measure.
I stopped short of renaming the GraphicsOptions enum values to match, too; but I'd be willing to do that in a follow-up change if people wanted it.
## Validation Steps Performed
It builds, and tests still pass.
The recent changes to use gsl::span everywhere added a few bounds checks
along codepaths where we were already checking bounds. Some of them may
be non-obvious to the optimizer, so we can now use til::at to help them
along.
To accomplish this, I've added a new overload of til::at that takes a
span and directly accesses its backing buffer.
We were using std::basic_string_view as a stand-in for std::span so that
we could change over all at once when C++20 dropped with full span
support. That day's not here yet, but as of 54a7fce3e we're using GSL 3,
whose span is C++20-compliant.
This commit replaces every instance of basic_string_view that was not
referring to an actual string with a span of the appropriate type.
I moved the `const` qualifier into span's `T` because while
`basic_string_view.at()` returns `const T&`, `span.at()` returns `T&`
(without the const). I wanted to maintain the invariant that members of
the span were immutable.
* Mechanical Changes
* `sv.at(x)` -> `gsl::at(sp, x)`
* `sv.c{begin,end}` -> `sp.{begin,end}` (span's iterators are const)
I had to replace a `std::basic_string<>` with a `std::vector<>` in
ConImeInfo, and I chose to replace a manual array walk in
ScreenInfoUiaProviderBase with a ranged-for. Please review those
specifically.
This will almost certainly cause a code size regression in Windows
because I'm blowing out all the PGO counts. Whoops.
Related: #3956, #975.
## Summary of the Pull Request
This PR adds support for the `SGR 8` and `SGR 28` escape sequences,
which enable and disable the _concealed/invisible_ graphic rendition
attribute. When a character is output with this attribute set, it is
rendered with the same foreground and background colors, so the text is
essentially invisible.
## PR Checklist
* [x] Closes#6876
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. Issue number
where discussion took place: #6876
## Detailed Description of the Pull Request / Additional comments
Most of the framework for this attribute was already implemented, so it
was just a matter of updating the `TextAttribute::CalculateRgbColors`
method to make the foreground the same as the background when the
_Invisible_ flag was set. Note that this has to happen after the
_Reverse Video_ attribute is applied, so if you have white-on-black text
that is reversed and invisible, it should be all white, rather than all
black.
## Validation Steps Performed
There were already existing SGR unit tests covering this attribute in
the `ScreenBufferTests`, and the `VtRendererTest`. But I've added to the
`AdapterTest` which verifies the SGR sequences for setting and resetting
the attribute, and I've extended the `TextAttributeTests` to verify that
the color calculations return the correct values when the attribute is
set.
I've also manually confirmed that we now render the _concealed text_
values correctly in the _ISO 6429_ tests in Vttest. And I've manually
tested the output of _concealed_ when combined with other attributes,
and made sure that we're matching the behaviour of most other terminals.
console: switch to /Zc:wchar_t (native wchar_t)
This matches what we use in OpenConsole and makes {fmt} play nice.
I've also removed the workaround we introduced into OutputCellIterator
to work around not using /Zc:wchar_t.
Fixes MSFT:27626309.
Fixes GH-2673.
Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_onecore_dep_uxp 1508f7c232ec58bebc37fedfdec3eb8f9bff5502
## Summary of the Pull Request
This PR adds support for the `SGR 2` escape sequence, which enables the
ANSI _faint_ graphic rendition attribute. When a character is output
with this attribute set, it uses a dimmer version of the active
foreground color.
## PR Checklist
* [x] Closes#6703
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. Issue number where discussion took place: #6703
## Detailed Description of the Pull Request / Additional comments
There was already an `ExtendedAttributes::Faint` flag in the
`TextAttribute` class, but I needed to add `SetFaint` and `IsFaint`
methods to access that flag, and update the `SetGraphicsRendition`
methods of the two dispatchers to set the attribute on receipt of the
`SGR 2` sequence. I also had to update the existing `SGR 22` handler to
reset _Faint_ in addition to _Bold_, since they share the same reset
sequence. For that reason, I thought it a good idea to change the name
of the `SGR 22` enum to `NotBoldOrFaint`.
For the purpose of rendering, I've updated the
`TextAttribute::CalculateRgbColors` method to return a dimmer version of
the foreground color when the _Faint_ attribute is set. This is simply
achieved by dividing each color component by two, which produces a
reasonable effect without being too complicated. Note that the _Faint_
effect is applied before _Reverse Video_, so if the output it reversed,
it's the background that will be faint.
The only other complication was the update of the `Xterm256Engine` in
the VT renderer. As mentioned above, _Bold_ and _Faint_ share the same
reset sequence, so to forward that state over conpty we have to go
through a slightly more complicated process than with other attributes.
We first check whether either attribute needs to be turned off to send
the reset sequence, and then check if the individual attributes need to
be turned on again.
## Validation
I've extended the existing SGR unit tests to cover the new attribute in
the `AdapterTest`, the `ScreenBufferTests`, and the `VtRendererTest`,
and added a test to confirm the color calculations when _Faint_ is set
in the `TextAttributeTests`.
I've also done a bunch of manual testing with all the different VT color
types and confirmed that our output is comparable to most other
terminals.
This is a refactoring of the renderer color calculations to simplify the
implementation, and to make it easier to support additional
color-altering rendition attributes in the future (e.g. _faint_ and
_conceal_).
## References
* This is a followup to PRs #3817 and #6809, which introduced additional
complexity in the color calculations, and which suggested the need for
refactoring.
## Detailed Description of the Pull Request / Additional comments
When we added support for `DECSCNM`, that required the foreground and
background color lookup methods to be able to return the opposite of
what was requested when the reversed mode was set. That made those
methods unnecessarily complicated, and I thought we could simplify them
considerably just by combining the calculations into a single method
that derived both colors at the same time.
And since both conhost and Windows Terminal needed to perform the same
calculations, it also made sense to move that functionality into the
`TextAttribute` class, where it could easily be shared.
In general this way of doing things is a bit more efficient. However, it
does result in some unnecessary work when only one of the colors is
required, as is the case for the gridline painter. So to make that less
of an issue, I've reordered the gridline code a bit so it at least
avoids looking up the colors when no gridlines are needed.
## Validation Steps Performed
Because of the API changes, quite a lot of the unit tests had to be
updated. For example instead of verifying colors with two separate calls
to `LookupForegroundColor` and `LookupBackgroundColor`, that's now
achieved with a single `LookupAttributeColors` call, comparing against a
pair of values. The specifics of the tests haven't changed though, and
they're all still working as expected.
I've also manually confirmed that the various color sequences and
rendition attributes are rendering correctly with the new refactoring.
There is going to be a very long tail of applications that will
explicitly request VT SGR 40/37 when what they really want is to
SetConsoleTextAttribute() with a black background/white foreground.
Instead of making those applications look bad (and therefore making us
look bad, because we're releasing this as an update to something that
"looks good" already), we're introducing this compatibility quirk.
Before the color reckoning in #6698 + #6506, *every* color was subject
to being spontaneously and erroneously turned into the default color.
Now, only the 16-color palette value that matches the active console
background/foreground color will be destroyed, and only when received
from specific applications.
Removal will be tracked by #6807.
Michael and I discussed what layer this quirk really belonged in. I
originally believed it would be sufficient to detect a background color
that matched the legacy default background, but @j4james provided an
example of where that wouldn't work out (powershell setting the
foreground color to white/gray). In addition, it was too heavyhanded: it
re-broke black backgrounds for every application.
Michael thought that it should live in the server, as a small VT parser
that righted the wrongs coming directly out of the application. On
further investigation, however, I realized that we'd need to push more
information up into the server (so that it could make the decision about
which VT was wrong and which was right) than should be strictly
necessary.
The host knows which colors are right and wrong, and it gets final say
in what ends up in the buffer.
Because of that, I chose to push the quirk state down through
WriteConsole to DoWriteConsole and toggle state on the
SCREEN_INFORMATION that indicates whether the colors coming out of the
application are to be distrusted. This quirk _only applies to pwsh.exe
and powershell.exe._
NOTE: This doesn't work for PowerShell the .NET Global tool, because it
is run as an assembly through dotnet.exe. I have no opinion on how to
fix this, or whether it is worth fixing.
VALIDATION
----------
I configured my terminals to have an incredibly garish color scheme to
show exactly what's going to happen as a result of this. The _default
terminal background_ is purple or red, and the foreground green. I've
printed out a heap of test colors to see how black interacts with them.
Pull request #6810 contains the images generated from this test.
The only color lines that change are the ones where black as a
background or white as a foreground is selected out of the 16-color
palette explicitly. Reverse video still works fine (because black is in
the foreground!), and it's even possible to represent "black on default"
and reverse it into "default on black", despite the black in question
having been `40`.
Fixes#6767.
Looking up the size of the viewport from the underlying dimensions of
the structures seemed like a good idea at the time (so it would only be
in one place), but it turns out to be more of a perf cost than we
expected. Not necessarily on any one hot path, but if we sort by
functions in WPR, it was the top consumer on the Terminal side. This
instead saves the size as a member of the `TextBuffer` and serves that
out. It only changes when it is constructed or resized traditionally, so
it's easy to update/keep track of. It impacted conhost/conpty to a
lesser degree but was still noticeable.
## Validation Steps Performed
- Run `time cat big.txt` under WPR. Checked before and after perf
metrics.
## PR Checklist
* [x] Closes perf itch
* [x] I work here
* [x] Manual test
* [x] Documentation irrelevant.
* [x] Schema irrelevant.
* [x] Am core contributor.
## Summary of the Pull Request
This PR adds support for the `SGR 53` and `SGR 55` escapes sequences,
which enable and disable the ANSI _overline_ graphic rendition
attribute, the equivalent of the console character attribute
`COMMON_LVB_GRID_HORIZONTAL`. When a character is output with this
attribute set, a horizontal line is rendered at the top of the character
cell.
## PR Checklist
* [x] Closes#6000
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already.
## Detailed Description of the Pull Request / Additional comments
To start with, I added `SetOverline` and `IsOverlined` methods to the
`TextAttribute` class, to set and get the legacy
`COMMON_LVB_GRID_HORIZONTAL` attribute. Technically there was already an
`IsTopHorizontalDisplayed` method, but I thought it more readable to add
a separate `IsOverlined` as an alias for that.
Then it was just a matter of adding calls to set and reset the attribute
in response to the `SGR 53` and `SGR 55` sequences in the
`SetGraphicsRendition` methods of the two dispatchers. The actual
rendering was already taken care of by the `PaintBufferGridLines` method
in the rendering engines.
The only other change required was to update the `_UpdateExtendedAttrs`
method in the `Xterm256Engine` of the VT renderer, to ensure the
attribute state would be forwarded to the Windows Terminal over conpty.
## Validation Steps Performed
I've extended the existing SGR unit tests to cover the new attribute in
the `AdapterTest`, the `OutputEngineTest`, and the `VtRendererTest`.
I've also manually tested the `SGR 53` and `SGR 55` sequences to confirm
that they do actually render (or remove) an overline on the characters
being output.
This PR reimplements the VT rendering engines to do a better job of
preserving the original color types when propagating attributes over
ConPTY. For the 16-color renderers it provides better support for
default colors and improves the efficiency of the color narrowing
conversions. It also fixes problems with the ordering of character
renditions that could result in attributes being dropped.
Originally the base renderer would calculate the RGB color values and
legacy/extended attributes up front, passing that data on to the active
engine's `UpdateDrawingBrushes` method. With this new implementation,
the renderer now just passes through the original `TextAttribute` along
with an `IRenderData` interface, and leaves it to the engines to extract
the information they need.
The GDI and DirectX engines now have to lookup the RGB colors themselves
(via simple `IRenderData` calls), but have no need for the other
attributes. The VT engines extract the information that they need from
the `TextAttribute`, instead of having to reverse engineer it from
`COLORREF`s.
The process for the 256-color Xterm engine starts with a check for
default colors. If both foreground and background are default, it
outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to
make sure any reset state is reapplied. With that out the way, the
foreground and background are updated (if changed) in one of 4 ways.
They can either be a default value (SGR 39 and 49), a 16-color index
(using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value
(both using SGR 38 and 48 sequences).
Then once the colors are accounted for, there is a separate step that
handles the character rendition attributes (bold, italics, underline,
etc.) This step must come _after_ the color sequences, in case a SGR
reset is required, which would otherwise have cleared any character
rendition attributes if it came last (which is what happened in the
original implementation).
The process for the 16-color engines is a little different. The target
client in this case (Windows telnet) is incapable of setting default
colors individually, so we need to output an SGR 0 reset if _either_
color has changed to default. With that out the way, we use the
`TextColor::GetLegacyIndex` method to obtain an approximate 16-color
index for each color, and apply the bold attribute by brightening the
foreground index (setting bit 8) if the color type permits that.
However, since Windows telnet only supports the 8 basic ANSI colors, the
best we can do for bright colors is to output an SGR 1 attribute to get
a bright foreground. There is nothing we can do about a bright
background, so after that we just have to drop the high bit from the
colors. If the resulting index values have changed from what they were
before, we then output ANSI 8-color SGR sequences to update them.
As with the 256-color engine, there is also a final step to handle the
character rendition attributes. But in this case, the only supported
attributes are underline and reversed video.
Since the VT engines no longer depend on the active color table and
default color values, there was quite a lot of code that could now be
removed. This included the `IDefaultColorProvider` interface and
implementations, the `Find(Nearest)TableIndex` functions, and also the
associated HLS conversion and difference calculations.
VALIDATION
Other than simple API parameter changes, the majority of updates
required in the unit tests were to correct assumptions about the way the
colors should be rendered, which were the source of the narrowing bugs
this PR was trying to fix. Like passing white on black to the
`UpdateDrawingBrushes` API, and expecting it to output the default `SGR
0` sequence, or passing an RGB color and expecting an indexed SGR
sequence.
In addition to that, I've added some VT renderer tests to make sure the
rendition attributes (bold, underline, etc) are correctly retained when
a default color update causes an `SGR 0` sequence to be generated (the
source of bug #3076). And I've extended the VT renderer color tests
(both 256-color and 16-color) to make sure we're covering all of the
different color types (default, RGB, and both forms of indexed colors).
I've also tried to manually verify that all of the test cases in the
linked bug reports (and their associated duplicates) are now fixed when
this PR is applied.
Closes#2661Closes#3076Closes#3717Closes#5384Closes#5864
This is only a partial fix for #293, but I suspect the remaining cases
are unfixable.
Essentially what this does is map the default legacy foreground and
background attributes (typically white on black) to the `IsDefault`
color type in the `TextColor` class. As a result, we can now initialize
the buffer for "legacy" shells (like PowerShell and cmd.exe) with
default colors, instead of white on black. This fixes the startup
rendering in conpty clients, which expect an initial default background
color. It also makes these colors update appropriately when the default
palette values change.
One complication in getting this to work, is that the console permits
users to change which color indices are designated as defaults, so we
can't assume they'll always be white on black. This means that the
legacy-to-`TextAttribute` conversion will need access to those default
values.
Unfortunately the defaults are stored in the conhost `Settings` class
(the `_wFillAttribute` field), which isn't easily accessible to all the
code that needs to construct a `TextAttribute` from a legacy value. The
`OutputCellIterator` is particularly problematic, because some iterator
types need to generate a new `TextAttribute` on every iteration.
So after trying a couple of different approaches, I decided that the
least worst option would be to add a pair of static properties for the
legacy defaults in the `TextAttribute` class itself, then refresh those
values from the `Settings` class whenever the defaults changed (this
only happens on startup, or when the conhost _Properties_ dialog is
edited).
And once the `TextAttribute` class had access to those defaults, it was
fairly easy to adapt the constructor to handle the conversion of default
values to the `IsDefault` color type. I could also then simplify the
`TextAttribute::GetLegacyAttributes` method which does the reverse
mapping, and which previously required the default values to be passed
in as a parameter
VALIDATION
I had to make one small change to the `TestRoundtripExhaustive` unit
test which assumed that all legacy attributes would convert to legacy
color types, which is no longer the case, but otherwise all the existing
tests passed as is. I added a new unit test verifying that the default
legacy attributes correctly mapped to default color types, and the
default color types were mapped back to the correct legacy attributes.
I've manually confirmed that this fixed the issue raised in #5952,
namely that the conhost screen is cleared with the correct default
colors, and also that it is correctly refreshed when changing the
palette from the properties dialog. And I've combined this PR with
#6506, and confirmed that the PowerShell and the cmd shell renderings in
Windows Terminal are at least improved, if not always perfect.
This is a prerequisite for PR #6506Closes#5952
Replace std::map with std::unordered_map when the order doesn't matter
and hash functions are provided. Simple optimizations, but I expect the
performance should be strictly better, especially for
CodepointWidthDetector.hpp.
## Summary of the Pull Request
Improve `ATTR_ROW::ReplaceAttrs` performance by only reserving the necessary capacity instead of resizing the new run.
That way `TextAttributeRun`s are only instantiated once instead of twice.
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
## Detailed Description of the Pull Request / Additional comments
Performance could be further improved by directly moving `TextAttributeRun`s into the new vector, but I considered this out of scope for this PR.
## Validation Steps Performed
CPU usage when running `cacafire` is slightly reduced.
This PR provides a faster algorithm for converting 8-bit and 24-bit
colors into the 4-bit legacy values that are required by the Win32
console APIs. It also fixes areas of the code that were incorrectly
using a simple 16-color conversion that didn't handle 8-bit and 24-bit
values.
The faster conversion algorithm should be an improvement for issues #783
and #3950.
One of the main points of this PR was to fix the
`ReadConsoleOutputAttribute` API, which was using a simplified legacy
color conversion (the original `TextAttribute:GetLegacyAttributes`
method), which could only handle values from the 16-color table. RGB
values, and colors from the 256-color table, would be mapped to
completely nonsensical values. This API has now been updated to use the
more correct `Settings::GenerateLegacyAttributes` method.
But there were also a couple of other places in the code that were using
`GetLegacyAttributes` when they really had no reason to be working with
legacy attributes at all. This could result in colors being downgraded
to 4-bit values (often badly, as explained above), when the code was
already perfectly capable of displaying the full 24-bits.
This included the fill colors in the IME composer (in `ConsoleImeInfo`),
and the construction of the highlighting colors in the color
search/selection handler (`Selection::_HandleColorSelection`). I also
got rid of some legacy attribute code in the `Popup` class, which was
originally intended to update colors below the popup when the settings
changed, but actually caused more problems than it solved.
The other major goal of this PR was to improve the performance of the
`GenerateLegacyAttributes` method, since the existing implementation
could be quite slow when dealing with RGB values.
The simple cases are handled much the same as they were before. For an
`IsDefault` color, we get the default index from the
`Settings::_wFillAttribute` field. For an `IsIndex16` color, the index
can just be returned as is.
For an `IsRgb` color, the RGB components are compressed down to 8 bits
(3 red, 3 green, 2 blue), simply by dropping the least significant bits.
This 8-bit value is then used to lookup a representative 16-color value
from a hard-coded table. An `IsIndex256` color is also converted with a
lookup table, just using the existing 8-bit index.
The RGB mapping table was calculated by taking each compressed 8-bit
color, and picking a entry from the _Campbell_ palette that best
approximated that color. This was done by looking at a range of 24-bit
colors that mapped to the 8-bit value, finding the best _Campbell_ match
for each of them (using a [CIEDE2000] color difference calculation), and
then the most common match became the index that the 8-bit value would
map to.
The 256-color table was just a simpler version of this process. For each
entry in the table, we take the default RGB palette value, and find it's
closest match in the _Campbell_ palette.
Because these tables are hard-coded, the results won't adjust to changes
in the palette. However, they should still produce reasonable results
for palettes that follow the standard ANSI color range. And since
they're only a very loose approximation of the colors anyway, the exact
value really isn't that important.
That said, I have tried to make sure that if you take an RGB value for a
particular index in a reasonable color scheme, then the legacy color
mapped from that value should ideally match the same index. This will
never be possible for all color schemes, but I have tweaked a few of the
table entries to improve the results for some of the common schemes.
One other point worth making regarding the hard-coded tables: even if we
wanted to take the active palette into account, that wouldn't actually
be possible over a conpty connection, because we can't easily know what
color scheme the client application is using. At least this way the
results in conhost are guaranteed to be the same as in the Windows
Terminal.
[CIEDE2000]: https://en.wikipedia.org/wiki/Color_difference#CIEDE2000
## Validation Steps Performed
This code still passes the `TextAttributeTests` that check the basic
`GetLegacyAttribute` behaviour and verify the all legacy attributes
roundtrip correctly. However, some of the values in the `RgbColorTests`
had to be updated, since we're now intentionally returning different
values as a result of the changes to the RGB conversion algorithm.
I haven't added additional unit tests, but I have done a lot of manual
testing to see how well the new algorithm works with a range of colors
and a variety of different color schemes. It's not perfect in every
situation, but I think it works well enough for the purpose it serves.
I've also confirmed that the issues reported in #5940 and #6247 are now
fixed by these changes.
Closes#5940Closes#6247
In Windows, we build with /Zc:wchar_t- (which makes wchar_t an unsigned
short typedef.) This causes build breaks when we compare two wchar_t
values (or a wchar_t and an enum class that's of type wchar_t) and the
compiler decides that it might want to _promote them to TextAttribute_
before doing the comparison.
## Summary of the Pull Request
When resizing the window title, a GDI object would be leaked. This has to do with our island message handler using `wil` to track these objects and `wil` having a bug.
## References
microsoft/wil#100
## PR Checklist
* [x] Closes#5949
* [x] I work here.
* [x] Tested manually
* [x] Doc not required.
* [x] Am core contributor.
## Validation Steps Performed
* [x] Added the GDI Objects column to Task Manager, set the Terminal to use the `titleWidth` size tabs, then changed the title a bunch with PowerShell. Confirmed repro before (increasing GDI count). Confirmed it's gone after (no change to object count).
This PR introduces a new `ColorType` to allow us to distinguish between
`SGR` indexed colors from the 16 color table, the lower half of which
can be brightened, and the ISO/ITU indexed colors from the 256 color
table, which have a fixed brightness. Retaining the distinction between
these two types will enable us to forward the correct `SGR` sequences to
conpty when addressing issue #2661.
The other benefit of retaining the color index (which we didn't
previously do for ISO/ITU colors) is that it ensures that the colors are
updated correctly when the color scheme is changed.
## References
* This is another step towards fixing the conpty narrowing bugs in issue
#2661.
* This is technically a fix for issue #5384, but that won't be apparent
until #2661 is complete.
## PR Checklist
* [x] Closes#1223
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already.
## Detailed Description of the Pull Request / Additional comments
The first part of this PR was the introduction of a new `ColorType` in
the `TextColor` class. Instead of just the one `IsIndex` type, there is
now an `IsIndex16` and an `IsIndex256`. `IsIndex16` covers the eight
original ANSI colors set with `SGR 3x` and `SGR 4x`, as well as the
brighter aixterm variants set with `SGR 9x` and `SGR 10x`. `IsIndex256`
covers the 256 ISO/ITU indexed colors set with `SGR 38;5` and `SGR
48;5`.
There are two reasons for this distinction. The first is that the ANSI
colors have the potential to be brightened by the `SGR 1` bold
attribute, while the ISO/ITO color do not. The second reason is that
when forwarding an attributes through conpty, we want to try and
preserve the original SGR sequence that generated each color (to the
extent that that is possible). By having the two separate types, we can
map the `IsIndex16` colors back to ANSI/aixterm values, and `IsIndex256`
to the ISO/ITU sequences.
In addition to the VT colors, we also have to deal with the legacy
colors set by the Windows console APIs, but we don't really need a
separate type for those. It seemed most appropriate to me to store them
as `IsIndex256` colors, since it doesn't make sense to have them
brightened by the `SGR 1` attribute (which is what would happen if they
were stored as `IsIndex16`). If a console app wanted a bright color it
would have selected one, so we shouldn't be messing with that choice.
The second part of the PR was the unification of the two color tables.
Originally we had a 16 color table for the legacy colors, and a separate
table for the 256 ISO/ITU colors. These have now been merged into one,
so color table lookups no longer need to decide which of the two tables
they should be referencing. I've also updated all the methods that took
a color table as a parameter to use a `basic_string_view` instead of
separate pointer and length variables, which I think makes them a lot
easier and safer to work with.
With this new architecture in place, I could now update the
`AdaptDispatch` SGR implementation to store the ISO/ITU indexed colors
as `IsIndex256` values, where before they were mapped to RGB values
(which prevented them reflecting any color scheme changes). I could also
update the `TerminalDispatch` implementation to differentiate between
the two index types, so that the `SGR 1` brightening would only be
applied to the ANSI colors.
I've also done a bit of code refactoring to try and minimise any direct
access to the color tables, getting rid of a lot of places that were
copying tables with `memmove` operations. I'm hoping this will make it
easier for us to update the code in the future if we want to reorder the
table entries (which is likely a requirement for unifying the
`AdaptDispatch` and `TerminalDispatch` implementations).
## Validation Steps Performed
For testing, I've just updated the existing unit tests to account for
the API changes. The `TextColorTests` required an extra parameter
specifying the index type when setting an index. And the `AdapterTest`
and `ScreenBufferTests` required the use of the new `SetIndexedXXX`
methods in order to be explicit about the index type, instead of relying
on the `TextAttribute` constructor and the old `SetForeground` and
`SetBackground` methods which didn't have a way to differentiate index
types.
I've manually tested the various console APIs
(`SetConsoleTextAttribute`, `ReadConsoleOutputAttribute`, and
`ReadConsoleOutput`), to make sure they are still setting and reading
the attributes as well as they used to. And I've tested the
`SetConsoleScreenBufferInfoEx` and `GetConsoleScreenBufferInfoEx` APIs
to make sure they can read and write the color table correctly. I've
also tested the color table in the properties dialog, made sure it was
saved and restored from the registry correctly, and similarly saved and
restored from a shortcut link.
Note that there are still a bunch of issues with the color table APIs,
but no new problems have been introduced by the changes in this PR, as
far as I could tell.
I've also done a bunch of manual tests of `OSC 4` to make sure it's
updating all the colors correctly (at least in conhost), and confirmed
that the test case in issue #1223 now works as expected.
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
Web apps apparently will paste the <title> as plaintext before the
actual HTML content from the clipboard. Since this seems to be
widespread behavior across web apps, this isn't just a bug in _some
app_, this is a bug on us. We shouldn't emit the title.
This PR removes the title tag from the generated HTML.
Closes#5347
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
In tonight's episode of "I wanna my CPU back", we'll see a quite familiar face whose name is Mr.AttrRow.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
#2937
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
I knew this is possible a long time ago. Just didn't got the chance to actually implement this. I understand that you guys are busy preparing the v1.0 release. So if this is a bad time, this can wait.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Manually tested. If all goes well, nothing will be broken.
