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.
Some sources files changes were necessary to retain build continuity in OS.
Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_onecore_dep_uxp 7ff895ff770838526b6d1d9e7d582f3c1e36d85b
There are certain cursor movement operations (in conhost) that can
result in "ghost" cursor instances being left behind, if the move causes
the viewport to scroll while the cursor is blinking off. Pressing enter
in a PowerShell prompt when at the bottom of the screen was one example
of this. This PR fixes that problem.
Whenever the cursor renders with an `InvertRect`, the affected areas of
the screen are saved in the `cursorInvertRects` variable. If the screen
is then scrolled while the cursor is visible, those rects are
"uninverted" in the `GdiEngine::ScrollFrame` method before the scrolling
takes place.
When the cursor has blinked off, though, the `GdiEngine::PaintCursor`
method won't set the `cursorInvertRects` variable, but it also doesn't
clear it. So if the screen is scrolled at that point, the `ScrollFrame`
method tries to "uninvert" the area where the cursor had previously been
painted. And since the cursor is no longer there, this has the opposite
effect, leaving an unwanted mark on the screen.
I've fixed this by clearing the `cursorInvertRects` at the start of the
paint cycle, in the the `GdiEngine::PaintBackground` method. Since this
occurs after the `ScrollFrame` step, it still allows for legitimate
cursor instances to be cleaned up when scrolling, but makes sure that
the variable will be cleared for the next cycle if the cursor is no
longer visible.
## Validation Steps Performed
I've manually verified that I no longer see ghost cursor fragments when
scrolling in PowerShell.
Closes#804
This pull request switches up the treatment we use for pattern-detected
links and OSC 8 hyperlinks:
* Links generated via OSC 8 have a sparse dotted underline instead of a
thick dashed one
* Links generated by pattern detection _are not underlined until they've
hovered_
* This papers over a visual glitch that is a result of us updating
the pattern matches every ~500ms (on change)
Closes#8123
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
Adds the ability to manually handle the terminal renderer resizing
events by allowing different render size and WPF control size. This is
done by adding an `AutoFill` property to the control that prevents the
renderer from automatically resizing and tells the WPF control to fill
in the extra space with the terminal background as shown below:
This PR adds the following:
- Helper method in the DX engine to convert character viewports into
pixel viewports
- `AutoFill` property that prevents automatic resizing of the renderer
- Tweaks and fixes that automatically fill in the empty space if
`AutoFill` is set to false
- Fixes resizing methods and streamlines their codepath
## Validation Steps Performed
Manual validation with the Visual Studio Integrated Terminal tool
window.
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
- Render hyperlinks with a dashed underline
- Render hovered hyperlinks with a solid underline
- Show URI tooltip on hover
TermControl now has a canvas that contains a tiny border to which a
tooltip is attached. When we hover over hyperlinked text, we move the
border to the mouse location and update the tooltip content with the
URI.
Introduced a new underline type (HyperlinkUnderline), supports rendering
for it, and uses it to render hyperlinks. HyperlinkUnderline is usually
a dashed underline, but when a link is hovered, all text with the same
hyperlink ID is rendered with a solid underline.
References #5001
<!-- 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
This commit removes our local copy of clang-format 8 and replaces it
with a newly-built nuget package containing clang-format 10.
This resolves the inconsistency between our version of clang-format and
the one shipped in Visual Studio.
A couple minor format changes were either required or erroneously forced
upon us--chief among them is a redistribution of `*`s around SAL
annotations in inline class members of COM classes. Don't ask why; I
couldn't figure it out.
We had some aspirational goals for our formatting, which were left in
but commented out. Enabling them changes our format a little more than
I'm comfortable with, so I uncommented them and locked them to the
format style we've been using for the past year. We may not love it, but
our aspirations may not matter here any longer. Consistent formatting is
better than perfect formatting.
Adds the ability to set the selection background opacity when setting the
selection background. This also exposes the selection background and alpha
through the terminal WPF container.
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
Consecutive RTL GlyphRuns are drawn from the last to the first.
References
#538, #7149, all those issues asking for RTL closed as dupes.
As @miniksa suggested in a comment on #7149 -- handle the thingy on the
render side.
If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in
order abcdGFEh.
This has ransom-noting, because I didn't touch the font scaling at all.
This should fix the majority of RTL issues, except it *doesn't* fix
issues with colors, because those get split in the TextBuffer phase in
the renderer I think, so they show up separately by the GlyphRun phase.
Carlos Zamora (1)
* UIA: use full buffer comparison in rects and endpoint setter (GH-6447)
Dan Thompson (2)
* Tweaks: normalize TextAttribute method names (adjective form) (GH-6951)
* Fix 'bcz exclusive' typo (GH-6938)
Dustin L. Howett (4)
* Fix VT mouse capture issues in Terminal and conhost (GH-7166)
* version: bump to 1.3 on master
* Update Cascadia Code to 2007.15 (GH-6958)
* Move to the TerminalDependencies NuGet feed (GH-6954)
James Holderness (3)
* Render the SGR "underlined" attribute in the style of the font (CC-7148)
* Add support for the "crossed-out" graphic rendition attribute (CC-7143)
* Refactor grid line renderers with support for more line types (CC-7107)
Leonard Hecker (1)
* Added til::spsc, a lock-free, single-producer/-consumer FIFO queue (CC-6751)
Michael Niksa (6)
* Update TAEF to 10.57.200731005-develop (GH-7164)
* Skip DX invalidation if we've already scrolled an entire screen worth of height (GH-6922)
* Commit attr runs less frequently by accumulating length of color run (GH-6919)
* Set memory order on slow atomics (GH-6920)
* Cache the viewport to make invalidation faster (GH-6918)
* Correct comment in this SPSC test as a quick follow up to merge.
Related work items: MSFT-28208358
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
This PR adds support for the ANSI _crossed-out_ graphic rendition
attribute, which is enabled by the `SGR 9` escape sequence.
* Support for the escape sequences and storage of the attribute was
originally added in #2917.
* Support for drawing the strikethrough effect in the grid line renderer
was added in #7107.
Since the majority of the code required for this attribute had already
been implemented, it was just a matter of activating the
`GridLines::Strikethrough` style in the `Renderer::s_GetGridlines`
method when the `CrossedOut` attribute was set.
VALIDATION
There were already some unit tests in place in `VtRendererTest` and the
`ScreenBufferTests`, but I've also now extended the SGR tests in
`AdapterTest` to cover this attribute.
I've manually confirmed the first test case from #6205 now works as
expected in both conhost and Terminal.
Closes#6205
Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_onecore_dep_uxp 29b1a1d663d0047dc0d2125dd05f75959bca27ef
Related work items: MSFT:27866336
This is a refactoring of the grid line renderers, adjusting the line
widths to scale with the font size, and optimising the implementation to
cut down on the number of draw calls. It also extends the supported grid
line types to include true underlines and strike-through lines in the
style of the active font.
The main gist of the optimisation was to render the horizontal lines
with a single draw call, instead of a loop with lots of little strokes
joined together. In the case of the vertical lines, which still needed
to be handled in a loop, I've tried to move the majority of static
calculations outside the loop, so there is bit of optimisation there
too.
At the same time this code was updated to support a variable stroke
width for the lines, instead of having them hardcoded to 1 pixel. The
width is now calculated as a fraction of the font size (0.025 "em"),
which is still going to be 1 pixel wide in most typical usage, but will
scale up appropriately if you zoom in far enough.
And in preparation for supporting the SGR strike-through attribute, and
true underlines, I've extended the grid line renders with options for
handling those line types as well. The offset and thickness of the lines
is obtained from the font metrics (rounded to a pixel width, with a
minimum of one pixel), so they match the style of the font.
VALIDATION
For now we're still only rendering grid lines, and only the top and
bottom lines in the case of the DirectX renderer in Windows Terminal. So
to test, I hacked in some code to force the renderer to use all the
different options, confirming that they were working in both the GDI and
DirectX renderers.
I've tested the output with a number of different fonts, comparing it
with the same text rendered in WordPad. For the most part they match
exactly, but there can be slight differences when we adjust the font
size for grid alignment. And in the case of the GDI renderer, where
we're working with pixel heights rather than points, it's difficult to
match the sizes exactly.
This is a first step towards supporting the strike-through attribute
(#6205) and true underlines (#2915).
Closes#6911
We spend a lot of time invalidating in the DX Renderer. This is a
creative trick to not bother invalidating any further if we can tell
that the bitmap is already completely invalidated. That is, if we've
scrolled at least an entire screen in height... then the entire bitmap
had to have been marked as invalid as the new areas were "uncovered" by
the `InvalidateScroll` command. So further setting invalid bits on top
of a fully invalid map is pointless.
Note: I didn't use `bitmap::all()` here because it is significantly
slower to check all the bits than it is to just reason out that the
bitmap was already fully marked.
## Validation Steps Performed
- Run `time cat big.txt`. Checked average time before/after, WPR traces
before/after.
By default, the memory order on atomics is `seq_cst`. This is a relatively expensive ordering and it shows in situations where we're rapidly signaling a consumer to pick up something from a producer. I've instead attempted to switch these to `release` (producer) and `acquire` (consumer) to improve the performance of these signals.
## Validation Steps Performed
- Run `time cat big.txt` and `time cat ls.txt` under VS Performance Profiler.
## 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
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.
In `Renderer::TriggerRedraw`, the act of fetching the viewport from the
`pData` over and over is wasted time. We already have a cached variable
of the viewport that is updated on every scroll check (on
`TriggerScroll` and on `PaintFrame`.) Scrolling wouldn't be working
correctly if the clients weren't already notifying us that the viewport
has changed for scroll purposes, so we can just keep using that cached
value for the invalidation restriction to speed things up over fetching
it again.
## Validation Steps Performed
- Run `time cat big.txt`. Checked average time before/after, WPR traces
before/after.
## PR Checklist
* [x] Closes perf itch
* [x] I work here
* [x] Manual test
* [x] Documentation irrelevant.
* [x] Schema irrelevant.
* [x] Am core contributor.
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.
GSL 3, the next major version of GSL after the one we're using, replaced
their local implementation of `span` with one that more closely mimics
C++20's span. Unfortunately, that is a breaking change for all of GSL's
consumers.
This commit updates our use of span to comply with the new changes in
GSL 3.
Chief among those breaking changes is:
* `span::at` no longer exists; I replaced many instances of `span::at`
with `gsl::at(x)`
* `span::size_type` has finally given up on `ptrdiff_t` and become
`size_t` like all other containers
While I was here, I also made the following mechanical replacements:
* In some of our "early standardized" code, we used std::optional's
`has_value` and `value` back-to-back. Each `value` incurs an
additional presence test.
* Change: `x.value().member` -> `x->member` (`optional::operator->`
skips the presence test)
* Change: `x.value()` -> `*x` (as above)
* GSL 3 uses `size_t` for `size_type`.
* Change: `gsl::narrow<size_t>(x.size())` -> `x.size()`
* Change: `gsl::narrow<ptrdiff_t>(nonSpan.size())` -> `nonSpan.size()`
during span construction
I also replaced two instances of `x[x.size() - 1]` with `x.back()` and
one instance of a manual array walk (for comparison) with a direct
comparison.
NOTE: Span comparison and `make_span` are not part of the C++20 span
library.
Fixes#6251
## 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.
A lot of time was spent between each individual line in the VT paint
engine in allocating some scratch space to assemble the clusters then
deallocating it only to have the next line do that again. Now we just
hold onto that memory space since it should be approximately the size of
a single line wide and will be used over and over and over as painting
continues.
## 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 perf test.
* [x] Documentation irrelevant.
* [x] Schema irrelevant.
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
Passes the bitmap by ref into the tracing function instead of making a copy on the way in. It's only read anyway for tracing (if enabled) so the copy was a pointless oversight.
## Validation Steps Performed
- Observed WPR trace before and after with `time cat big.txt` in WSL.
## 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.
Pretty straightforward. `toggleRetroEffect` will work to toggle the
retro terminal effect on/off.
* Made possible by contributions from #6551, _and viewers like you_
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.
Restores the simple text run analysis and skipping of most of the
shaping/layout steps. Corrects one of the fast-path steps to ensure that
offsets and clusters are assigned.
## References
- Bug #6488
- Bug #6664
- Simple run PR #6206
- Simple run revert PR #6665
- Recycle glyph runs PR #6483
The "correction" functions, by which box drawing analysis is one of
them, is dependent on the steps coming before it properly assigning the
four main vectors of the text layout glyphs: indices, advances, offsets,
and clusters. When the fast path is identified by the code from #6206,
only two of those are fully updated: indices and advances. The offsets
doesn't tend to cause a problem because offsets are rarely used so
they're pretty much always 0 already (but this PR enforces that they're
zero for the simple/fast path.) The clusters, however, were not mapped
for the fast path. This showed itself in one of two ways:
1. Before the recycled runs PR #6483, the cluster map had a 0 in every
field for the stock initialized vector.
2. After the recycled runs PR #6483, the cluster map had the previous
run's mapping in it.
This meant that when we reached the steps where glyph runs were
potentially split during the correction phase for box drawing
characters, unexpected values were present to map the glyph indices to
clusters and were corrected, adjusted, or split in an unexpected
fashion.
For instance, the index out of range bug could appear when the default 0
values appended to the end of the clusters vector were decremented down
to a negative value during the run splitter as the true DWrite cluster
mapper doesn't generate that sort of pattern in the slow path case
without also breaking the run itself.
The resolution here is therefore to ensure that all of the fields
related to glyph layout are populated even in the fast path. This
doesn't affect the slow path because that one always populated all
fields by asking DWrite to do it. The fast path just skips a bunch of
DWrite steps because it can implicitly identify patterns and save a
bunch of time.
I've also identified a few vectors that weren't cleared on reset/reuse
of the layout. I'm clearing those now so the `.resize()` operations
performed on them to get to the correct lengths will fill them with
fresh and empty values instead of hanging on to ones that may have been
from the previous. This should be OK memory perf wise because the act of
`.clear()` on a vector shouldn't free anything, just mark it invalid.
And doing `.resize()` from an empty one should just default construct
them into already allocated space (which ought to be super quick).
## Validation
* [x] Far.exe doesn't crash and looks fine
* [x] "\e[30;47m\u{2500} What \u{2500}\e[m" from #6488 appears
appropriately antialiased
* [x] Validate the "\e[30;47m\u{2500} What \u{2500}\e[m" still works
when `FillGeometry` is nerfed as a quick test that the runs are split
correctly.
* [x] Put `u{fffd} into Powershell Core to make a replacement char in
the output. Then press enter a few times and see that shrunken initial
characters on random rows. Verify this is gone.
Closes#6668Closes#6669
Co-Authored-By: Chester Liu <skyline75489@outlook.com>
This reverts commit 94eab6e391.
We'll reintroduce this again after making sure it plays nicely with
recycling and box drawing glyphs.
Fixes#6488Fixes#6664
This saves an awful lot of construction/destruction and memory
allocation, especially with text that changes a lot (see: cacafire).
Three things:
1. Recycling the text layouts. This holds onto the `CustomTextLayout` so
all the things that don't change related to drawing targets and
whatnot aren't freed and recreated every frame.
2. Reordering the runs in place. This saves a vector
allocation/copy/delete every time OrderRuns is called. They can be
rearranged in place.
3. Only clip once per row. This reduces the clip push/pop to only one
time per row. Since we're always redrawing an entire row at a time,
this saves a lot of alloc/free of the clip frame, dramatically
reduces queued commands, and makes less work on the flush since
clipping requires staging the drawing and then bringing it back to
the main surface.
I was told that the DeviceContext version supercedes the RenderTarget
one. This moves us to it so we can gain access to a higher level of
control over the various pieces in our pipeline as we continue to evolve
the renderer.
The underlying motivation here is to potentially use a
`ID2D1CommandList` to batch our commands and run them all later outside
the lock. That can only really be done with this more granular level of
control over the pipeline. So this moves to that in a single step that
is easily findable in history should we have problems
I discussed this with @NiklasBorson of the Direct2D/DirectWrite team as
well as with @DHowett before doing it.
## Validation
- [x] Checked docs to make sure that these work on Windows 7 with
Platform Update
- [x] Manual smoke test real quick
- [ ] Try running on Win7 + Platform Update after change
- [x] Probably do more than just a smoke test manually or otherwise
Closes#6525
## Summary of the Pull Request
## PR Checklist
* [x] Closes#3927
* [x] I work here.
* [x] Tested manually.
* [x] Requires documentation to be updated: (generate doc bug here)
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
- I found four settings that weren't hot reloadable with the 3927 comment above them:
1. Experimental retro terminal effect
2. Experimental software rendering
3. Experimental full repaint rendering
4. Antialiasing settings for text
I made them all hot reloadable by telling the `TermControl` to propagate them on settings change to the `DxEngine`.
Then I set up the `DxEngine` inside the setters to only set them if they changed. And if they do change, to trigger a full repaint and/or a complete drop and recreate of the entire DX device chain (as would happen if it were lost for another reason like a user-mode graphics failure, disconnected display, etc.)
I made the boolean an atomic because the settings can be coming in off of another thread (the XAML eventing one) and the renderer is picking the status up on its thread at the top of the BeginPaint frame.
## Validation Steps Performed
- [x] Opened it up and toggled all the settings while staring at PowerShell
- [x] Opened it up and toggled all the settings while staring at something intensive like a `cacafire` fire
* [wpf] WM_KEYUP crashes on x64 #6444
- Turns out that doing the `(uint)lParam` cast worked fine for the
keydowns, because the value of lParam usually didn't have super
high-order bits set. That's not the case for keyups, where the 30th
bit is _always_ set. This is fixed by explicitly getting the byte
with the scancode in it.
* [wpf] WM_KEYUP generates wrong value in Win32 input mode #6445
- This was fixed by basically the same thing as the above.
* [wpf] WPF control crashes on startup trying to render cursor #6446
- This was a regression from #6337. I forgot to initialize the brush
used to paint the cursor, because the UWP version always uses color
(but the WPF one relies on the text foreground color).
* Also adds a minor change to the WPF test app, so that the user can
actually exit `win32-input-mode`.
* #6337 regressed #6446
* #6309 regressed the other two.
Closes#6444Closes#6445Closes#6446
This pull request reduces input lag, especially with selection, by using
`IDXGISwapChain2::GetFrameLatencyWaitableObject`.
This is based on the [DXGI 1.3 documentation].
Excerpt from the [DXGI 1.3 improvement list]:
> The following functionality has been added in Microsoft DirectX
Graphics Infrastructure (DXGI) 1.3, which is included starting in
Windows 8.1.
Before, during rendering:
1. render frame
2. call `Present` on swap chain:
2.a. blocks until it can present
2.b. meanwhile, selection/text in terminal might have changed, but
we're still using the frame that we rendered before blocking
2.c. presents
After, during rendering:
1. block until we can present
2. render frame with latest data
3. call `Present` on swap chain:
3.a. present without blocking
[DXGI 1.3 documentation]: https://docs.microsoft.com/en-us/windows/uwp/gaming/reduce-latency-with-dxgi-1-3-swap-chains
[DXGI 1.3 improvement list]: https://docs.microsoft.com/en-us/windows/win32/direct3ddxgi/dxgi-1-3-improvements:
## Summary of the Pull Request
Caches vectors in the class and uses a new helper to opportunistically shrink/grow as viewport sizes change in order to save performance on alloc/free of commonly used vectors.
## PR Checklist
* [x] Scratches a perf itch.
* [x] I work here.
* [x] wil tests added
* [x] No add'l doc.
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
Two fixes:
1. For outputting lots of text, the base renderer class spent a lot of time allocating and freeing and reallocating the `Cluster` vector that adapts the text buffer information into render clusters. I've now cached this vector in the base render class itself and I shrink/grow it based on the viewport update that happens at the top of every frame. To prevent too much thrashing in the downward/shrink direction, I wrote the `til::manage_vector` helper that contains a threshold to only shrink if it asks for small enough of a size relative to the existing one. I used 80% of the existing size as the threshold for this one.
2. For outputting lots of changing colors, the VT graphics output engine spent a bunch of time allocating and reallocating the vector for `GraphicsOptions`. This one doesn't really have a predictable size, but I never expect it to get extremely big. So I just held it in the base class.
## Validation Steps Performed
* [x] Ran the til unit test
* [x] Checked render cluster vector time before/after against `big.txt` from #1064
* [x] Checked VT graphics output vector time before/after against `cacafire`
Case | Before | After
---|---|---|
`big.txt` |  | 
`cacafire` |  | 
Adds support for `win32-input-mode` to conhost, conpty, and the Windows
Terminal.
* The shared `terminalInput` class supports sending these sequences when
a VT client application requests this mode.
* ConPTY supports synthesizing `INPUT_RECORD`s from the input sent to it
from a terminal
* ConPTY requests this mode immediately on startup (if started with a
new flag, `PSEUDOCONSOLE_WIN32_INPUT_MODE`)
* The Terminal now supports sending this input as well, when conpty asks
for it.
Also adds a new ConPTY flag `PSEUDOCONSOLE_WIN32_INPUT_MODE` which
requests this functionality from conpty, and the Terminal requests this
by default.
Also adds `experimental.input.forceVT` as a global setting to let a user
opt-out of this behavior, if they don't want it / this ends up breaking
horribly.
## Validation Steps Performed
* played with this mode in vtpipeterm
* played with this mode in Terminal
* checked a bunch of scenarios, as outlined in a [comment] on #4999
[comment]: https://github.com/microsoft/terminal/issues/4999#issuecomment-628718631
References #4999: The megathread
References #5887: The spec
Closes#879Closes#2865Closes#530Closes#3079Closes#1119Closes#1694Closes#3608Closes#4334Closes#4446
## Summary of the Pull Request
This is the plan that @miniksa suggested to me. Instead of trying to do lots of work in all the renderers to do backgrounds as one pass, and foregrounds as another, we can localize this change to basically just the DX renderer.
1. First, we give the DX engine a "heads up" on where the cursor is going to be drawn during the frame, in `PrepareRenderInfo`.
- This function is left unimplemented in the other render engines.
2. While printing runs of text, the DX renderer will try to paint the cursor in `CustomTextRenderer::DrawGlyphRun` INSTEAD of `DxEngine::PaintCursor`. This lets us weave the cursor background between the text background and the text.
## References
* #6151 was a spec in this general area. I should probably go back and update it, and we should probably approve that first.
* #6193 is also right up in this mess
## PR Checklist
* [x] Closes#1203
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
* This is essentially `"cursorTextColor": "textForeground"` from #6151.
* A follow up work item is needed to add support for the current behavior, (`"cursorTextColor": null`), and hooking up that setting to the renderer.
## Summary of the Pull Request
Restores proper line drawing during IME operations in `conhost`
## PR Checklist
* [x] Closes#803
* [x] I work here.
* [x] Tested manually.
* [x] Check the performance of this and see if it's worse-enough to merit a more confusing algorithm. It was worse for the majority case so I scoped it.
* [x] No doc, it should have worked this way.
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
- Changed `ConsoleImeInfo::s_ConvertToCells` to be less confusing. It's doing about the same thing, but it's way easier to read now and the compiler/linker/optimizer should just be the same.
- Edited `Renderer::_PaintBufferOutputHelper` to check each attribute for line drawing characters as the right half of a two-col character might have different line drawing characters than the left-half.
## Validation Steps Performed
- [x] Manual operation of IME in conhost with Japanese IME.
- [x] Manual operation of IME in conhost with Chinese IME.
- [x] Manual operation of IME in conhost with Chinese (Traditional) IME.
- [x] Manual operation of IME in conhost with and Korean IME. - @leonMSFT says Korean doesn't work this way. But Korean is broken worse in that it's not showing suggestions at all. Filing new bug. #6227
- [x] Validated against API-filling calls through `SetConsoleTextAttribute` per @j4james's sample code
## 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 PR reverts a relatively minor change that was made incorrectly to
ConPTY in #5771.
In that PR, I authored two tests. One of them actually caught the bug
that was supposed to be fixed by #5771. The other test was simply
authored during the investigation. I believed at the time that the test
revealed a bug in conpty that was fixed by _removing_ this block of
code. However, an investigation itno #5839 revealed that this code was
actually fairly critical.
So, I'm also _skipping_ this buggy test for now. I'm also adding a
specific test case to this bug.
The problem in the bugged case of `WrapNewLineAtBottom` is that
`WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer,
which is causing the cursor to automatically move to the next line in
the buffer. This is because `WriteCharsLegacy` isn't being called with
the `WC_DELAY_EOL_WRAP` flag. So, in that test case,
* The client emits a wrapped line to conpty
* conpty fills the bottom line with that text, then dutifully increments
the buffer to make space for the cursor on a _new_ bottom line.
* Conpty reprints the last `~` of the wrapped line
* Then it gets to the next line, which is being painted _before_ the
client emits the rest of the line of text to fill that row.
* Conpty thinks this row is empty, (it is) and manually breaks the row.
However, the test expects this row to be emitted as wrapped. The problem
comes from the torn state in the middle of these frames - the original
line probably _should_ remain wrapped, but this is a sufficiently rare
case that the fix is being punted into the next release.
It's possible that improving how we handle line wrapping might also fix
this case - currently we're only marking a row as wrapped when we print
the last cell of a row, but we should probably mark it as wrapped
instead when we print the first char of the _following_ row. That work
is being tracked in #5800
### The real bug in this PR
The problem in the `DeleteWrappedWord` test is that the first line is
still being marked as wrapped. So when we get to painting the line below
it, we'll see that there are no characters to be printed (only spaces),
we emit a `^[20X^[20C`, but the cursor is still at the end of the first
line. Because it's there, we don't actually clear the text we want to
clear.
So DeleteWrappedWord, #5839 needs the `_wrappedRow = std::nullopt;`
statement here.
## References
* I guess just look at #5800, I put everything in there.
## Validation Steps Performed
* Tested manually that this was fixed for the Terminal
* ran tests
Closes#5839
## Summary of the Pull Request
Adds user settings to adjust rendering behavior to mitigate blurry text on some devices.
## References
- #778 introduced this, almost certainly.
## PR Checklist
* [x] Closes#5759, mostly
* [x] I work here.
* [ ] We need community verification that this will help.
* [x] Updated schema and schema doc.
* [x] Am core contributor. Discussed in Monday sync meeting and w/ @DHowett-MSFT.
## Detailed Description of the Pull Request / Additional comments
When we switched from full-screen repaints to incremental rendering, it seems like we exposed a situation where some display drivers and hardware combinations do not handle scroll and/or dirty regions (from `IDXGISwapChain::Present1`) without blurring the data from the previous frame. As we're really close to ship, I'm offering two options to let people in this situation escape it on their own. We hope in the future to figure out what's actually going on here and mitigate it further in software, but until then, these escape hatches are available.
1. `experimental.rendering.forceFullRepaint` - This one restores the pre-778 behavior to the Terminal. On every single frame paint, we'll invalidate the entire screen and repaint it.
2. `experimental.rendering.software` - This one uses the software WARP renderer instead of using the hardware and display driver directly. The theory is that this will sidestep any driver bugs or hardware variations.
One, the other, or both of these may be field-applied by users who are experiencing this behavior.
Reverting #778 completely would also resolve this, but it would give back our largest performance win in the whole Terminal project. We don't believe that's acceptable when seemingly a majority of the users are experiencing the performance benefit with no detriment to graphical display.
## Validation Steps Performed
- [x] Flipped them on and verified with the debugger that they are being applied to the rendering pipeline
- [ ] Gave a private copy to community members in #5759 and had them try whether one, the other, or both resolved their issue.
## Summary of the Pull Request
This PR resolves an issue with the Git for Windows (MSYS) version of `less`. It _doesn't_ use VT processing for emitting text tothe buffer, so when it hits `WriteCharsLegacy`, `WC_DELAY_EOL_WRAP` is NOT set.
When this happens, `less` is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the
```c++
Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
```
call in `_stream.cpp:560`.
The cursor is _currently_ at `{40, 29}`, the _start_ of the run of text that wrapped. We're trying to adjust it to `{0, 30}`, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to `IncrementCircularBuffer` first, so we can move the cursor there.
When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at `{40, 29}` here, so unfortunately, the `cursorIsInDeferredWrap` check in `XtermEngine::PaintCursor` is `false`. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is `{40, 29}`.
If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.
## PR Checklist
* [x] Closes#5691
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
I suppose that's the detailed description above
## Validation Steps Performed
* ran tests
* checked that the bug was actually fixed in the Terminal
## Summary of the Pull Request
Identifies and scales glyphs in the box and line drawing ranges U+2500-U+259F to fit their cells.
## PR Checklist
* [x] Closes#455
* [x] I work here.
* [x] Manual tests. This is all graphical.
* [x] Metric ton of comments
* [x] Math spreadsheet included in PR.
* [x] Double check RTL glyphs.
* [x] Why is there the extra pixel?
* [x] Scrolling the mouse wheel check is done.
* [x] Not drawing outline?
* [x] Am core contributor. Roar.
* [x] Try suppressing negative scale factors and see if that gets rid of weird shading.
## Detailed Description of the Pull Request / Additional comments
### Background
- We want the Terminal to be fast at drawing. To be fast at drawing, we perform differential drawing, or only drawing what is different from the previous frame. We use DXGI's `Present1` method to help us with this as it helps us compose only the deltas onto the previous frame at drawing time and assists us in scrolling regions from the previous frame without intervention. However, it only works on strictly integer pixel row heights.
- Most of the hit testing and size-calculation logic in both the `conhost` and the Terminal products are based on the size of an individual cell. Historically, a cell was always dictated in a `COORD` structure, or two `SHORT` values... which are integers. As such, when we specify the space for any individual glyph to be displayed inside our terminal drawing region, we want it to fall perfectly inside of an integer box to ensure all these other algorithms work correctly and continue to do so.
- Finally, we want the Terminal to have font fallback and locate glyphs that aren't in the primary selected font from any other font it can find on the system that contains the glyph, per DirectWrite's font fallback mechanisms. These glyphs won't necessarily have the same font or glyph metrics as the base font, but we need them to fit inside the same cell dimensions as if they did because the hit testing and other algorithms aren't aware of which particular font is sourcing each glyph, just the dimensions of the bounding box per cell.
### How does Terminal deal with this?
- When we select a font, we perform some calculations using the design metrics of the font and glyphs to determine how we could fit them inside a cell with integer dimensions. Our process here is that we take the requested font size (which is generally a proxy for height), find the matching glyph width for that height then round it to an integer. We back convert from that now integer width to a height value which is almost certainly now a floating point number. But because we need an integer box value, we add line padding above and below the glyphs to ensure that the height is an integer as well as the width. Finally, we don't add the padding strictly equally. We attempt to align the English baseline of the glyph box directly onto an integer pixel multiple so most characters sit crisply on a line when displayed.
- Note that fonts and their glyphs have a prescribed baseline, line gap, and advance values. We use those as guidelines to get us started, but then to meet our requirements, we pad out from those. This results in fonts that should be properly authored showing gaps. It also results in fonts that are improperly authored looking even worse than they normally would.
### Now how does block and line drawing come in?
- Block and Line drawing glyphs are generally authored so they will look fine when the font and glyph metrics are followed exactly as prescribed by the font. (For some fonts, this still isn't true and we want them to look fine anyway.)
- When we add additional padding or rounding to make glyphs fit inside of a cell, we can be adding more space than was prescribed around these glyphs. This can cause a gap to be visible.
- Additionally, when we move things like baselines to land on a perfect integer pixel, we may be drawing a glyph lower in the bounding box than was prescribed originally.
### And how do we solve it?
- We identify all glyphs in the line and block drawing ranges.
- We find the bounding boxes of both the cell and the glyph.
- We compare the height of the glyph to the height of the cell to see if we need to scale. We prescribe a scale transform if the glyph wouldn't be tall enough to fit the box. (We leave it alone otherwise as some glyphs intentionally overscan the box and scaling them can cause banding effects.)
- We inspect the overhang/underhang above and below the boxes and translate transform them (slide them) so they cover the entire cell area.
- We repeat the previous two steps but in the horizontal direction.
## Validation Steps Performed
- See these commments:
- https://github.com/microsoft/terminal/issues/455#issuecomment-620248375
- https://github.com/microsoft/terminal/issues/455#issuecomment-621533916
- https://github.com/microsoft/terminal/issues/455#issuecomment-622585453
Also see the below one with more screenshots:
- https://github.com/microsoft/terminal/pull/5743#issuecomment-624940567
[Git2Git] Merged PR 4644345: conhost: disable the DX renderer in inbox builds
We're going to be taking on some changes to the Dx renderer that are at
the very least annoying and at the very most inconsequential to the
inbox console. This commit removes support for the DX renderer from the
inbox console.
SizeBench reports that ConRenderDx contributes 55.1kb to the conhost
image (as its third largest constituent library), so this should net us
a couple pleasant WPG improvements down the line.
Related work items: #26291552 Retrieved from https://microsoft.visualstudio.com os OS official/rs_onecore_dep_uxp 6e36786d447b7975298ba31ccd77c5c649fbfbe6
Related work items: #26291552
<!-- 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
This fixes the RTL regression caused in #4747. We create the rectangle taking the direction (through the BiDi Level) into account, and then the rendering works again. The GlyphRun shaping could still probably use some work to be a polished thingy, and there are still issues with RTL getting chopped up a lot when there's font fallback going on, but this fixes the regression, and it's now functional again.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
#4779#4747
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#4779
* [x] 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
The baseline is actually direction dependent. So when it was being initialized, the unconditional baseline as left broke it, setting the box off to right of the text. We just check if the `GlyphRun->bidiLevel` is set, and if so, we adjust it so that the baseline lines up with the right, not with the left.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
## Summary of the Pull Request
This PR implements a pair of shims for `cmd` and `powershell`, so that their `cls` and `Clear-Host` functions will clear the entire terminal buffer (like they do in conhost), instead of just the viewport. With the conpty viewport and buffer being the same size, there's effectively no way to know if an application is calling these API's in this way with the intention of clearing the buffer or the viewport. We absolutely have to guess.
Each of these shims checks to see if the way that the API is being called exactly matches the way `cmd` or `powershell` would call these APIs. If it does, we manually write a `^[[3J` to the connected terminal, to get he Terminal to clear it's own scrollback.
~~_⚠️ If another application were trying to clear the **viewport** with an exactly similar API call, this would also cause the terminal scrollback to get cleared ⚠️_~~
* [x] Should these shims be restricted to when the process that's calling them is actually `cmd.exe` or `powershell.exe`? Can I even do this? I think we've done such a good job of isolating the client process information from the rest of the host code that I can't figure out how to do this.
- YES, this can be done, and I did it.
* [ ] **TODO**: _While I'm here_, should I have `DoSrvPrivateEraseAll` (the implementation for `^[[2J`, in `getset.cpp`) also manually trigger a EraseAll in the terminal in conpty mode?
## PR Checklist
* [x] Closes#3126
* [x] Actually closes#1305 too, which is really the same thing, but probably deserves a callout
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Validation Steps Performed
* ran tests
* checked `cls` in the Terminal
* checked `Clear-Host` in the Terminal
* Checked running `powershell clear-host` from `cmd.exe`
Turns out we're still being a bit too aggressive when removing spaces.
If there are spaces at the end of the first run painted to a bottom
line, _and the bottom line was a different color than the previous_,
then we can't trim those spaces off the string. We still need to emit
those to make sure the terminal has colored spaces in it as well.
## References
* there's like 80 PRs in the last month for this function
## PR Checklist
* [x] Closes#5502
* [x] I work here
* [x] Tests added/passed
## Validation Steps
* [x] ran the tests
* [x] checked that vtpipeterm still worked
* [x] Checked that the bug was fixed in the Terminal
<!-- 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 "Can we be even faster?", we will... you know what, just take a look at the code.
<!-- 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 #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
It is actually a quite common technique seen inside the codebase to first reserve the spaces before pushing something into vectors. I don't know why it is not used here.
Before:
After:
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
<!-- 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
A tiny performance fix in `renderer.cpp`.
<!-- 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 #xxx
* [X] 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
The cluster construction code is intensively called during rendering. Even though a single `back()` is fast, but accumulated `back()`s still take a noticiable amount of CPU cycles.
Before:
After:
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Manually validated.
## Summary of the Pull Request
- Adjusts scaling practices in `DxEngine` (and related scaling practices in `TerminalControl`) for pixel-perfect row baselines and spacing at High DPI such that differential row-by-row rendering can be applied at High DPI.
## References
- #5185
## PR Checklist
* [x] Closes#5320, closes#3515, closes#1064
* [x] I work here.
* [x] Manually tested.
* [x] No doc.
* [x] Am core contributor. Also discussed with some of them already via Teams.
## Detailed Description of the Pull Request / Additional comments
**WAS:**
- We were using implicit DPI scaling on the `ID2D1RenderTarget` and running all of our processing in DIPs (Device-Independent Pixels). That's all well and good for getting things bootstrapped quickly, but it leaves the actual scaling of the draw commands up to the discretion of the rendering target.
- When we don't get to explicitly choose exactly how many pixels tall/wide and our X/Y placement perfectly, the nature of floating point multiplication and division required to do the presentation can cause us to drift off slightly out of our control depending on what the final display resolution actually is.
- Differential drawing cannot work unless we can know the exact integer pixels that need to be copied/moved/preserved/replaced between frames to give to the `IDXGISwapChain1::Present1` method. If things spill into fractional pixels or the sizes of rows/columns vary as they are rounded up and down implicitly, then we cannot do the differential rendering.
**NOW:**
- When deciding on a font, the `DxEngine` will take the scale factor into account and adjust the proposed height of the requested font. Then the remainder of the existing code that adjusts the baseline and integer-ifies each character cell will run naturally from there. That code already works correctly to align the height at normal DPI and scale out the font heights and advances to take an exact integer of pixels.
- `TermControl` has to use the scale now, in some places, and stop scaling in other places. This has to do with how the target's nature used to be implicit and is now explicit. For instance, determining where the cursor click hits must be scaled now. And determining the pixel size of the display canvas must no longer be scaled.
- `DxEngine` will no longer attempt to scale the invalid regions per my attempts in #5185 because the cell size is scaled. So it should work the same as at 96 DPI.
- The block is removed from the `DxEngine` that was causing a full invalidate on every frame at High DPI.
- A TODO was removed from `TermControl` that was invalidating everything when the DPI changed because the underlying renderer will already do that.
## Validation Steps Performed
* [x] Check at 150% DPI. Print text, scroll text down and up, do selection.
* [x] Check at 100% DPI. Print text, scroll text down and up, do selection.
* [x] Span two different DPI monitors and drag between them.
* [x] Giant pile of tests in https://github.com/microsoft/terminal/pull/5345#issuecomment-614127648
Co-authored-by: Dustin Howett <duhowett@microsoft.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
The logic here, regarding deleting the spaces and just instantly adding
them bad, is incredibly suspect. Given that we're close to 0.11, I don't
think I can change it.
I've added a TODO with an issue number to figure out the right logic
here.
Fixes#5386.
## PR Checklist
* [x] Closes#5386
* [x] CLA signed
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I'm horrified.
## Validation Steps Performed
Tests, manual validation of the scenario in 5386 and a repro program.
# Summary of the Pull Request
This PR will allow the cursor to be double width when on top of a double width character. This required changing `IsCursorDoubleWidth` to check whether the glyph the cursor's on top of is double width. This code is exactly the same as the original PR that addressed this issue in #2932. That one got reverted at some point due to the crashes related to it, but due to a combination of Terminal having come further since that PR and other changes to address use-after-frees, some of the crashes may/may not be relevant now. The ones that seemed to be relevant/repro-able, I attempt to address in this PR.
The `IsCursorDoubleWidth` check would fail during the `TextBuffer::Reflow` call inside of `Terminal::UserResize` occasionally, particularly when `newCursor.EndDeferDrawing()` is called. This is because when we tell the newCursor to `EndDefer`, the renderer will attempt to redraw the cursor. As part of this redraw, it'll ask if `IsCursorDoubleWidth`, and if the renderer managed to ask this before `UserResize` swapped out the old buffer with the new one from `Reflow`, the renderer will be asking the old buffer if its out-of-bounds cursor is double width. This was pretty easily repro'd using `cmatrix -u0` and resizing the window like a madman.
As a solution, I've moved the Start/End DeferDrawing calls out of `Reflow` and into `UserResize`. This way, I can "clamp" the portion of the code where the newBuffer is getting created and reflowed and swapped into the Terminal buffer, and only allow the renderer to draw once the swap is done. This also means that ConHost's `ResizeWithReflow` needed to change slightly.
In addition, I've added a WriteLock to `SetCursorOn`. It was mentioned as a fix for a crash in #2965 (although I can't repro), and I also figured it would be good to try to emulate where ConHost locks with regards to Cursor operations, and this seemed to be one that we were missing.
## PR Checklist
* [x] Closes#2713
* [x] CLA signed
* [x] Tests added/passed
## Validation Steps Performed
Manual validation that the cursor is indeed chonky, added a test case to check that we are correctly saying that the cursor is double width (not too sure if I put it in the right place). Also open to other test case ideas and thoughts on what else I should be careful for since I am quite nervous about what other crashes might occur.
## Summary of the Pull Request
When WSL vim prints the initial empty buffer (the one that's just a bunch of '\~'s), it prints this by doing the following:
* Print '\~' followed by enough spaces to clear the line
* Use CUP (`^[[H`) to move the cursor to the start of the next line
* repeat until the buffer is full
When we'd get the line of "\~ "... in conhost, we'd mark that line as wrapped.
Logically, it doesn't really make any sense that when we follow that up by moving the cursor, the line is wrapped. However, this is just how conhost is right now.
This wasn't ever a problem in just conhost before, because we really didn't care if lines in the alt buffer were "wrapped" or not. Plus, when vim would get resized, it would just reprint it's own buffer anyways. Nor was this a problem in conpty before this year (2020). We've only just recently added logic to conpty to try and preserve wrapped lines.
Initially, I tried fixing this by breaking the line manually when the cursor was moved. This seemed to work great, except for the win32 vim.exe. Vim.exe doesn't emit a newline or a CUP to get to the next line. It just _goes for it_ and keeps printing. So there's _no way_ for us to know the line broke, because they're essentially just printing one long line, assuming we'll automatically move the cursor.
So instead, I'm making sure to emit the proper number of spaces at the end of a line when the line is wrapped. We won't do any funny business in that scenario and try to optimize for them, we'll _just print the spaces_.
## References
* #5181 - This change regressed this
* #4415 - Actually implemented wrapped lines in conpty
## PR Checklist
* [x] Closes#5291
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Validation Steps Performed
* Wrote a unittest first and foremost
* Checked vtpipeterm to make sure vim still works
* checked Terminal to make sure vim still works
## Summary of the Pull Request
Renderer: Add support for backoff and auto-disable on failed retry
This commit introduces a backoff (150ms * number of tries) to the
renderer's retry logic (introduced in #2830). It also changes the
FAIL_FAST to a less globally-harmful render thread disable, so that we
stop blowing up any application hosting a terminal when the graphics
driver goes away.
In addition, it adds a callback that a Renderer consumer can use to
determine when the renderer _has_ failed, and a public method to kick it
back into life.
Fixes#5340.
This PR also wires up TermControl so that it shows some UI when the renderer tastes clay.
## PR Checklist
* [x] Closes#5340
* [x] cla
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already.
## Validation Steps Performed
I tested this by dropping the number of retries to 1 and forcing a TDR while doing `wsl cmatrix -u0`. It picked up exactly where it left off.
As a bonus, you can actually still type into the terminal when it's graphically suspended (and `exit` still works.). The block is _entirely graphical_.
## Summary of the Pull Request
Adjusts DirectX renderer to use `til::bitmap` to track invalidation
regions. Uses special modification to invalidate a row-at-a-time to
ensure ligatures and NxM glyphs continue to work.
## References
Likely helps #1064
## PR Checklist
* [x] Closes#778
* [x] I work here.
* [x] Manual testing performed. See Performance traces in #778.
* [x] Automated tests for `til` changes.
* [x] Am core contributor. And discussed with @DHowett-MSFT.
## Detailed Description of the Pull Request / Additional comments
- Applies `til::bitmap` as the new invalidation scheme inside the
DirectX renderer and updates all entrypoints for collecting
invalidation data to coalesce into this structure.
- Semi-permanently routes all invalidations through a helper method
`_InvalidateRectangle` that will expand any invalidation to cover the
entire line. This ensures that ligatures and NxM glyphs will continue
to render appropriately while still allowing us to dramatically reduce
the number of lines drawn overall. In the future, we may come up with
a tighter solution than line-by-line invalidation and can modify this
helper method appropriately at that later date to further scope the
invalid region.
- Ensures that the `experimental.retroTerminalEffects` feature continues
to invalidate the entire display on start of frame as the shader is
applied at the end of the frame composition and will stack on itself
in an amusing fashion when we only redraw part of the display.
- Moves many member variables inside the DirectX renderer into the new
`til::size`, `til::point`, and `til::rectangle` methods to facilitate
easier management and mathematical operations. Consequently adds
`try/catch` blocks around many of the already-existing `noexcept`
methods to deal with mathematical or casting failures now detected by
using the support classes.
- Corrects `TerminalCore` redraw triggers to appropriately communicate
scrolling circumstances to the renderer so it can optimize the draw
regions appropriately.
- Fixes an issue in the base `Renderer` that was causing overlapping
scroll regions due to behavior of `Viewport::TrimToViewport` modifying
the local. This fix is "good enough" for now and should go away when
`Viewport` is fully migrated to `til::rectangle`.
- Adds multiplication and division operators to `til::rectangle` and
supporting tests. These operates will help scale back and forth
between a cell-based rectangle and a pixel-based rectangle. They take
special care to ensure that a pixel rectangle being divided downward
back to cells will expand (with the ceiling division methods) to cover
a full cell when even one pixel inside the cell is touched (as is how
a redraw would have to occur).
- Blocks off trace logging of invalid regions if no one is listening to
optimize performance.
- Restores full usage of `IDXGISwapChain1::Present1` to accurately and
fully communicate dirty and scroll regions to the underlying DirectX
framework. This additional information allows the framework to
optimize drawing between frames by eliminating data transfer of
regions that aren't modified and shuffling frames in place. See
[Remarks](https://docs.microsoft.com/en-us/windows/win32/api/dxgi1_2/nf-dxgi1_2-idxgiswapchain1-present1#remarks)
for more details.
- Updates `til::bitmap` set methods to use more optimized versions of
the setters on the `dynamic_bitset<>` that can bulk fill bits as the
existing algorithm was noticeably slow after applying the
"expand-to-row" helper to the DirectX renderer invalidation.
- All `til` import hierarchy is now handled in the parent `til.h` file
and not in the child files to prevent circular imports from happening.
We don't expect the import of any individual library file, only the
base one. So this should be OK for now.
## Validation Steps Performed
- Ran `cmatrix`, `cmatrix -u0`, and `cacafire` after changes were made.
- Made a bunch of ligatures with `Cascadia Code` in the Terminal
before/after the changes and confirmed they still ligate.
- Ran `dir` in Powershell and fixed the scrolling issues
- Clicked all over the place and dragged to make sure selection works.
- Checked retro terminal effect manually with Powershell.
Now that the Terminal is doing a better job of actually marking which
lines were and were not wrapped, we're not always copying lines as
"wrapped" when they should be. We're more correctly marking lines as not
wrapped, when previously we'd leave them marked wrapped.
The real problem is here in the `ScrollFrame` method - we'd manually
newline the cursor to make the terminal's viewport shift down to a new
line. If we had to scroll the viewport for a _wrapped_ line, this would
cause the Terminal to mark that line as broken, because conpty would
emit an extra `\n` that didn't actually exist.
This more correctly implements `ScrollFrame`. Now, well move where we
"thought" the cursor was, so when we get to the next `PaintBufferLine`,
if the cursor needs to newline for the next line, it'll newline, but if
we're in the middle of a wrapped line, we'll just keep printing the
wrapped line.
A couple follow up bugs were found to be caused by the same bad logic.
See #5039 and #5161 for more details on the investigations there.
## References
* #4741 RwR, which probably made this worse
* #5122, which I branched off of
* #1245, #357 - a pair of other conpty wrapped lines bugs
* #5228 - A followup issue for this PR
## PR Checklist
* [x] Closes#5113
* [x] Closes#5180 (by fixing DECRST 25)
* [x] Closes#5039
* [x] Closes#5161 (by ensuring we only `removeSpaces` on the actual
bottom line)
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Validation Steps Performed
* Checked the cases from #1245, #357 to validate that they still work
* Added more and more tests for these scenarios, and then I added MORE
tests
* The entire team played with this in selfhost builds
## Summary of the Pull Request
Reduce the number of times we dispatch a cursor changed event. We were firing it every time the renderer had to do anything related to the cursor. Unfortunately, blinking the cursor triggered this behavior. Now we just check if the position has changed.
## PR Checklist
* [X] Closes#5143
## Validation Steps Performed
Verified using Narrator
Also verified #3791 still works right
Correct scrolling invalidation region for tmux in pty w/ bitmap
Add tracing for circling and scrolling operations. Fix improper
invalidation within AdjustCursorPosition routine in the subsection about
scrolling down at the bottom with a set of margins enabled.
## References
- Introduced with #5024
## Detailed Description of the Pull Request / Additional comments
- This occurs when there is a scroll region restriction applied and a
newline operation is performed to attempt to spin the contents of just
the scroll region. This is a frequent behavior of tmux.
- Right now, the Terminal doesn't support any sort of "scroll content"
operation, so what happens here generally speaking is that the PTY in
the ConHost will repaint everything when this happens.
- The PTY when doing `AdjustCursorPosition` with a scroll region
restriction would do the following things:
1. Slide literally everything in the direction it needed to go to take
advantage of rotating the circular buffer. (This would force a
repaint in PTY as the PTY always forces repaint when the buffer
circles.)
2. Copy the lines that weren't supposed to move back to where they were
supposed to go.
3. Backfill the "revealed" region that encompasses what was supposed to
be the newline.
- The invalidations for the three operations above were:
1. Invalidate the number of rows of the delta at the top of the buffer
(this part was wrong)
2. Invalidate the lines that got copied back into position (probably
unnecessary, but OK)
3. Invalidate the revealed/filled-with-spaces line (this is good).
- When we were using a simple single rectangle for invalidation, the
union of the top row of the buffer from 1 and the bottom row of the
buffer from 2 (and 3 was irrelevant as it was already unioned it)
resulted in repainting the entire buffer and all was good.
- When we switched to a bitmap, it dutifully only repainted the top line
and the bottom two lines as the middle ones weren't a consequence of
intersect.
- The logic was wrong. We shouldn't be invalidating rows-from-the-top
for the amount of the delta. The 1 part should be invalidating
everything BUT the lines that were invalidated in parts 2 and 3.
(Arguably part 2 shouldn't be happening at all, but I'm not optimizing
for that right now.)
- So this solves it by restoring an entire screen repaint for this sort
of slide data operation by giving the correct number of invalidated
lines to the bitmap.
## Validation Steps Performed
- Manual validation with the steps described in #5104
- Automatic test `ConptyRoundtripTests::ScrollWithMargins`.
Closes#5104
Retrieved from https://microsoft.visualstudio.com os OS official/rs_onecore_dep_uxp cba60cafaadfcc7890a45dea3e1a24412c3d0ec6
Related work items: MSFT:25631386
This commit introduces a github action to check our spelling and fixes
the following misspelled words so that we come up green.
It also renames TfEditSes to TfEditSession, because Ses is not a word.
currently, excerpt, fallthrough, identified, occurred, propagate,
provided, rendered, resetting, separate, succeeded, successfully,
terminal, transferred, adheres, breaks, combining, preceded,
architecture, populated, previous, setter, visible, window, within,
appxmanifest, hyphen, control, offset, powerpoint, suppress, parsing,
prioritized, aforementioned, check in, build, filling, indices, layout,
mapping, trying, scroll, terabyte, vetoes, viewport, whose
## Summary of the Pull Request
Moves the ConPTY drawing mechanism (`VtRenderer`) to use the fine-grained `til::bitmap` individual-dirty-bit tracking mechanism instead of coarse-grained rectangle unions to improve drawing performance by dramatically reducing the total area redrawn.
## PR Checklist
* [x] Part of #778 and #1064
* [x] I work here
* [x] Tests added and updated.
* [x] I'm a core contributor
## Detailed Description of the Pull Request / Additional comments
- Converted `GetDirtyArea()` interface from `IRenderEngine` to use a vector of `til::rectangle` instead of the `SMALL_RECT` to banhammer inclusive rectangles.
- `VtEngine` now holds and operates on the `til::bitmap` for invalidation regions. All invalidation operation functions that used to be embedded inside `VtEngine` are deleted in favor of using the ones in `til::bitmap`.
- Updated `VtEngine` tracing to use new `til::bitmap` on trace and the new `to_string()` methods detailed below.
- Comparison operators for `til::bitmap` and complementary tests.
- Fixed an issue where the dirty rectangle shortcut in `til::bitmap` was set to 0,0,0,0 by default which means that `|=` on it with each `set()` operation was stretching the rectangle from 0,0. Now it's a `std::optional` so it has no value after just being cleared and will build from whatever the first invalidated rectangle is. Complementary tests added.
- Optional run caching for `til::bitmap` in the `runs()` method since both VT and DX renderers will likely want to generate the set of runs at the beginning of a frame and refer to them over and over through that frame. Saves the iteration and creation and caches inside `til::bitmap` where the chance of invalidation of the underlying data is known best. It is still possible to iterate manually with `begin()` and `end()` from the outside without caching, if desired. Complementary tests added.
- WEX templates added for `til::bitmap` and used in tests.
- `translate()` method for `til::bitmap` which will slide the dirty points in the direction specified by a `til::point` and optionally back-fill the uncovered area as dirty. Complementary tests added.
- Moves all string generation for `til` types `size`, `point`, `rectangle`, and `some` into a `to_string` method on each object such that it can be used in both ETW tracing scenarios AND in the TAEF templates uniformly. Adds a similar method for `bitmap`.
- Add tagging to `_bitmap_const_iterator` such that it appears as a valid **Input Iterator** to STL collections and can be used in a `std::vector` constructor as a range. Adds and cleans up operators on this iterator to match the theoretical requirements for an **Input Iterator**. Complementary tests added.
- Add loose operators to `til` which will allow some basic math operations (+, -, *, /) between `til::size` and `til::point` and vice versa. Complementary tests added. Complementary tests added.
- Adds operators to `til::rectangle` to allow scaling with basic math operations (+, -, *) versus `til::size` and translation with basic math operations (+, -) against `til::point`. Complementary tests added.
- In-place variants of some operations added to assorted `til` objects. Complementary tests added.
- Update VT tests to compare invalidation against the new map structure instead of raw rectangles where possible.
## Validation Steps Performed
- Wrote additional til Unit Tests for all additional operators and functions added to the project to support this operation
- Updated the existing VT renderer tests
- Ran perf check
When we painted spaces up until the character right before the right
edge of the screen, we would erroneously use Erase in Line instead of
Erase Character due to an off-by-one.
Fixes#4727
## Summary of the Pull Request
This notifies automation clients (i.e.: NVDA, narrator, etc...) of new output being rendered to the screen.
## References
Close#2447 - Signaling for new output and cursor
Close#3791 - fixed by signaling cursor changes
## Detailed Description of the Pull Request / Additional comments
- Added tracing for UiaRenderer. This makes it easier to debug issues with notifying an automation client.
- Fire TextChanged automation events when new content is output to the screen.
## Validation Steps Performed
Verified with NVDA [1]
## Narrator
Narrator works _better_, but is unable to detect new output consistently. There is no harm for narrator when this change goes in.
[1] https://github.com/microsoft/terminal/issues/2447#issuecomment-595879890
Retrieved from https://microsoft.visualstudio.com os OS official/rs_onecore_dep_uxp 5b3acd8b5bac38da02fc86a29c81dfd252e79d1f
Related work items: MSFT:25505535
<!-- 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
Always use the system's locale to render text to ensure the correct font variants are used.
`_ResolveFontFaceWithFallback()` overrides the last argument with the locale name of the font, but users normally configure fonts with latin alphabet only and use font linking to display non-latin characters, which causes the the locale names of the latin fonts are used to render the non-latin fonts. https://github.com/microsoft/terminal/issues/4508#issuecomment-598552472
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#4508
* [x] 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
## Validation
On a zh-hans system, simplified Chinese hans are used after this patch (above), versus Japanese hans before (below).
## Summary of the Pull Request
This changes the renederer to make sure to not draw the cursor when it is placed outside the viewport. When the window height isn't an exact multiple of a row's height, then there's a little bit of space below the last line we're actually drawing. If cursor is on the line below the viewport, then it can actually get drawn into this space. Since we're not drawing the text for that line, this is a little odd.
This PR fixes the issue by simply ensuring the cursor is in the veiwport before we draw it.
## References
## PR Checklist
* [x] Closes#3166
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
## Validation Steps Performed
Checked with the GDI renderer as well as the DX renderer in conhost to make sure this is fixed for both of them, as well as the Terminal
cmatrix is somewhat of a pathological case for our infrastructure: it
prints out a bunch of green and white characters and then updates them a
million times a second.
It also maintains a column of space between every green character. When
it prints this column, it prints it in "default" or "white". This ends
up making runs of text that look like this:
(def: G=green B=bright white W=white *=matrix char =space)
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
As characters trickle in:
G*W G*W G*W G*W G*W G*W G*W B*W
G*W G*W G*W G*W G*W G*W G*W G W
G*W G*W G*W B*W G*W G*W G*W G W
G*W B*W G*W G W G*W G*W G*W G*W
G*W G W G*W G W G*W B*W G*W G*W
B*W G W G*W G W G*W G W B*W G*W
G W G W G*W G W G*W G W G W B*W
G W G W B*W G W G*W G W G W G W
Every one of those color transitions causes us to break up the run of
text and start rendering it again. This impacts GDI, Direct2D *and*
ConPTY. In the example above, there are 120 runs.
The problem is, printing a space doesn't **use** the foreground color!
This commit introduces an optimization. When we're about to break a text
cluster because its attributes changed, we make sure that it's not just
filled with spaces and doesn't differ in any visually-meaningful way
(like underline or strikethrough, considering global invert state).
This lets us optimize both the rendering _and_ the PTY output to look
like this:
G* * * * * * * B*G
G* * * * * * *
G* * * B*G * * *
G* B*G * * * * *
G* * * B*G * *
B*G * * B*G *
G * * B*G
G B*G *
Text will be printed at best line-by-line and at worst only when the
visible properties of the screen actually change. In the example
above, there are only 21 runs.
This speeds up cmatrix remarkably.
Refs #1064
This PR adds support for "Resize with Reflow" to the Terminal. In
conhost, `ResizeWithReflow` is the function that's responsible for
reflowing wrapped lines of text as the buffer gets resized. Now that
#4415 has merged, we can also implement this in the Terminal. Now, when
the Terminal is resized, it will reflow the lines of it's buffer in the
same way that conhost does. This means, the terminal will no longer chop
off the ends of lines as the buffer is too small to represent them.
As a happy side effect of this PR, it also fixed#3490. This was a bug
that plagued me during the investigation into this functionality. The
original #3490 PR, #4354, tried to fix this bug with some heavy conpty
changes. Turns out, that only made things worse, and far more
complicated. When I really got to thinking about it, I realized "conhost
can handle this right, why can't the Terminal?". Turns out, by adding
resize with reflow, I was also able to fix this at the same time.
Conhost does a little bit of math after reflowing to attempt to keep the
viewport in the same relative place after a reflow. By re-using that
logic in the Terminal, I was able to fix#3490.
I also included that big ole test from #3490, because everyone likes
adding 60 test cases in a PR.
## References
* #4200 - this scenario
* #405/#4415 - conpty emits wrapped lines, which was needed for this PR
* #4403 - delayed EOL wrapping via conpty, which was also needed for
this
* #4354 - we don't speak of this PR anymore
## PR Checklist
* [x] Closes#1465
* [x] Closes#3490
* [x] Closes#4771
* [x] Tests added/passed
## EDIT: Changes to this PR on 5 March 2020
I learned more since my original version of this PR. I wrote that in
January, and despite my notes that say it was totally working, it
_really_ wasn't.
Part of the hard problem, as mentioned in #3490, is that the Terminal
might request a resize to (W, H-1), and while conpty is preparing that
frame, or before the terminal has received that frame, the Terminal
resizes to (W, H-2). Now, there aren't enough lines in the terminal
buffer to catch all the lines that conpty is about to emit. When that
happens, lines get duplicated in the buffer. From a UX perspective, this
certainly looks a lot worse than a couple lost lines. It looks like
utter chaos.
So I've introduced a new mode to conpty to try and counteract this
behavior. This behavior I'm calling "quirky resize". The **TL;DR** of
quirky resize mode is that conpty won't emit the entire buffer on a
resize, and will trust that the terminal is prepared to reflow it's
buffer on it's own.
This will enable the quirky resize behavior for applications that are
prepared for it. The "quirky resize" is "don't `InvalidateAll` when the
terminal resizes". This is added as a quirk as to not regress other
terminal applications that aren't prepared for this behavior
(gnome-terminal, conhost in particular). For those kinds of terminals,
when the buffer is resized, it's just going to lose lines. That's what
currently happens for them.
When the quirk is enabled, conpty won't repaint the entire buffer. This
gets around the "duplicated lines" issue that requesting multiple
resizes in a row can cause. However, for these terminals that are
unprepared, the conpty cursor might end up in the wrong position after a
quirky resize.
The case in point is maximizing the terminal. For maximizing
(height->50) from a buffer that's 30 lines tall, with the cursor on
y=30, this is what happens:
* With the quirk disabled, conpty reprints the entire buffer. This is
60 lines that get printed. This ends up blowing away about 20 lines
of scrollback history, as the terminal app would have tried to keep
the text pinned to the bottom of the window. The term. app moved the
viewport up 20 lines, and then the 50 lines of conpty output (30
lines of text, and 20 blank lines at the bottom) overwrote the lines
from the scrollback. This is bad, but not immediately obvious, and
is **what currently happens**.
* With the quirk enabled, conpty doesn't emit any lines, but the
actual content of the window is still only in the top 30 lines.
However, the terminal app has still moved 20 lines down from the
scrollback back into the viewport. So the terminal's cursor is at
y=50 now, but conpty's is at 30. This means that the terminal and
conpty are out of sync, and there's not a good way of re-syncing
these. It's very possible (trivial in `powershell`) that the new
output will jump up to y=30 override the existing output in the
terminal buffer.
The Windows Terminal is already prepared for this quirky behavior, so it
doesn't keep the output at the bottom of the window. It shifts it's
viewport down to match what conpty things the buffer looks like.
What happens when we have passthrough mode and WT is like "I would like
quirky resize"? I guess things will just work fine, cause there won't be
a buffer behind the passthrough app that the terminal cares about. Sure,
in the passthrough case the Terminal could _not_ quirky resize, but the
quirky resize won't be wrong.
When ConPTY encounters a string we don't understand, immediately flush the frame.
## References
This PR supersedes #2665. This solution is much simpler than what was proposed in that PR.
As mentioned in #2665: "This might have some long-term consequences for #1173."
## PR Checklist
* [x] Closes#2011
* [x] Closes#4106
* [x] I work here
* [x] Tests added/passed
## Summary of the Pull Request
Allows VT engine methods that print formatted strings (cursor movements, color changes, etc.) to provide a guess at the max buffer size required eliminating the double-call for formatting in the common case.
## PR Checklist
* [x] Found while working on #778
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
- The most common case for VT rendering is formatting a few numbers into a sequence. For the most part, we can already tell the maximum length that the string could be based on the number of substitutions and the size of the parameters.
- The existing formatting method would always double-call. It would first call for how long the string was going to be post-formatting, allocate that memory, then call again and fill it up. This cost two full times of running through the string to find a length we probably already knew for the most part.
- Now if a size is provided, we allocate that first and attempt the "second pass" of formatting directly into the buffer. This saves the count step in the common case.
- If this fails, we fall back and do the two-pass method (which theoretically means the bad case is now 3 passes.)
- The next biggest waste of time in this method was allocating and freeing strings for every format pass. Due to the nature of the VT renderer, many things need to be formatted this way. I've now instead moved the format method to hold a static string that really only grows over the course of the session for all of these format operations. I expect a majority of the time, it will only be consuming approximately 5-15 length of a std::string of memory space. I cannot currently see a circumstance where it would use more than that, but I'm consciously trading memory usage when running as a PTY for overall runtime performance here.
## Validation Steps Performed
- Ran the thing manually and checked it out with wsl and cmatrix and Powershell and such attached to Terminal
- Wrote and ran automated tests on formatting method
## Summary of the Pull Request
- Changes the `IRenderEngine` interface to return a vector of values instead of just a single one. Engines that want to report one still can. Engines that want to report multiple smaller ones will be able to do so going forward.
## PR Checklist
* [x] In support of differential rendering #778
* [x] I work here.
* [x] Manually tested it still works.
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
- Some of my ideas for the `DxEngine` require the ability to specify multiple smaller rectangles instead of one giant one, specifically to mitigate the case where someone refreshes just one cell in two opposite corners of the display (which currently coalesces into refreshing the entire display.)
- This is pulled out into an individual PR to make it easier to review that concept changing.
## Validation Steps Performed
- Ran the Terminal
With certain font faces at certain sizes, the advances seem to be
slightly more than the pixel grid; Cascadia Code at 13pt (though, 200%
scale) had an advance of 10.000001.
This commit makes it so that anything sub-1/100 of a cell won't make us
break up runs, because doing so results in suboptimal rendering.
Fixes#4806.
## Summary of the Pull Request
Adjusts column padding code in `CustomTextLayout` to only pad out for surrogate pairs, not anything that reports two columns.
## References
- See also #4747
## PR Checklist
* [x] Closes#4780
* [x] I work here.
* [x] Manual tests.
* [x] No doc, this fixes code to match comment. Oops.
* [x] Am core contributor. Also discussed with @leonMSFT.
## Detailed Description of the Pull Request / Additional comments
For surrogate pairs like high Unicode emoji, we receive two wchar_ts but only one column count (which is usually 2 because emoji are usually inscribed in the full-width squares.) To compensate for this, I added in a little padding function at the top of the `CustomTextLayout` construction that adds a column of 0 aligned with the second half of a surrogate pair so the text-to-glyph mapping lines up correctly.
Unfortunately, I made a mistake while either responding to PR feedback in #4747 or in the first place and I made it pad out extra 0 columns based on the FIRST column count, not based on whether or not there is a trailing surrogate pair. The correct thing to do is to pad it out based on the LENGTH of text associated with the given column count. This means that full width characters which can be represented in one wchar_t, like those coming from the IME in most cases (U+5C41 for example) will have a column count of 2. This is perfectly correct for mapping text-to-glyphs and doesn't need a 0 added after it. A house emoji (U+1F3E0) comes in as two wchar_ts (0xD83C 0xDFE0) with the column count of 2. To ensure that the arrays are aligned, the 2 matches up with the 0xD83C but the 0xDFE0 needs a 0 on it so it will be skipped over. (Don't worry, because it's a surrogate, it's naturally consumed correctly by the glyph mapper.)
The effect was that every OTHER character inserted by the IME was scaled to 0 size (as an advance of 0 was expected for 0 columns).
The fix restores it so those characters don't have an associated count and aren't scaled.
## Validation Steps Performed
- Opened it up
- Put in the house emoji like #4747 (U+1f3e0)
- Put in some characters with simplified Chinese IME (fixed now)
- Put in the utf83.txt sample text used in #4747
## Summary of the Pull Request
- Improves the correction of the scaling and spacing that is applied to
glyphs if they are too large or too small for the number of columns that
the text buffer is expecting
## References
- Supersedes #4438
Co-authored-by: Mili (Yi) Zhang <milizhang@gmail.com>
- Related to #4704 (#4731)
## PR Checklist
* [x] Closes#696
* [x] Closes#4375
* [x] Closes#4708
* [x] Closes a crash that @DHowett-MSFT complained about with
`"x" * ($Host.UI.RawUI.BufferSize.Width - 1) + "`u{241b}"`
* [x] Eliminates an exception getting thrown with the U+1F3E0 emoji in
`_CorrectGlyphRun`
* [x] Corrects several graphical issues that occurred after #4731 was
merged to master (weird repeats and splits of runs)
* [x] I work here.
* [x] Tested manually versus given scenarios.
* [x] Documentation written into comments in the code.
* [x] I'm a core contributor.
## Detailed Description of the Pull Request / Additional comments
- The `_CorrectGlyphRun` function now walks through and uses the
`_glyphClusters` map to determine the text span and glyph span for each
cluster so it can be considered as a single unit for scaling.
- The total number of columns expected across the entire cluster
text/glyph unit is considered for the available spacing for drawing
- The total glyph advances are summed to see how much space they will
take
- If more space than necessary to draw, all glyphs in the cluster are
offset into the center and the extra space is padded onto the advance of
the last glyph in the range.
- If less space than necessary to draw, the entire cluster is marked for
shrinking as a single unit by providing the initial text index and
length (that is back-mapped out of the glyph run) up to the parent
function so it can use the `_SetCurrentRun` and `_SplitCurrentRun`
existing functions (which operate on text) to split the run into pieces
and only scale the one glyph cluster, not things next to it as well.
- The scale factor chosen for shrinking is now based on the proportion
of the advances instead of going through some font math wizardry
- The parent that calls the run splitting functions now checks to not
attempt to split off text after the cluster if it's already at the end.
This was @DHowett-MSFT's crash.
- The split run function has been corrected to fix the `glyphStart`
position of the back half (it failed to `+=` instead of `=` which
resulted in duplicated text, sometimes).
- Surrogate pair emoji were not allocating an appropriate number of
`_textClusterColumns`. The constructor has been updated such that the
trailing half of surrogate pairs gets a 0 column width (as the lead is
marked appropriately by the `GetColumns()` function). This was the
exception thrown.
- The `_glyphScaleCorrections` array stored up over the calls to
`_CorrectGlyphRun` now uses a struct `ScaleCorrection` as we're up to 3
values.
- The `ScaleCorrection` values are named to clearly indicate they're in
relation to the original text span, not the glyph spans.
- The values that are used to construct `ScaleCorrection`s within
`_CorrectGlyphRun` have been double checked and corrected to not
accidentally use glyph index/counts when text index/counts are what's
required.
## Validation Steps Performed
- Tested the utf82.txt file from one of the linked bugs. Looked
specifically at Burmese through Thai to ensure restoration (for the most
part) of the behavior
- Ensured that U+1f3e0 emoji (🏠) continues to draw correctly
- Checked Fixedsys Excelsior font to ensure it's not shrinking the line
with its ligatures
- Checked ligatureness of Cascadia Code font
- Checked combining characters U+0300-U+0304 with a capital A
## Summary of the Pull Request
Fix a bug where the `Renderer::PaintFrame` method:
1. is not called until the next `RenderThread::NotifyThread` call but needs to be called because there the terminal was updated (theoretical bug)
2. is called twice but needs to be called only once (verified bug)
## References
The bug was introduced by #3511.
## PR Checklist
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
## Detailed Description of the Pull Request / Additional comments
### Before
#### First bug
In the original code, `_fNextFrameRequested` is set to `true` in render thread because `std::atomic_flag::test_and_set` is called.
This is wrong because it means that the render thread will render the terminal again even if there is no change after the last render.
I think the the goal was to load the boolean value for `_fNextFrameRequested` to check whether the thread should sleep or not.
The problem is that there is no method on `std::atomic_flag` to load its boolean value. I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value. I guess that this was believed to be equivalent to just a simple load, without doing any change to the value because it restores it at the end.
But it's not: this is dangerous because if the value is changed to `true` between the call to `std::atomic_flag::test_and_set` and the call to `std::atomic_flag::clear`, then the value ends up being `false` at the end which is wrong because we don't want to change it! And if that value ends up being `false`, it means that we miss a render because we will wait on `_hEvent` during the next iteration on the render thread.
Well actually, here, this not even a problem because when that code is ran, `_fPainting` is `false` which means that the other thread that modifies the `_fNextFrameRequested` value through `RenderThread::NotifyPaint` will not actually modify `_fNextFrameRequested` but rather call `SetEvent` (see the method's body).
But wait! There is a problem there too! `std::atomic_flag::test_and_set` is called for `_fPainting` which sets its value to `true`. It was probably unintended. So actually, the next call to `RenderThread::NotifyPaint` _will_ end up modifying `_fNextFrameRequested` which means that the data race I was talking about _might_ happen!
#### Second bug
Let's go back a little bit in my explanation. I was talking about the fact that:
> I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value.
The problem is that the reverse was done in the implementation: `std::atomic_flag::clear` is called if the value was _`true`_ originally!
So at this point, if the value of `_fNextFrameRequested` was `false`, then `std::atomic_flag::test_and_set` sets its is set to `true` and returns `false`. So for the next iteration, `_fNextFrameRequested` is `true` and the render thread will re-render but that was not needed.
### After
I used `std::atomic<bool>` instead of `std::atomic_flag` for `_fNextFrameRequested` and the other atomic field because it has a `load` and a `store` method so we can actually load the value without changing it.
I also replaced `_fPainting` by `_fWaiting`, which is basically the opposite of `_fPainting` but stays `true` for a little shorter than `_fPainting` would stay `false`. Indeed, I think that it makes more sense to directly wrap/scope _just_ the call to `WaitForSingleObject` by setting my atomic variable to `true` _just_ before and to `false` _just_ after because:
* It makes more sense while you're reading the code: it's easier IMO to understand what the purpose of `_fWaiting` is (that is, to call `SetEvent` from `RenderThread::NotifyPaint` if it's `true`).
* It's probably a tiny bit better for performance because it will become `true` for a little shorter which means less calls to `SetEvent`.
#### Warning
I don't really understand [std::memory_order](https://en.cppreference.com/w/cpp/atomic/memory_order)s.
So I used the default one (`std::memory_order_seq_cst`) which is the safest.
I believe that if no read or write are reordered in the two threads (`RenderThread::NotifyPaint` and `RenderThread::_ThreadProc`), then the code I wrote will behave correctly.
I think that `std::memory_order_seq_cst` enforces that so it should be fine, but I'm not sure.
## Validation Steps Performed
**I tried to reproduce the second bug that I described in the first section of this PR.**
I put a breakpoint on `RenderThread::NotifyPaint` and on `Renderer::PaintFrame`. Initially they are disabled. Then I ran the terminal in Release mode, waited a bit for the prompt to display and the cursor to start blinking. Then I enabled the breakpoints.
### Before
Each `RenderThread::NotifyPaint` is followed by 2 `Renderer::PaintFrame` calls. ❌
### After
Each `RenderThread::NotifyPaint` is followed by 1 `Renderer::PaintFrame` call. ✔️
## Summary of the Pull Request
Changes how conpty emits text to preserve line-wrap state, and additionally adds rudimentary support to the Windows Terminal for wrapped lines.
## References
* Does _not_ fix (!) #3088, but that might be lower down in conhost. This makes wt behave like conhost, so at least there's that
* Still needs a proper deferred EOL wrap implementation in #780, which is left as a todo
* #4200 is the mega bucket with all this work
* MSFT:16485846 was the first attempt at this task, which caused the regression MSFT:18123777 so we backed it out.
* #4403 - I made sure this worked with that PR before I even sent #4403
## PR Checklist
* [x] Closes#405
* [x] Closes#3367
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
I started with the following implementation:
When conpty is about to write the last column, note that we wrapped this line here. If the next character the vt renderer is told to paint get is supposed to be at the start of the following line, then we know that the previous line had wrapped, so we _won't_ emit the usual `\r\n` here, and we'll just continue emitting text.
However, this isn't _exactly_ right - if someone fills the row _exactly_ with text, the information that's available to the vt renderer isn't enough to know for sure if this line broke or not. It is possible for the client to write a full line of text, with a `\n` at the end, to manually break the line. So, I had to also add the `lineWrapped` param to the `IRenderEngine` interface, which is about half the files in this changelist.
## Validation Steps Performed
* Ran tests
* Checked how the Windows Terminal behaves with these changes
* Made sure that conhost/inception and gnome-terminal both act as you'd expect with wrapped lines from conpty
## Summary of the Pull Request
- Surrogate pairs are being split in half with the run splitting check.
## References
- Related to #4708 but not going to fix it.
## PR Checklist
* [x] Closes#4704
* [x] I work here.
* [x] I am a core contributor.
## Detailed Description of the Pull Request / Additional comments
- The adjustment of the run heights in the correction function reports back a text index and a scaling factor. However, I didn't remember at the time that the text is being stored as UTF-16. So the index given can be pointing to the high surrogate of a pair. Thus adding 1 to split "after" the text character, then backing up by 1 isn't valid in if the index given was for a high surrogate.
The quick fix is to advance by two if it's a high surrogate and one otherwise.
## Validation Steps Performed
- Used the sample code from #4704 to print the house emoji in various situations into the buffer.
<!-- 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
- Scale the retro terminal effects (#3468) scan lines with the screen's DPI.
- Remove artifacts from sampling wrap around.
Before & after, with my display scale set to 350%:
Before & after showing artifact removal, with my display scale set to 100%, and image enlarged to 400%:
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#4362
* [x] 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
Adds a constant buffer, which could be used for other settings for the retro terminal pixel shader.
I haven't touched C++ in over a decade before this change, and this is the first time I've played with DirectX, so please assume my code isn't exactly best practice. 🙂
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
- Changed display scale with experimental.retroTerminalEffect enabled, enjoyed scan lines on high resolution monitors.
- Enabled experimental.retroTerminalEffect, turned the setting off, changed display scale. Retro tabs still scale scan lines.
