De-duplicate code for TerminalDispatch and AdaptDispatch #3849

zadjii-msft · 2019-12-05T16:05:42Z

Both these classes are doing a lot of the nearly same work at this point. We should probably not, and have more common implementations somehow.

IIRC, when I was writing it, I think part of the idea was to move ITerminalApi up and out of the Terminal to be shared, then have one Dispatch that calls into the API, then an adapter between that API and the conhost API

This is a super crude diagram

         +------------------+
         |  OutputEngine    |
         +---------+--------+
                   |
         +---------v--------+
         | OutputDispatch   |
         +--------+---------+
                  |
         +--------v---------+
         |   ITerminalApi   |
         +---+------------+-+
             |            |
             |            |
+------------v--+     +---v------------+
|  Terminal     |     |  HostAdapter   |
+---------------+     +-------+--------+
                              |
                      +-------v--------+
                      |   outputStream |
                      |      +         |
                      |      |         |
                      |      v         |
                      |   getset       |
                      +----------------+

In this model, Terminal and HostAdapter both implement ITerminalApi, so there only needs to be one Dispatcher. HostAdapter then calls into the conGetSet API we already have.

This would pretty horribly break tests that test the existing structure :/

I could be horribly off base with this though. I came up with this idea like 9 months ago and never wrote it down.

Should the bold/bright attribute apply to SGR 38/48 indexed colors? #5384 (xterm indexed color problem)

The text was updated successfully, but these errors were encountered:

miniksa · 2019-12-05T17:22:39Z

This sounds reasonable to me. Doesn't matter if it breaks the test. We've already identified that half of the adapter tests are pretty silly anyway. Probably need to revisit that anyway.

miniksa · 2019-12-17T23:28:08Z

It looks like the TerminalDispatch methods don't really have tests on them either. So it's probably up to this task to consider testing overall.

j4james · 2019-12-18T00:06:57Z

My initial thought for this was that we would just drop the TerminalDispatch code and replace it entirely with AdaptDispatch. That already has all of the functionality that TerminalDispatch is trying to replicate, and it actually does have tests in place too.

Then the interface that splits the conhost and Terminal implementations could possibly be based on the current ConGetSet interface. It'll require quite a lot of cleanup before it's really suitable for that task, but I think that's probably the right place for it.

## Summary of the Pull Request This is essentially a rewrite of the VT tab stop functionality, implemented entirely within the `AdaptDispatch` class. This significantly simplifies the `ConGetSet` interface, and should hopefully make it easier to share the functionality with the Windows Terminal VT implementation in the future. By removing the dependence on the `SCREEN_INFORMATION` class, it fixes the problem of the the tab state not being preserved when switching between the main and alternate buffers. And the new architecture also fixes problems with the tabs not being correctly initialized when the screen is resized. ## References This fixes one aspect of issue #3545. It also supersedes the fix for #411 (PR #2816). I'm hoping the simplification of `ConGetSet` will help with #3849. ## PR Checklist * [x] Closes #4669 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments In the new tab architecture, there is now a `vector<bool>` (__tabStopColumns_), which tracks whether any particular column is a tab stop or not. There is also a __initDefaultTabStops_ flag indicating whether the default tab stop positions need to be initialised when the screen is resized. The way this works, the vector is initially empty, and only initialized (to the current width of the screen) when it needs to be used. When the vector grows in size, the __initDefaultTabStops_ flag determines whether the new columns are set to false, or if every 8th column is set to true. By default we want the latter behaviour - newly revealed columns should have default tab stops assigned to them - so __initDefaultTabStops_ is set to true. However, after a `TBC 3` operation (i.e. we've cleared all tab stops), there should be no tab stops in any newly revealed columns, so __initDefaultTabStops_ is set to false. Note that the __tabStopColumns_ vector is never made smaller when the window is shrunk, and that way it can preserve the state of tab stops that are off screen, but which may come into range if the window is made bigger again. However, we can can still reset the vector completely after an `RIS` or `TBC 3` operation, since the state can then be reconstructed automatically based on just the __initDefaultTabStops_ flag. ## Validation Steps Performed The original screen buffer tests had to be rewritten to set and query the tab stop state using escape sequences rather than interacting with the `SCREEN_INFORMATION` class directly, but otherwise the structure of most tests remained largely the same. However, the alt buffer test was significantly rewritten, since the original behaviour was incorrect, and the initialization test was dropped completely, since it was no longer applicable. The adapter tests were also dropped, since they were testing the `ConGetSet` interface which has now been removed. I also had to make an addition to the method setup of the screen buffer tests (making sure the viewport was appropriately initialized), since there were some tests (unrelated to tab stops) that were previously dependent on the state being set in the tab initialization test which has now been removed. I've manually tested the issue described in #4669 and confirmed that the tabs now produce the correct spacing after a resize.

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

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

This is essentially a rewrite of the `TerminalDispatch::SetGraphicsRendition` method, bringing it into closer alignment with the `AdaptDispatch` implementation, simplifying the `ITerminalApi` interface, and making the code easier to extend. It adds support for a number of attributes which weren't previously implemented. REFERENCES * This is a mirror of the `AdaptDispatch` refactoring in PR #5758. * The closer alignment with `AdaptDispatch` is a small step towards solving issue #3849. * The newly supported attributes should help a little with issues #5461 (italics) and #6205 (strike-through). DETAILS I've literally copied and pasted the `SetGraphicsRendition` implementation from `AdaptDispatch` into `TerminalDispatch`, with only few minor changes: * The `SetTextAttribute` and `GetTextAttribute` calls are slightly different in the `TerminalDispatch` version, since they don't return a pointless `success` value, and in the case of the getter, the `TextAttribute` is returned directly instead of by reference. Ultimately I'd like to move the `AdaptDispatch` code towards that way of doing things too, but I'd like to deal with that later as part of a wider refactoring of the `ConGetSet` interface. * The `SetIndexedForeground256` and `SetIndexedBackground256` calls required the color indices to be remapped in the `AdaptDispatch` implementation, because the conhost color table is in a different order to the XTerm standard. `TerminalDispatch` doesn't have that problem, so doesn't require the mapping. * The index color constants used in the 16-color `SetIndexedForeground` and `SetIndexedBackground` calls are also slightly different for the same reason. VALIDATION I cherry-picked this code on top of the #6506 and #6698 PRs, since that's only way to really get the different color formats passed-through to the terminal. I then ran a bunch of manual tests with various color coverage scripts that I have, and confirmed that all the different color formats were being rendered as expected. Closes #6725

j4james · 2021-04-11T18:02:17Z

As I've mentioned elsewhere, I've been investigating ways to simplify the ConGetSet interface, with the idea that it could eventually be merged with the ITerminalApi interface, and AdaptDispatch could then more easily be merged with TerminalDispatch. I'm going to go one step at a time on this, so I'm not going to dump a massive PR on you, but the list below covers some of the ideas I'm considering. If there's anything here you definitely don't want me to do, please let me know.

Replace GetConsoleScreenBufferInfoEx with a set of methods providing the specific properties we actually need, namely the buffer size, the virtual viewport dimensions, and the cursor position. This should also let us get rid of all the MoveToBottom calls which I think are only there to make sure the viewport returned by GetConsoleScreenBufferInfoEx is aligned with the virtual viewport.
Get rid of the DeleteLines, InsertLines, and PrivateReverseLineFeed methods. These operations can easily be implemented in AdaptDispatch using existing scrolling and filling APIs, and then Windows Terminal can eventually inherit that functionality without having to implement any additional interfaces.
Of the methods we do want to keep, we need to rename them to better match the ITerminalApi interface, for example getting rid of all of those Private prefixes. And if we're going to be rewriting a lot of the interface, this seems like a good time to try and modernize things a bit, e.g. replacing COORD with til::point, and RECT with til::rectangle (assuming that's the current preferred standard). I'm also very keen to get rid of the boolean return values as error indicators - we should be using return values for returning things, and throwing exceptions for errors.
Again while we're rewriting things, this might be a good time to consider simplifying some of the bloat in the conhost implementation. Like is it really necessary for every ConhostInternalGetSet method to call a separate DoSrvPrivateXXXX function to actually do the work. Why not just implement the code right there in the ConhostInternalGetSet class?
In the interests of future expansion, I'd like to try and coalesce some of the APIs where that seems reasonable. For example, do we really need 5 different mouse mode methods (which will eventually require another 5 matching getter methods), when we could just have one method with a mode parameter? The same thing applies to the various keyboard modes - I think there are only three at the moment, but that list is going to grow. It would make future expansion a lot easier if that just meant adding one more enum value.

miniksa · 2021-07-09T20:56:07Z

I love all 5 of these things, @j4james. Feel free to take them on in whatever staging you deem sensible so we can have them reviewed.

Instead of having a separate method for setting each mouse and keyboard mode, this PR consolidates them all into a single method which takes a mode parameter, and stores the modes in a `til::enumset` rather than having a separate `bool` for each mode. This enables us to get rid of a lot of boilerplate code, and makes the code easier to extend when we want to introduce additional modes in the future. It'll also makes it easier to read back the state of the various modes when implementing the `DECRQM` query. Most of the complication is in the `TerminalInput` class, which had to be adjusted to work with an `enumset` in place of all the `bool` fields. For the rest, it was largely a matter of replacing calls to all the old mode setting methods with the new `SetInputMode` method, and deleting a bunch of unused code. One thing worth mentioning is that the `AdaptDispatch` implementation used to have a `_ShouldPassThroughInputModeChange` method that was called after every mode change. This code has now been moved up into the `SetInputMode` implementation in `ConhostInternalGetSet` so it's just handled in one place. Keeping this out of the dispatch class will also be beneficial for sharing the implementation with `TerminalDispatch`. ## Validation The updated interface necessitated some adjustments to the tests in `AdapterTest` and `MouseInputTest`, but the essential structure of the tests remains unchanged, and everything still passes. I've also tested the keyboard and mouse modes in Vttest and confirmed they still work at least as well as they did before (both conhost and Windows Terminal), and I tested the alternate scroll mode manually (conhost only). Simplifying the `ConGetSet` and `ITerminalApi` is also part of the plan to de-duplicate the `AdaptDispatch` and `TerminalDispatch` implementation (#3849).

## Summary of the Pull Request In the original implementation, we used two different orderings for the color tables. The WT color table used ANSI order, while the conhost color table used a Windows-specific order. This PR standardizes on the ANSI color order everywhere, so the usage of indexed colors is consistent across both parts of the code base, which will hopefully allow more of the code to be shared one day. ## References This is another small step towards de-duplicating `AdaptDispatch` and `TerminalDispatch` for issue #3849, and is essentially a followup to the SGR dispatch refactoring in PR #6728. ## PR Checklist * [x] Closes #11461 * [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: #11461 ## Detailed Description of the Pull Request / Additional comments Conhost still needs to deal with legacy attributes using Windows color order, so those values now need to be transposed to ANSI colors order when creating a `TextAttribute` object. This is done with a simple mapping table, which also handles the translation of the default color entries, so it's actually slightly faster than the original code. And when converting `TextAttribute` values back to legacy console attributes, we were already using a mapping table to handle the narrowing of 256-color values down to 16 colors, so we just needed to adjust that table to account for the translation from ANSI to Windows, and then could make use of the same table for both 256-color and 16-color values. There are also a few places in conhost that read from or write to the color tables, and those now need to transpose the index values. I've addressed this by creating separate `SetLegacyColorTableEntry` and `GetLegacyColorTableEntry` methods in the `Settings` class which take care of the mapping, so it's now clearer in which cases the code is dealing with legacy values, and which are ANSI values. These methods are used in the `SetConsoleScreenBufferInfoEx` and `GetConsoleScreenBufferInfoEx` APIs, as well as a few place where color preferences are handled (the registry, shortcut links, and the properties dialog), none of which are particularly sensitive to performance. However, we also use the legacy table when looking up the default colors for rendering (which happens a lot), so I've refactored that code so the default color calculations now only occur once per frame. The plus side of all of this is that the VT code doesn't need to do the index translation anymore, so we can finally get rid of all the calls to `XTermToWindowsIndex`, and we no longer need a separate color table initialization method for conhost, so I was able to merge a number of color initialization methods into one. We also no longer need to translate from legacy values to ANSI when generating VT sequences for conpty. The one exception to that is the 16-color VT renderer, which uses the `TextColor::GetLegacyIndex` method to approximate 16-color equivalents for RGB and 256-color values. Since that method returns a legacy index, it still needs to be translated to ANSI before it can be used in a VT sequence. But this should be no worse than it was before. One more special case is conhost's secret _Color Selection_ feature. That uses `Ctrl`+Number and `Alt`+Number key sequences to highlight parts of the buffer, and the mapping from number to color is based on the Windows color order. So that mapping now needs to be transposed, but that's also not performance sensitive. The only thing that I haven't bothered to update is the trace logging code in the `Telemetry` class, which logs the first 16 entries in the color table. Those entries are now going to be in a different order, but I didn't think that would be of great concern to anyone. ## Validation Steps Performed A lot of unit tests needed to be updated to use ANSI color constants when setting indexed colors, where before they might have been expecting values in Windows order. But this replaced a wild mix of different constants, sometimes having to use bit shifting, as well as values mapped with `XTermToWindowsIndex`, so I think the tests are a whole lot clearer now. Only a few cases have been left with literal numbers where that seemed more appropriate. In addition to getting the unit tests working, I've also manually tested the behaviour of all the console APIs which I thought could be affected by these changes, and confirmed that they produced the same results in the new code as they did in the original implementation. This includes: - `WriteConsoleOutput` - `ReadConsoleOutput` - `SetConsoleTextAttribute` with `WriteConsoleOutputCharacter` - `FillConsoleOutputAttribute` and `FillConsoleOutputCharacter` - `ScrollConsoleScreenBuffer` - `GetConsoleScreenBufferInfo` - `GetConsoleScreenBufferInfoEx` - `SetConsoleScreenBufferInfoEx` I've also manually tested changing colors via the console properties menu, the registry, and shortcut links, including setting default colors and popup colors. And I've tested that the "Quirks Mode" is still working as expected in PowerShell. In terms of performance, I wrote a little test app that filled a 80x9999 buffer with random color combinations using `WriteConsoleOutput`, which I figured was likely to be the most performance sensitive call, and I think it now actually performs slightly better than the original implementation. I've also tested similar code - just filling the visible window - with SGR VT sequences of various types, and the performance seems about the same as it was before.

This PR merges the default colors and cursor color into the main color table, enabling us to simplify the `ConGetSet` and `ITerminalApi` interfaces, with just two methods required for getting and setting any form of color palette entry. The is a follow-up to the color table standardization in #11602, and a another small step towards de-duplicating `AdaptDispatch` and `TerminalDispatch` for issue #3849. It should also make it easier to support color queries (#3718) and a configurable bold color (#5682) in the future. On the conhost side, default colors could originally be either indexed positions in the 16-color table, or separate standalone RGB values. With the new system, the default colors will always be in the color table, so we just need to track their index positions. To make this work, those positions need to be calculated at startup based on the loaded registry/shortcut settings, and updated when settings are changed (this is handled in `CalculateDefaultColorIndices`). But the plus side is that it's now much easier to lookup the default color values for rendering. For now the default colors in Windows Terminal use hardcoded positions, because it doesn't need indexed default colors like conhost. But in the future I'd like to extend the index handling to both terminals, so we can eventually support the VT525 indexed color operations. As for the cursor color, that was previously stored in the `Cursor` class, which meant that it needed to be copied around in various places where cursors were being instantiated. Now that it's managed separately in the color table, a lot of that code is no longer required. ## Validation Some of the unit test initialization code needed to be updated to setup the color table and default index values as required for the new system. There were also some adjustments needed to account for API changes, in particular for methods that now take index values for the default colors in place of COLORREFs. But for the most part, the essential behavior of the tests remains unchanged. I've also run a variety of manual tests looking at the legacy console APIs as well as the various VT color sequences, and checking that everything works as expected when color schemes are changed, both in Windows Terminal and conhost, and in the latter case with both indexed colors and RGB values. Closes #11768

## Summary of the Pull Request This PR refactors the `ConGetSet` API, eliminating some of the bloat produced by the `DoSrvPrivateXXXX` functions, simplifying the method naming to more closely match the `ITerminalApi` interface, and making better use of exceptions for error conditions in place of boolean return values. ## References This is another small step towards merging the `AdaptDispatch` and `TerminalDispatch` classes (#3849). ## PR Checklist * [x] Closes #12193 * [x] Closes #12194 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #3849 ## Detailed Description of the Pull Request / Additional comments There are two main parts to this. The first step was to get rid of all the `DoSrvPrivateXXX` functions, and move their implementation directly into the `ConhostInternalGetSet` class. For the most part this was just copying and pasting the code, but I also fixed a couple of bugs where we were using the wrong output buffer (the global buffer rather than the one associated with the output handle), and got rid of some unnecessary calls to `GetActiveBuffer`. The second part was to make better use of exceptions for error conditions. Instead of catching the exceptions at the `ConGetSet` level, we now allow them to fall through all the way to the `StateMachine`. This greatly simplifies the `AdaptDispatch` implementation since it no longer needs to check a boolean return value on every `ConGetSet` call. This also enables the getter methods to return properties directly instead of having to use a reference parameter. ## Validation Steps Performed A number of the unit tests had to be updated to match the new API. Sometimes this just required changes to method names, but in other cases error conditions that were previously detected with boolean returns now needed to be caught as exceptions. There were also a few direct calls to `DoSrvPrivateXXX` functions that now needed to be invoked through other means: either by generating an equivalent escape sequence, or calling a lower level API serving the same purpose. And in the adapter tests, the mock `ConGetSet` implementation required significant refactoring to match the new interface, mostly to account for the changes in error handling.

* Prevent potential null pointer crash in export buffer action (#12180) ## Summary of the Pull Request Prevents a potential null pointer crash in the export buffer action. ## PR Checklist * [x] Closes #12170 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Validation Steps Performed Manually tested. * Disable being able to set the automatic adjustment of indistinguishable text (#12160) ## Summary of the Pull Request Disables the automatic adjustment of indistinguishable text (added in #11095) because of the concerns brought up in #11917. Also, the setting is hidden in the SUI for as long as this feature remains disabled. ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here * [build] Fix the release build after we removed WTU (#12188) * Merged PR 6815087: [Git2Git] !6814613: conhost FT: skip a test that fails on OneCore [Git2Git] Merged PR 6814613: conhost FT: skip a test that fails on OneCore Fixes MSFT-33720056 Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_wdx_dxp_windev 6410311eee21e9ce147464af89d35f3569d4d2b9 * Setup OneFuzz for CI (#10431) ## Summary of the Pull Request This PR sets up a OneFuzz pipeline on Azure DevOps for our repo. ## Detailed Description of the Pull Request / Additional comments - fuzz.yml: defines the stages and pipeline for ADO - build-console-fuzzing: builds the solution in the Fuzzing configuration - build-console-steps: omits a few tasks that are unnecessary for this build configuration - sln and vcxproj changes: the solution wasn't building in CI. This makes sure that's fixed. - fuzzing.md: a short guide on how to get OneFuzz set up and add a new fuzzer ## References #7638 * atlas: apply colors/flags per column instead of just the first one (#12189) The atlas engine currently applies colors and flags (underline, overline, ...) based on the first column in a glyph; this means that every member of a ligature is drawn in the same color with the same underline style. This commit makes us look up cell color and flags based on the actual buffer column it represents, so that ligatures can be printed in multiple colors and with different underline styles. * Make sure titles always sanitized before passing over conpty (#12211) When title updates are forwarded from the host to the client terminal, they're passed over conpty in an `OSC 0` sequence. If there are any control characters embedded in that title text it's essential they be filtered out, otherwise they are likely to be misinterpreted by the VT parser on the other side. This PR fixes a case where that sanitization step was missed for titles initialized at startup. Originally the sanitization step was handled in `DoSrvSetConsoleTitleW`, which catches title changes made via VT escape sequences, or through the console API, but missed the title initialization at startup. I've now moved that sanitization code into the `CONSOLE_INFORMATION::SetTitle` method, which should cover all cases. This sanitization is only meant to occur when in "pty mode", though, which we were originally establishing with an `IsInVtIoMode` call. However, `IsInVtIoMode` does not return the correct result when the title is set at startup, since the VT I/O thread is not initialized at that point. So I've instead had to change that to an `InConptyMode` call, which determines the conpty state from the launch args. ## Validation Steps Performed I've manually confirmed the test case described in issue #12206 is now working correctly. However, the change to using `InConptyMode` caused some of the unit tests to fail, because there isn't a real conpty connection when testing. Fortunately there are some `EnableConptyModeForTests` methods used by the unit tests to fake the appearance of a conpty connection, and I just needed to add some additional state in one of those methods to trigger the correct `InConptyMode` response. Closes #12206 * ci: move helix queueing to a separate stage as well (#12167) This pull request moves the Helix queuing to a separate stage outside of the build stage. * Fix gridline drawings for DxEngine (#12224) With the introduction of a common `RenderSettings` class in 62c95b5, `PaintBufferGridLines` was now uniformly called with a proper alpha-less `COLORREF` argument. This commit crudely fixes the issue by forcing the color to be fully opaque in the `DxEngine` class. ## PR Checklist * [x] Closes #12223 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Launch pwsh.exe * Hyperlinks get underlines on hover ✅ * AtlasEngine: Make shader hot-reloads possible in Windows Terminal (#12227) This minor change makes it possible to hot-reload the AtlasEngine shaders under Windows Terminal. Launching an UWP application from Visual Studio doesn't necessarily result in a working directory inside the project folder, which makes relative file paths impractical. While the `__FILE__` macro is compiler dependent it works under our build setup with MSVC at least. ## Validation Steps Performed * Shader hot-reloading works under Windows Terminal ✅ * Fix opacity restore for command palette previews (#12229) This commit correctly restores the previous opacity when the command palette preview is cancelled. It includes an additional change in order to make the `AdjustOpacity` setter consistent and symmetric with the `Opacity` getter. ## PR Checklist * [x] Closes #12228 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Open Windows Terminal command palette * Choose "Set background opacity..." * Cycle through the items without selecting one * Press Escape * Previous opacity is restored ✅ * Misc. elevation crash fixes (#12205) This is a collection of fixes: * dd213a5c180997e8b761ca9f8078c0ba58503a0b: This was a crash I discovered while investigating. Probably not the root cause crash, but a crash nonetheless. * ba491212afcbafdabc89d88b78f1dc5076dea0c3...0b18ae4ed73cbb15452c7c15aefa0c2671db9605: A collection of fixes to _not_ create the window when we're about to handoff each of the new tabs, panes, to an elevated window. That should prevent us from starting up XAML at all, which should take care of #12169. Additionally, it'll prevent us from restoring the unelevated windows, which should resolve #12190 * The remainder of the commits where fixes for other weird edge cases as a part of this. Notably: * ff7259918933067872a14df5b0e3c6f6d7764e57: Autopromote the first `split-pane` to a new tab, in the case that it's preceded with only `new-tab` actions that opened elevated windows. #### checklist * [x] I work here * [x] Docs are fine * [x] Closes #12190 * [x] Closes #12169 * Fix unbalanced unlock in PtySignalInputThread::_Shutdown (#12181) The only way to notice that LeaveCriticalSection() was called more often than EnterCriticalSection() is by calling EnterCriticalSection() again in the future, which will then deadlock. Since this bug occurs on exit it wasn't noticeable with the old console lock. With the new, stricter ticket lock this bug causes a fail-fast exit. ## PR Checklist * [x] Closes #12168 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Launch and then exit Windows Terminal * OpenConsole cleanly exits ✅ * PR 6616045: Hand off to Windows Terminal Stable by default (!) This commit also introduces a check that we are in an interactive session before we perform handoff, so as to not break service accounts. It also adds some tracing. * Fix the build after ingesting 3bd3a4f71 * Fix spelling after inbox merge * Uh oh someone broke main on an FI Fixes FI bugs introduced in 33c2cd458a1fb00077c0ae62cd8043e388d50943 * This actually fixes the FI. I obviously forgot to build the tests. * Fix a merge conflict that broke the build (#12245) Less "merge conflict" and more "something that got missed in a merge". The offending commits are * 68ab807 * b3fab51 The Fuzzer build doesn't run on PR, so it didn't notice this couldn't build. @carlos-zamora as an FYI * Add support for hyperlinks to AtlasEngine (#12225) `Renderer` owns the information of the hovered interval in `_hoveredInterval` and provides no access to this information. We can only infer it from calls to `PaintBufferGridLines`, if we're given the request to draw an underline despite the previous call to `UpdateDrawingBrushes` not specifying it. While it'd be possible to fix this and pass the underline flag to `UpdateDrawingBrushes`, I personally consider this aspect of `Renderer` to be a "leaky abstraction" as it's inherently incompatible with any engine not working like `DxEngine`, such as the `AtlasEngine`. It's likely more worthwhile to fundamentally change the `Renderer` architecture in the future. ## PR Checklist * [x] Closes #11871 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Launch pwsh.exe * Hyperlinks are underlined when hovered ✅ * Launch WSL / bash * Run `printf '\e]8;;http://example.com\e\\This is a link\e]8;;\e\\\n'` * Hyperlink is underlined when hovered ✅ * AtlasEngine: Redraw immediately on opacity changes (#12226) Since `AtlasEngine` prefers drawing without alpha for performance reasons, calls to `EnableTransparentBackground` require a draw cycle. This PR makes `Renderer::_NotifyPaintFrame` public and calls it. Related to #9999. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Open Windows Terminal command palette * Choose "Set background opacity..." * Cycling through the items without selecting one changes background opacity immediately ✅ * Pin the `NETCore.App.Host` version to appease nuget (#12248) This gets rid of warnings like ``` NU1102: Unable to find package Microsoft.NETCore.App.Host.win-x64 with version (= 3.1.21) - Found 1 version(s) in TerminalDependencies [ Nearest version: 3.1.18 ] ``` Technically, there's a `NETCore.App.Host.3.1.21` that's out now. We _could_ migrate to that, but then we'd have to make sure to manually re-upload that nuget package to our nuget feed, and then we'd still get this warning the next time that package is updated. Theoretically, there's a 6.x version too, but considering this is a debugging tool, we don't care all that much. * [x] I work here * [x] Discussed this with Dustin * [x] Gets rid of a SUPER TRIVIAL warning. * Send an updated cursor position at the end of writing a run (#12210) I can find the commit where this regressed tomorrow if needed. In #10685 we stopped emitting these notifications while we were deferring cursor drawing. We however forgot to send the notification at the end of the defer. This likely has a small perf impact. We however do need these cursor position events for IMEs to be able to track the cursor position (and low key for #10821) * [x] regressed in #10685 * [x] Closes #11170 * [x] I work here * [x] Tests added * Merged PR 6881653: [Git2Git] Merged PR 6881093: BUILD FIX: Reintroduce BOM to conhost manifests [Git2Git] Merged PR 6881093: BUILD FIX: Reintroduce BOM to conhost manifests Related work items: MSFT-37882151 Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_wdx_dxp_windev b6f388b08b0b431ffc5663fe27eef260688febd6 * Merged PR 6882227: [Git2Git] Merged PR 6881763: Change to hardcoded system GUIDs to appease manifest validation Manifest validation won't accept migration/initialization of HKCU-based registry keys anymore. The enforcement scripts says that they should be defined in code instead. So here it is: defined in code insteead. Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_wdx_dxp_windev 67c720b628de4acefbc381891b7e7d29d387038e Related work items: MSFT-37867666 * Remove the Tray Icon from velocity (#12246) This will turn the feature on always from this point on. That means 1.13 Stble would be the first Stable release to have this feature (unless we cherry-pick this to 1.12 stable, which we may) * [x] Closes #12220 * [x] I work here * Fix Opacity in Windows 10, again (#12255) More fallout from the settings refactor. Probably because testing on a Windows 10 device is hard, because you actually need a physical machine to get acrylic to behave correctly. Basically, the code is simpler now, but we missed the windows 10 only edge case where acrylic can get turned on, but we forget to enable the acrylic brush, so it just stays off. Refer to #11619 where this regressed, and #11643, #12229, because this is just a hard problem apparently * [x] Closes #11743. Technically OP is complaining about behavior that's by-design, but it made me realize this regressed in 1.12. * [ ] No tests on this part of the `TermControl` unfortunately. * [x] Hauled out my old Win10 laptop to verify that opacity works right: - [x] A fresh profile isn't created with any opacity - [x] Mouse wheeling turns on acrylic - [x] Using `opacity` only in the settings still stealthily enables acrylic * Update PGO helpers to mitigate Y2K22 bug (#12262) The PGO helpers NuGet had the Y2K22 bug. This receives and integrates the updated package in our project to restore NuGet functionality. ## PR Checklist * [x] Closes #12261 * [x] I work here * [x] If it builds it sits. ## Validation Steps Performed * [x] Build new PGO instrument data with this pipeline update: https://dev.azure.com/microsoft/Dart/_build/results?buildId=44304850&view=results * Expose Defterm info to ComboBoxItem properly (#12259) ## Summary of the Pull Request Makes `Model::DefaultTerminal` an `IStringable`, which presents it as a string, when possible. This enables text search for defterm's setting. This also enables screen readers to identify the combo box items by their text content (app name, author, and version) as opposed to being treated as an item containing more text. As a part of that, I cleaned up the UIA tree to treat the item's name as "\<name\>, \<author\>, \<version\>". This is consistent with how the Settings App presents installed apps in Apps > Installed apps. #11251 will be resolved upon verification by the accessibility team. ## Validation Steps Performed Verified using Narrator and Accessibility Insights. * Add a keyboard shortcut handler to the TabRowControl (#12260) This makes the scenario mentioned in #8480 work. It's maybe not as holistic a solution as we'd like, but it definitely works. Tested both with Narrator, and using the TabView scroll handles to get tab focus into the tab row manually * [x] Closing the _active_ tab with <kbd>Ctrl+Shift+w</kbd> works (not the `TabViewItem` that has focus, but that's how Edgium works so that seems fine) * [x] Opening a tab with <kbd>Ctrl+Shift+t</kbd> works * [x] Opening the cmdpal with <kbd>Ctrl+Shift+p</kbd> works * [x] Will take care of #8480 once we get the a11y team to validate * [x] I work here #### Notes: None of ```xaml PreviewKeyDown="_KeyDownHandler" KeyDown="_KeyDownHandler" KeyUp="_KeyDownHandler" ``` On the TerminalPage directly seem to fire when the focus is in the `TabViewItem` or the New Tab flyout. But they fire just fine when focus is in the `TermControl`. Interesting, because you'd think that the `TermControl` would have already handled the key... * [Command Palette] Announce if suggestions were found to screen readers (#12266) ## Summary of the Pull Request Expands on #9582. If the command palette finds results, the screen reader says "Suggestions available". Makes the scenario mentioned in #7907 work. This is sufficient for various reasons: 1. According to the bug report, saying that suggestions are available is sufficient > Screen reader should provide the results info on searching commands like 10 results found or suggestions available when there are any search results (Source: #7907) 2. This is common practice. Settings app and XAML Controls Gallery do this for their search box. Also, the user should be able to know how many results were found by tabbing/selecting a result item. When this is done, the screen reader will use `SizeOfSet` and `PositionInSet` to announce how many results were found and which one we're currently on. ## Validation Steps Performed Verified this behavior using Narrator. Verified it matches the behavior of the Settings app and the XAML Controls Gallery. * Switch WinUI to the Windows 11 styles (#12241) * Rename the "Bold" SGR attribute as "Intense" (#12270) When we gave users the ability to configure how the `SGR 1` attribute should be rendered, we described those options as "intense is bright" and "intense is bold". Internally, though, we still referred to the `SGR 1` attribute as bold. This PR renames all occurrences of "Bold" (when referring to the `SGR 1` attribute) as "Intense", so the terminology is more consistent now. PR #10969 is where we decided on the wording to describe the `SGR 1` attribute. Specific changes include: * `TextAttribute::IsBold` method renamed to `IsIntense` * `TextAttribute::SetBold` method renamed to `SetIntense` * `VtEngine::_SetBold` method renamed to `_SetIntense` * `ExtendedAttributes::Bold` enum renamed to `Intense` * `GraphicsOptions::BoldBright` enum renamed to `Intense` * `GraphicsOptions::NotBoldOrFaint` enum renamed to `NotIntenseOrFaint` * `SgrSaveRestoreStackOptions::Boldness` enum renamed to `Intense` ## Validation Steps Performed I've checked that the code still compiles and the unit tests still run successfully. Closes #12252 * AtlasEngine: Implement ClearType blending (#12242) This commit extracts DirectWrite related shader code into dwrite.hlsl and adds support for ClearType blending. Additionally the following changes are piggybacked into this commit: * Some incorrect code around fallback glyph sizing was removed as this is already accomplished by `CreateTextLayout` internally * Hot-reload failed to work with dwrite.hlsl as the `pFileName` parameter was missing * Legibility of the dotted underline was improved by increasing the line gap from 1:1 to 3:1 Part of #9999. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Types are clear ✅ * Fix defterm in wake of auto-elevation (#12272) There are a couple places where we now bail immediately on startup, if we think the window is going to get created without any tabs. We do that to prevent a blank window from flashing on the screen when launching auto-elevate profiles. Unfortunately, those broke defterm in a particularly hard to debug way. In the defterm invocation, there actually aren't any tabs when the app completes initialization. We use the initialization to actually accept the defterm handoff. So what would happen is that the window would immediately close itself gracefully, never accepting the handoff. In my defense, #8514, the original auto-elevated PR, predates defterm merging (906edf7) by a few months, so I totally forgot to test this when rolling it into the subsequent iterations of that PR. * Related to: * #7489 * #12137 * #12205 * [x] Closes #12267 * [x] I work here * [ ] No tests on this code unfortunately * [x] Tested manually Includes a semi-related code fix to #10922 to make that quieter. That is perpetually noisy, and when trying to debug defterm, you've only got about 30s to do that before it bails, so the `sxe eh` breaks in there are quite annoying. * Rename "Windows Terminal" -> "Terminal" (#12264) This should be most of the surfaces that we really care about for displaying "Windows Terminal". There's a pile of other references in code, but I couldn't find any other resources that mention it. I left a lot of the references to Windows Terminal throughout. Seemed like it was fine to keep calling it that in most places, just these localized strings that are going to be displayed in the Shell that should be changed. Rough compare: ![image](https://user-images.githubusercontent.com/18356694/151248506-edf9a6ab-d93f-438f-8755-cdf6ae643736.png) The strings were also moved to the Context Menu resources file, because that's localized into more languages. @DHowett we may want to spin a full build to make sure this works and I didn't miss anything * [x] Closes #12091 * Add support for ctrl+click on the dropdown to launch elevated (#12209) Just like the shift+click and the alt click shortcuts, now Ctrl+Click will launch a new window with that profile, elevated. I also found that the GenerateName wasn't updated for the elevate arg, so added that. I considered adding the following to the defaults, but decided against it. It added 10 more entries to the command palette that were only separated by the `elevate: true` param, so that didn't feel valuable. Those are posted below for posterity. * [x] closes https://github.com/microsoft/terminal/projects/5#card-50759221 * [x] Tested manually Includes a semi-related dead code removal for the `elevated` param for `_OpenNewWindow`, which didn't end up being used, and wouldn't work as intended anyways. * Add Elevate flag to the SUI (#12257) Does what it says on the box ![image](https://user-images.githubusercontent.com/18356694/151168418-68dd9737-ceee-4dbc-8ab4-2b07f5d3fddb.png) * [x] I work here * [x] Discussed in OneNote * Don't open a hole in the terminal window when pasting (#12208) Turns out, this bug only repros in Controls version 2. I'm not sure why, but it didn't repro only on main. So this fix does nothing until #11720 merges. This PR prevents us from setting properties on the paste warning dialog unless we actually need to paste. 5f9c551b7edced4ed72a4b9694998143f96589a3 proves that settings these properties is what would cause the bug in the first place. I went a step further and cleaned this up a bit. This was always a little weird, having to get the `BracketedPasteEnabled` for the active control on the UI thread before we actually display the warning. In the post-#5000 future where going back to the control like this would be a x-proc hop, I figured I should just skip that entirely and plumb the `BracketedPaste` state out in the initial request. * [x] Closes #12202 * [x] I work here * [x] No tests, but there's not a great place for a test like this * [x] Doesn't affect docs See also: #12241 which would introduce #12202 on its own. * Fix broken reset button on some profile settings (#12275) ## Summary of the Pull Request This fixes a bug where several settings would not show the reset button. The root cause of this issue is two fold: 1. Hooking up `CurrentXXX` - `GETSET_BINDABLE_ENUM_SETTING` was hooked up to the **settings** model profile object instead of the **view** model profile object. Since the settings model has no `PropertyChanged` system, any changes were directly being applied to the setting, but not notifying the view model (and thus, the view, by extension) to update themselves. - This fix required me to slightly modify the macro. Rather than using two parameters (object and function name), I used one parameter (path to getter/setter). 2. Responding to the `PropertyChanged` notifications - Now that we're actually dispatching the `PropertyChanged` notifications, we need to actually respond to them. This behavior was defined in `Profiles::OnNavigatedTo()` in the `PropertyChanged()` handler. Funny enough, that code was still there, it just didn't do anything because it was trying to notify that `Profiles::CurrentXXX` changed. This is invalid because `CurrentXXX` got moved to `ProfileViewModel`. - The fix here was pretty easy. Just move the property changed handler to `ProfileViewModel`'s `PropertyChanged` handler that is defined in the ctor. ## References Bug introduced in #11877 ## Validation Steps Performed ✅ Profile termination behavior ✅ Bell notification style ✅ Text antialiasing ✅ Scrollbar visibility * Update our SUI to follow win 11 guidelines (#11720) ## Summary of the Pull Request Updates our SUI to follow the windows 11 style guidelines. Includes updating our setting containers to follow the 'expander' style. ## PR Checklist * [x] Closes #10631 * [x] Closes #9978 * [x] Closes #9595 * [x] Closes #11231 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here * AtlasEngine: Fix various ClearType rendering issues (#12278) This commit fixes the following issues when ClearType rendering is enabled: * Colored glyphs are now drawn using grayscale AA, similar to all current browsers. This ensures proper gamma correctness during blending in our shader, while generally making no discernable difference for legibility. * Our ClearType shader only emits fully opaque colors, just like the official ClearType implementation. Due to this we need to force grayscale AA if the user specifies both ClearType and a background image, as the image would otherwise not be visible. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Grayscale AA when drawing emojis ✅ * Grayscale AA when using a background image ✅ * Add a BreadcrumbBar to the SUI (#12144) ## Summary of the Pull Request **Note: This PR targets #11720** Replaces our old pivot-style settings UI with a breadcrumb bar style, as per the windows 11 style guidelines. This required splitting `Profiles.xaml` into 3 separate files, `Profiles_Base.xaml` for general settings, `Profiles_Appearance.xaml` for appearance settings, `Profiles_Advanced.xaml` for advanced settings The header in the navigation view is now a [BreadcrumbBar](https://docs.microsoft.com/en-us/windows/apps/design/controls/breadcrumbbar), which can be used to navigate back to `Profiles_Base` after moving into the advanced or appearance page (see GIF below)  ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed ![breadcrumb](https://user-images.githubusercontent.com/26824113/150410517-2232811e-4f5b-4732-9a0d-569cc94093b3.gif) * Change where the NotificationIcon looks up our resources (#12282) I didn't have the tray icon enabled before I suppose, so this never got hit? Anyhow, we need to change where we look for the AppName. Otherwise we crash on launch 😨 * [x] fixes `main` * [x] I work here * regressed in #12264 * [x] Tested by: actually running the Terminal with this, it launched * Simplify the IStateMachineEngine interface (#12277) There were a number of methods in the `IStateMachineEngine` interface which controlled how the `StateMachine` interpreted escape sequences. But essentially what it came down to was a bunch of a properties that were always true for the `InputStateMachineEngine`, and always false for the `OutputStateMachine` engine. To simplify the implementation, and make things a little more efficient, I've now replaced all of those virtual calls with a single boolean field in the `StateMachine` that is initialised in the constructor. I started by adding an `isEngineForInput` parameter to the constructor to indicate the the type of engine being passed in. But to keep things simple for callers, and I also then added a constructor without that parameter, which could derive the value automatically based on the type of the engine pointer. Then in the `StateMachine` implementation, anywhere we were previously calling `ParseControlSequenceAfterSs3`, `FlushAtEndOfString`, `DispatchControlCharsFromEscape`, or `DispatchIntermediatesFromEscape`, we now just reference `_isEngineForInput`. But I've also copied across some of the original comments from those methods, to make it clear at the point of usage why we have a difference in behavior for input and output. To make sure the unit tests would catch any problems, I hardcoded the `_isEngineForInput` field to `false`, and confirmed that it broke a bunch of input engine tests. Then I hardcoded it to `true`, and confirmed that it broke a bunch of state machine and output engine tests. With the `_isEngineForInput` set correctly, everything passed. I also manually tested the various output edge cases that would be effected by this code - C0 controls within an escape sequence, time delays in the middle of an escape sequence, `SCS` character set selection which requires intermediates following an escape, and a G3 single shift select which depends on `SS3`. Closes #12254 * Move to the 21H1 Helix pool (#12285) The 19H1 pool is being decommissioned. * Fix SizeOfSet and PositionInSet for 'Open JSON File' nav item (#12286) ## Summary of the Pull Request According to https://github.com/microsoft/microsoft-ui-xaml/issues/1971, `PaneFooter` does not set the `SizeOfSet` or `PositionInSet` properties. However, `FooterMenuItems` does and works for our scenario. So we just replaced `PaneFooter` with `FooterMenuItems`. Will handle #11154 upon verification from the accessibility team. ## Validation Steps Performed ✅ Verified using Accessibility Insights ✅ "Open Json File" button can still be invoked and keyboard navigated to as expected * Fix a number of small issues with the SUI (#12287) ## Summary of the Pull Request Fix various things from the recent SUI changes - The Appearance/Advanced toggle buttons now have a max width - We don't need `Profiles.cpp` anymore - The `Elevate` setting is now back in the SUI - There is no longer an alignment difference between non-expander settings and expander settings - Expander settings no longer require hitting `Tab` twice to get to them  ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here * Two belling fixes (#12281) Sorry for combining two fixes in one PR. I can separate if need be. * [x] Closes #12276: - `"bellSound": null` didn't work. This one was easier, and is atomically in bcc2ca04fc14f39f37849b4bd837ad6cdb4cdaaa. Basically, we would deserialize that as an array with a single empty string in it, which we'd try to then play. I think it's more idomatic to have that deserialized as an empty array, which correctly falls back to playing the default sound. * [x] Closes #12258: - This one is the majority of the rest of the PR. If you leave the MediaPlayer open, then the media keys will _affect the Terminal_. More importantly, once the bell sounds, they'd replay the bell, which is insane. So the fix is to re-create the media player when we need it. We do this per-pane for simpler lifetime tracking. I'm not worried about the overhead of creating a mediaplayer here, since we're already throttling bells. * Originally added in #11511 * [x] Tested manually - Use [`no.mp4`](https://www.youtube.com/watch?v=x2w9TyCv2gk) for this since that's like, 17s long - Checked that closing panes / the terminal while a bell was playing didn't crash - Playing a bunch of bells at once works - closing a pane stops the bell it's playing - once the bell stops, the media keys went back to working for Spotify * [x] I work here * Use the macros for duplicating as well (#12284) Introduced in #11416 We weren't using these macros for duplicating as well, so I forgot to duplicate a couple settings. This PR switches duplicating over to using the macros as well, which shou;d reduce future bugs. Also adds notes to which properties are intentionally omitted from these macros. * [x] closes #12265 * [x] Verified manually that #12120 still works as expected * Code format after inbox integration * Add a11y names to more controls (#12299) ## Summary of the Pull Request This adds names to more of our focusable elements. This should be the rest of them that I missed in #11364 ## References * #9990: a11y megathread * #11155: original version of this ## PR Checklist * [x] Should take care of #11996 once confirmed * [x] I work here ## Validation Steps Performed Used Accessibility Insights to verify. ## Detailed Description of the Pull Request / Additional comments There is one other weird bit. All the expanders that have content below the expander (not inline), show up as focusable, but don't have names. Even when I add names to them. I believe this is due to https://github.com/microsoft/microsoft-ui-xaml/issues/5820, which is fixed in https://github.com/microsoft/microsoft-ui-xaml/pull/6032, in https://github.com/microsoft/microsoft-ui-xaml/releases/tag/v2.8.0-prerelease.220118001. Unfortunately, we're on a 2.7 prerelease, so we don't have that fix yet. I may see how painful moving to that is, because we're gonna get another a11y ping as soon as 1.13 ships. I pre-emptively added names to these guys in f7ba158dc, so that the new MUX should just fix this without any thinking on our part. * Replaced the sizeof parameter of the if statement with ARRAYSIZE (#12273) The pull request fixes the issue where "sizeof" parameter was use instead of "ARRAYSIZE". ## PR Checklist * [x] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments This was a pretty straight forward issue, i just replace sizeof which gives the byte size with ARRAYSIZE which give the number of elements in the array. * Make the SearchBox announce if it found results or not (#12301) Basically, this is the same as #12266, but for the `SearchBoxControl`. Trickily, the ControlCore is the one that knows if there were search results, but the TermControl has to be the one to announce it. * [x] Will take care of #11973 once a11y team confirms * [x] Tested manually with Narrator * [x] Resolves a part of #6319, which I'm repurposing just to displaying the number of results in general. * See also #3920 * Add PGOBuildMode to PGD merge step (#12306) I added a condition to exclude some of the NuGet PGO stuff when there was no build mode, but appear to not have propagated any of it in the PGD merge job. This sets it to Optimize here so it'll go through. ## PR Checklist * [x] Closes #12300 * [x] I work here. * [x] It blends. ## Validation Steps Performed * [x] Ran the PGO Instrument Phase * [x] Ran the PGO Optimize Phase * Add `experimental.useAtlasEngine` to schema (#12304) does what it says on the can. * [x] closes #12302 Co-authored-by: Dustin L. Howett <duhowett@microsoft.com> * AtlasEngine: Fix support for combining diacritics (#12307) `IDWriteTextAnalyzer::GetGlyphs` is not enough to get a `DWRITE_SHAPING_TEXT_PROPERTIES::canBreakShapingAfter` value that works for combining diacritical marks. This requires an additional call to `GetGlyphPlacements`. This commit increases CPU usage for complex text by ~10%. ## PR Checklist * [x] Closes #11925 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * ``echo "[e`u{0301}`u{0301}]"`` prints an "e" with 2 accents ✅ * Set the default WSL starting directory to ~ (#12315) The update that enables this on 20H1+, [KB5007253], went out in November 2021. [KB5007253]: https://support.microsoft.com/en-us/topic/november-22-2021-kb5007253-os-builds-19041-1387-19042-1387-19043-1387-and-19044-1387-preview-d1847be9-46c1-49fc-bf56-1d469fc1b3af * Make sure Terminal Stable shows up as default on 22544+ (#12320) Since we turned this feature on in windows, and it relies on _lying about the contents of the registry_, Terminal needs to be in on the joke. This will need to be reverted and serviced if we choose not to ship like this. Fixes #12308 * Research how often folks disable the KB warning (#12322) * Localize and polish Profile page navigators (#12321) ## Summary of the Pull Request Adds some polish around the navigators in the profile page (i.e. "appearance" and "advanced" button) by doing the following: - use the localized resources for the pivot on the navigators - simplify the navigators to be buttons instead of toggle buttons Doing so has Narrator identify these as buttons rather than toggle buttons. So now Narrator won't say that the button is "off", which just makes more sense. ## Validation Steps Performed ✅ Narrator says "Advanced button" or "Appearance button" on the navigator ✅ The navigators look the same as before * Update readme to account for minversion bump (#12332) As noticed in https://github.com/microsoft/terminal/pull/12129#issuecomment-1027975112. Missed in #12129. * version: bump to 1.14 on main * Update accessible names for 'add profile' page buttons (#12324) ## Summary of the Pull Request When using a screen reader, the buttons on the "add a new profile" page were being read weirdly: - "New empty profile" button read as "create new button button" - "duplicate" button read as "duplicate button button" It's generally standard to read out the text inside the button, so I did just that by reusing the existing localized resources. This also removes the redundant "button" that is said by the screen reader. I also removed the unused `AutomationId` and unnecessary `Button.Content` tags. #11156 can be closed upon validation by the accessibility team. ## Validation Steps Performed ✅ navigate to both buttons using Narrator; make sure it sounds right * Fix more SUI and Rejuvenated UI issues (#12326) - The add new profile page now uses a dropdown rather than radio buttons - Subheaders, breadcrumb bar, buttons etc are now all centralized when the window is maximized (so they all align with the expanders now) - We no longer override the titlebar colors and instead use the xaml defaults (these still aren't great but at least we will get the fix automatically when it happens upstream) - Breadcrumb bar no longer has a negative margin, so there's no weird overlap that happens when the window becomes small - The number boxes for launch size and font size now use the `Inline` placement mode rather than compact, allowing modification to the number with fewer clicks - Textboxes now have a greater max width so they can occupy more space in the expander if needed * appx: Use a different resource for the Properties DisplayName (#12337) We have to do this so that the store sees us as one thing ("Windows Terminal") and the Start menu sees us as another ("Terminal"). The store will reject our package if the value we use for "DisplayName" here doesn't match the store's "reserved names". This value is *not used* by the start menu. * Refactor and simplify the ConGetSet API (#12247) ## Summary of the Pull Request This PR refactors the `ConGetSet` API, eliminating some of the bloat produced by the `DoSrvPrivateXXXX` functions, simplifying the method naming to more closely match the `ITerminalApi` interface, and making better use of exceptions for error conditions in place of boolean return values. ## References This is another small step towards merging the `AdaptDispatch` and `TerminalDispatch` classes (#3849). ## PR Checklist * [x] Closes #12193 * [x] Closes #12194 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #3849 ## Detailed Description of the Pull Request / Additional comments There are two main parts to this. The first step was to get rid of all the `DoSrvPrivateXXX` functions, and move their implementation directly into the `ConhostInternalGetSet` class. For the most part this was just copying and pasting the code, but I also fixed a couple of bugs where we were using the wrong output buffer (the global buffer rather than the one associated with the output handle), and got rid of some unnecessary calls to `GetActiveBuffer`. The second part was to make better use of exceptions for error conditions. Instead of catching the exceptions at the `ConGetSet` level, we now allow them to fall through all the way to the `StateMachine`. This greatly simplifies the `AdaptDispatch` implementation since it no longer needs to check a boolean return value on every `ConGetSet` call. This also enables the getter methods to return properties directly instead of having to use a reference parameter. ## Validation Steps Performed A number of the unit tests had to be updated to match the new API. Sometimes this just required changes to method names, but in other cases error conditions that were previously detected with boolean returns now needed to be caught as exceptions. There were also a few direct calls to `DoSrvPrivateXXX` functions that now needed to be invoked through other means: either by generating an equivalent escape sequence, or calling a lower level API serving the same purpose. And in the adapter tests, the mock `ConGetSet` implementation required significant refactoring to match the new interface, mostly to account for the changes in error handling. * Adding missed MIT license in a few files (#12368) ## Summary of the Pull Request Adding missed MIT license in a few files. ## PR Checklist * [X] Closes #12062 * [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * Always publish the binlog (#12355) We're investigating an issue where the build succeeded but something still got lost somewhere. If we had the binlogs, maybe we could diff and track it down faster. * Fix toggle switches needing a negative margin (#12381) ## Summary of the Pull Request Reducing the `MinWidth` of a toggle switch means it no longer needs a negative margin to align it correctly ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed Setting a different language no longer causes the toggle switch to fall out of the expander * Hardcode the language list for the package manifest and settings dropdown (#12375) The `x-generate` statement seems to have fallen apart somewhere and is no longer generating the valid list of languages for display. This hardcodes the list into the manifest to restore it, which is a valid option per the documentation. We also hardcode the limited subset of languages into the Settings application because the main application supports fewer languages than we have been translated into for the shell extensions for Windows Explorer and Start Menu integration. ## PR Checklist * [x] Closes #12351 * [x] I work here. * [x] Manual tests below ## Validation Steps Performed - [x] Clean built locally with `msbuild.exe openconsole.sln /p:Configuration=Release /p:Platform=x64 /p:WindowsTerminalBranding=Release /t:Terminal\CascadiaPackage /m /bl:log4.binlog` and checked that the `appxmanifest.xml` that popped out the other side contained the same languages that it used to contain. - [x] Built in the release pipeline - [x] Installed release and preview branded packages. Changed my machine language to Polish (pl-PL) which is not one of the fully localized languages, but is one of the limited ones. Checked the start menu and right-click menus and saw Polish text for Terminal and Terminal Preview. Checked the Settings page in our app and saw only the limited 14 language list for the application itself. * Fix disclaimer text not centralizing when maximized (#12374) ## Summary of the Pull Request Fix the disclaimer text boxes in `Rendering` and `Defaults`not centralizing along with the expanders when the window is maximized ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed <img width="1128" alt="defaults" src="https://user-images.githubusercontent.com/26824113/152584084-a999cb29-73bc-4970-889a-f95ea64c1b4c.png"> <img width="1128" alt="rendering" src="https://user-images.githubusercontent.com/26824113/152584099-a54519da-7bca-4ebe-b487-b68ac5cf1a37.png"> * Fix invoking the rightmost breadcrumb bringing you back to Profiles_Base (#12376) ## Summary of the Pull Request We no longer do anything when the rightmost breadcrumb is invoked ## PR Checklist * [x] Closes #12325 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed Tested manually, cannot repro #12325 anymore * Fix the grammar in a comment (#12386) ## Summary of the Pull Request After 'must', the verb is used without 'to'. Correct: "must" or "have to". ## PR Checklist * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA ## Validation Steps Performed Not required, only comment has been changed. * Update Bug_Report.yml (#12404) Added label for "Bug Report". * Fix profile matching for paths containing unquoted whitespace (#12348) The previous code had two bugs for: * paths with more than 1 whitespace The code joins the argv array by replacing null-word terminators with whitespace. Unfortunately it always referred to the separator between `argv[0]` and `argv[1]` for this instead of continuing to join those between 1 and 2, etc. * paths sharing a common prefix with another directory `SearchPathW` returns paths that aren't necessarily paths to files. A call to `GetFileAttributesW` was added, ensuring we only resolve file paths. ## PR Checklist * [x] Closes #12345 * [x] I work here * [ ] Tests added/passed ## Validation Steps Performed * Paths with more than 1 whitespace resolve correctly ✅ * Paths with neighboring directories sharing a common prefix resolve correctly ✅ * Tests added ✅ * Fix a potential crash when setting up the jumplist (#12430) I have no idea how this is even possible to hit. If this is able to be null, then we failed to load the settings in such a catastrophic way that nothing should work. However, OP's Terminal seemed to have already loaded the settings. By all accounts, doesn't make sense. Regardless, the code here would crash if this ever is null, so we may as well catch it. * [x] Closes #12360 * [ ] No way to verify this since it isn't even reproable on OPs machine, but it does have a lot of hits for that failure bucket (!!!) * Fix focus box around color schemes combo box (#12439) The focus box around the color schemes combo box was getting cut off, this change adds a small margin to the stackpanel to allow space for the focus box ## PR Checklist * [x] Closes #12328 * XtermEngine: Explicitly emit cursor state on the first frame (#12434) This commit fixes an issue, where we failed to emit a DECTCEM sequence to hide the cursor if it was hidden before XtermEngine's first frame was finalized. Even in such cases we need to emit a DECTCEM sequence in order to ensure we're in a consistent state. ## Validation Steps Performed * Added test✅ * Run #12401's repro steps around 30 times✅ Closes #12401 * Properly fall back to Segoe MDL2 for our icons on Win10 (#12438) Segoe Fluent isn't available on Windows 10, and doesn't stealthily ship with WinUI. So if we manually set the font family to `"Segoe Fluent Icons"`, then that will just display boxes in Win10. This instead uses the resource `"{ThemeResource SymbolThemeFontFamily}"` which will gracefully fall back on Win10. See: * https://github.com/microsoft/microsoft-ui-xaml/issues/3745, which inspired this solution. Guess what! The backgound image icons were also manually specifying this font, so they had to get updated too. I couldn't find any other `Segoe Fluent` references in the code. * [x] Closes #12350 * [x] Checked Windows 11 locally * [x] Checked Win10 (screenshots incoming from other machine) * Fix browse buttons getting cut off when the window is too narrow (#12435) With the recent change to allow text boxes to be bigger, the `Browse` button that some of them have was getting cut off when the window was too narrow. This change puts the `Browse` button below the text box instead of next to it to prevent this issue. ## PR Checklist * [x] Closes #12335 * Publish the symbols from our MSIX bundle to the public server (#12441) Closes #12203 * Validate cursor position in UIA UTR ctor (#12436) This adds some validation in the `UiaTextRange` ctor for the cursor position. #8730 was caused by creating a `UiaTextRange` at the cursor position when it was in a delayed state (meaning it's purposefully hanging off of the right edge of the buffer). Normally, `Cursor` maintains a flag to keep track of when that occurs, but Windows Terminal isn't maintaining that properly in `Terminal::WriteBuffer`. The _correct_ approach would be to fix `WriteBuffer` then leverage that flag for validation in `UiaTextRange`. However, messing with `WriteBuffer` is a little too risky for our comfort right now. So we'll do the second half of that by checking if the cursor position is valid. Since the cursor is really only expected to be out of bounds when it's in that delayed state, we get the same result (just maybe a tad slower than simply checking a flag). Closes #8730 Filed #12440 to track changes in `Terminal::_WriteBuffer` for delayed EOL wrap. ## Validation Steps Performed While using magnifier, input/delete wrapped text in input buffer. * Allow exceptions from ITerminalApi and TerminalDispatch (#12432) This PR updates the `ITerminalApi` and `TerminalDispatch` classes to allow exceptions to be thrown in case of errors instead of using boolean return values. ## References This brings the Terminal code into alignment with the `AdaptDispatch` and `ConGetSet` changes made in PR #12247. And while this isn't exactly a fix for #12378, it does at least stop the app from crashing now. ## Detailed Description of the Pull Request / Additional comments All the `TerminalDispatch` methods have had their `noexcept` specifiers dropped, and any `try`/`catch` wrapping removed, so exceptions will now fall through to the `StateMachine` class where they should be safely caught and logged. The same goes for the `ITerminalApi` interface and its implementation in the `Terminal` class. And many of the methods in this interface have also had their `bool` return values changed to `void`, since there is usually not a need for error return values now. ## Validation Steps Performed I've manually tested the `OSC 9;9` sequence described in #12378 and confirmed that it no longer crashes. * Fix defterm + elevate by default (#12442) We absolutely cannot allow a defterm connection to auto-elevate. Defterm doesn't work for elevated senarios in the first place. If we try accepting the connection, the spawning an elevated version of the Terminal with that profile... that's a recipe for disaster. We won't ever open up a tab in this window. * [x] Closes #12370 * [x] Tested manually, since there's not a great way to add defterm tests * Don't ever allow `~` as a startingDirectory (#12437) Basically, some WSL distros ship fragments that replace the `commandline` with the executable for their distro (`ubuntu.exe`, etc.). We didn't expect that when we changed the `startingDirectory` for them all to `~`. Unfortunately, `~` is really never a valid path for a process on windows, so those distros would now fail with ``` [error 2147942667 (0x8007010b) when launching `ubuntu1804.exe'] Could not access starting directory "~" ``` If we find that we were unable to mangle `~` into the user's WSL `commandline`, then we will re-evaluate that `startingDirectory` as `%USERPROFILE%`, which is at least something sensible, if albeit not what they wanted. * regressed in #12315 * [x] Closes #12353 * [x] Tested with a (`ubuntu1804.exe`, `~`) profile - launched successfully, where 1.13 in market fails. * [x] added tests * Eliminate the DispatchCommon class (#12389) ## Summary of the Pull Request Other than the `s_ResizeWindow` function, the `DispatchCommon` class was just a couple of static functions indirectly calling the `ConGetSet` interface. So by moving the `s_ResizeWindow` implementation into the `ConhostInternalGetSet` class, we could easily replace all usage of `DispatchCommon` with direct calls to `ConGetSet`. ## PR Checklist * [x] Closes #12253 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #12253 ## Validation Steps Performed I've manually confirmed the resizing operations still work as expected. The other functions are harder to test, but were trivial replacements. * Eliminate the AdaptDefaults class (#12390) ## Summary of the Pull Request The only method that was really needed in the `AdaptDefault` class was the `PrintString` method, and that could easily be moved into the `ConGetSet` interface. That lets us get rid of the `AdaptDefault` class altogether, simplifying the construction of the `AdaptDispatch` class, and brings us more in line with the `TerminalDispatch` implementation. ## PR Checklist * [x] Closes #12318 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #12318 ## Detailed Description of the Pull Request / Additional comments The `Execute` method was never used at all, so there was no problem with losing that. But I also noticed there was an equivalent `ExecuteChar` method in the `ITerminalApi` interface, which also wasn't being used, so I've removed that too. Then there was a `Print` method taking a single `wchar_t` parameter, but that was ultimately implemented as a `PrintString` call anyway, so that translation could easily be accomplished in the `AdaptDispatch` calling code, the same way it's done in `TerminalDispatch`. That left us with the `PrintString` method, which could simply be moved into the `ConGetSet` interface, which would then more closely match the `ITerminalApi` interface. There was also a `GetResult` method, that returned the status of the last `PrintString`, but that result was never actually checked anywhere. What I've done now is make the `PrintString` method throw an exception on failure, and that will be caught and logged in the `StateMachine`. ## Validation Steps Performed I've started a bash shell in conhost to verify that it still works. Since almost everything goes through `PrintString` in VT mode, it should be obvious if something was broken. * Enable the 'automatic adjustment of indistinguishable text' setting for Dev builds (#12444) ## Summary of the Pull Request Followup from our discussion during team sync, the 'automatic adjustment of indistinguishable text' setting is going to be enabled for dev builds only. ## References #12160 ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed Setting appears on dev build and the feature works * Automate packaged submission into Windows (#12449) We're now building a fully provenance, compliance, and security validated package (vpack) through our Release pipeline. This attaches the last phase which automates the submission into the Windows product. It will also automatically trace back the source, commit SHA, and build to the submission here from the Windows side. * [x] Automates a manual activity I performed a few times recently * [x] I work here * [x] Ran a test of it against `release-1.12` and it worked * Add tooltips for nav items in the SUI (#12448) ## Summary of the Pull Request Profiles with long names were having their titles cut off in the navigation view sidebar. This change adds tooltips to all nav view items so the full names can still be read. ## References #11353 ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x[ I work here ## Validation Steps Performed <img width="261" alt="sidebartooltip" src="https://user-images.githubusercontent.com/26824113/153270004-02ec3ca7-8787-41be-a4ee-c60efa8cc5e6.png"> <img width="341" alt="sidebartooltip2" src="https://user-images.githubusercontent.com/26824113/153270033-263069f6-75ff-4215-9c83-e0a946ce9616.png"> * Fix the add/delete unfocused appearance buttons (#12451) ## Summary of the Pull Request The add/delete unfocused appearance buttons now have text on them and are closed to the `Unfocused appearance` header ## References #11353 ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed <img width="548" alt="add" src="https://user-images.githubusercontent.com/26824113/153463971-de14a68b-5ed9-4768-80f8-2a5a5a21bc9f.png"> <img width="557" alt="delete" src="https://user-images.githubusercontent.com/26824113/153463993-9a7413d4-d895-4813-a6ff-1b157f1e72f4.png"> * Update the cmdpal narrator message to include the number of results. (#12429) Updates this narrator announcement message to include the number of results it found. There are two versions: * one for a singular result * one for multiple results. which should help with loc. We're trying to get this in with the loc hotfix, so 👀 please * [x] will take care of the last bit of #7907 verified with narrator locally. * Source index the public symbols too (#12450) Now that we've figured out how to publish the public symbols to the official Microsoft download server... we may as well embed the source code linking information inside of them given that it's right here on GitHub. This attempts to run our existing source linking scripts against the public copy of the symbols. ## PR Checklist * [x] Closes #12443 * [x] I work here * [x] Tested manually ## Validation Steps Performed * [x] Build with it: https://dev.azure.com/microsoft/Dart/_build/results?buildId=44930661&view=logs&j=8f802011-b567-5b81-5fa6-bce316c020ce * [x] Point the debugger at them and see if it can find the sources * [x] Maybe also look at them in a hex editor or whatnot and validate I can see the source paths pointing at GitHub * AtlasEngine: Fix leak check report for OpenConsole (#12415) AtlasEngine enables various debug options for D2D and D3D in Debug builds. Among those are resource leak checks, which were broken in OpenConsole due to the unclean exit in `ServiceLocator::RundownAndExit`. This commit fixes the issue by running the destructors of any renderers registered in the Window class first. ## PR Checklist * [x] Closes #12414 * [x] I work here ## Validation Steps Performed * Set `HKEY_CURRENT_USER\Console\UseDx` to `2` * Run `OpenConsole.exe` * Exit * No exceptions are thrown ✅ * Updates all our icons to use Segoe Fluent (#12469) * use `FontFamily="{ThemeResource SymbolThemeFontFamily}"` where possible, in XAML * use `FontFamily{ L"Segoe Fluent Icons, Segoe MDL2 Assets" }` in codebehind Basically just a simple string replace. * [x] This was a bullet point in #11353 * [x] Confirmed manually on my win10 PC * see also #12438 Actually, this is the last bullet in #11353, so I'm gonna say closes #11353. Screenshots below. * Enable Segoe Variable (#12462) By adding another entry to our `maxversiontested`s. Screenshots in https://github.com/microsoft/terminal/issues/12452#issuecomment-1035356054 * [x] Closes #12452 * [x] I work here * [x] Docs are fine * [x] Tests are fine * Fix off-by-one bug in NormalizeCommandLine (#12484) #12348 introduced an off-by-one bug. While the `NormalizeCommandLine` loop should exit early when there aren't at least _two_ arguments to be joined, the final argument-append needs to happen even if just _one_ argument exists. This commit fixes the issue and introduces changes to additionally monitor the early loop exit, as well as the call to `ExpandEnvironmentStringsW`. ## PR Checklist * [x] Closes #12461 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * All `TerminalSettingsTests` tests pass ✅ * Manually set the colors of the TabViewBackground (#12460) This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode * DON'T default-construct the `MediaPlayer` (#12463) I believe this fixes #12383, but I can't seem to find a way to set up a N SKU VM to confirm this. * [ ] TODO: wait till the morning to finish copying the N vhd I found off the build shares, to confirm this doesn't crash on launch. * Fix a memory leak in onecore interactivity (#1234…

This is an attempt to simplify the `ConGetSet` interface down to the smallest set of methods necessary to support the `AdaptDispatch` class. The idea is that it should then be easier to implement that interface in Windows Terminal, so we can replace the `TerminalDispatch` class with `AdaptDispatch`. This is a continuation of the refactoring started in #12247, and a significant step towards #3849. ## Detailed Description of the Pull Request / Additional comments The general idea was to give the `AdaptDispatch` class direct access to the high-level structures on which it needs to operate. Some of these structures are now passed in when the class is constructed (the `Renderer`, `RenderSettings`, and `TerminalInput`), and some are exposed as new methods in `ConGetSet` (`GetStateMachine`, `GetTextBuffer`, and `GetViewport`). Many of the existing `ConhostInternalGetSet` methods could easily then be reimplemented in `AdaptDispatch`, since they were often simply forwarding to methods in one of the above structures. Some were a little more complicated, though, and require further explanation. * `GetConsoleScreenBufferInfoEx`: What we were typically using this for was to obtain the viewport, although what we really wanted was the virtual viewport, which is now accessible via the `GetViewport` method. This was also used to obtain the cursor position and buffer width, which we can now get via the `GetTextBuffer` method. * `SetConsoleScreenBufferInfoEx`: This was only really used for the `AdaptDispatch::SetColumns` implementation (for `DECCOLM` mode), and that could be replaced with `ResizeWindow`. This is a slight change in behaviour (it sizes the window rather than the buffer), but neither is technically correct for `DECCOLM`, so I think it's good enough for now, and at least it's consistent with the other VT sizing operations. * `SetCursorPosition`: This could mostly be replaced with direct manipulation of the `Cursor` object (accessible via the text buffer), although again this is a slight change in behavior. The original code would also have made a call to `ConsoleImeResizeCompStrView` (which I don't think is applicable to VT movement), and would potentially have moved the viewport (not essential for now, but could later be supported by `DECHCCM`). It also called `VtIo::SetCursorPosition` to handle cursor inheritance, but that should only apply to `InteractDispatch`, so I've moved that to the `InteractDispatch::MoveCursor` method. * `ScrollRegion`: This has been replaced by two simple helper methods in `AdaptDispatch` which better meet the VT requirements - `_ScrollRectVertically` and `_ScrollRectHorizontally`. Unlike the original `ScrollRegion` implementation, these don't generate `EVENT_CONSOLE_UPDATE_SCROLL` events (see #12656 for more details). * `FillRegion`: This has been replaced by the `_FillRect` helper method in `AdaptDispatch`. It differs from the original `FillRegion` in that it takes a rect rather than a start position and length, which gives us more flexibility for future operations. * `ReverseLineFeed`: This has been replaced with a somewhat refactored reimplementation in `AdaptDispatch`, mostly using the `_ScrollRectVertically` helper described above. * `EraseAll`: This was previously handled by `SCREEN_INFORMATION::VtEraseAll`, but has now been entirely reimplemented in the `AdaptDispatch::_EraseAll` method. * `DeleteLines`/`InsertLines`/`_modifyLines`: These have been replaced by the `_InsertDeleteLineHelper` method in `AdaptDispatch`, which mostly relies on the `_ScrollRectVertically` helper described above. Finally there were a few methods that weren't actually needed in the `ConGetSet` interface: * `MoveToBottom`: This was really just a hack to get the virtual viewport from `GetConsoleScreenBufferInfoEx`. We may still want something like in the future (e.g. to support `DECVCCM` or #8879), but I don't think it's essential for now. * `SuppressResizeRepaint`: This was only needed in `InteractDispatch` and `PtySignalInputThread`, and they could easily access the `VtIo` object to implement it themselves. * `ClearBuffer`: This was only used in `PtySignalInputThread`, and that could easily access the buffer directly via the global console information. * `WriteControlInput`: This was only used in `InteractDispatch`, and that could easily be replaced with a direct call to `HandleGenericKeyEvent`. As part of these changes, I've also refactored some of the existing `AdaptDispatch` code: * `_InsertDeleteHelper` (renamed `_InsertDeleteCharacterHelper`) is now just a straightforward call to the new `_ScrollRectHorizontally` helper. * `EraseInDisplay` and `EraseInLine` have been implemented as a series of `_FillRect` calls, so `_EraseSingleLineHelper` is no longer required. * `_EraseScrollback` is a essentially a special form of scrolling operation, which mostly depends on the `TextBuffer::ScrollRows` method, and with the filling now provided by the new `_FillRect` helper. * There are quite a few operations now in `AdaptDispatch` that are affected by the scrolling margins, so I've pulled out the common margin setup into a new `_GetVerticalMargins` helper method. This also fixes some edge cases where margins could end up out of range. ## Validation Steps Performed There were a number of unit tests that needed to be updated to work around functions that have now been removed, but these substitutions were fairly straightforward for the most part. The adapter tests were a different story, though. In that case we were explicitly testing how operations were passed through to the `ConGetSet` interface, but with more than half those methods now gone, a significant rewrite was required. I've tried to retain the crux of the original tests, but we now have to validate the state changes on the underlying data structures, where before that state would have been tracked in the `TestGetSet` mock. And in some cases we were testing what happened when a method failed, but since that scenario is no longer possible, I've simply removed those tests. I've also tried to manually test all the affected operations to confirm that they're still working as expected, both in vttest as well as my own test scripts. Closes #12662

## Summary of the Pull Request This PR replaces the `TerminalDispatch` class with the `AdaptDispatch` class from conhost, so we're no longer duplicating the VT functionality in two places. It also gives us a more complete VT implementation on the Terminal side, so it should work better in pass-through mode. ## References This is essentially part two of PR #12703. ## PR Checklist * [x] Closes #3849 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #12662 ## Detailed Description of the Pull Request / Additional comments The first thing was to give the `ConGetSet` interface a new name, since it's now no longer specific to conhost. I went with `ITerminalApi`, since that was the equivalent interface on the terminal side, and it still seemed like a generic enough name. I also changed the way the api is managed by the `AdaptDispatch` class, so it's now stored as a reference rather than a `unique_ptr`, which more closely matches the way the `TerminalDispatch` class worked. I then had to make sure that `AdaptDispatch` actually included all of the functionality currently in `TerminalDispatch`. That meant copying across the code for bracketed paste mode, the copy to clipboard operation, and the various ConEmu OSC operations. This also required a few new methods to the `ConGetSet`/`ITerminalApi` interface, but for now these are just stubs in conhost. Then there were a few thing in the api interface that needed cleaning up. The `ReparentWindow` method doesn't belong there, so I've moved that into `PtySignalInputThread` class. And the `WriteInput` method was too low-level for the Terminal requirements, so I've replaced that with a `ReturnResponse` method which takes a `wstring_view`. It was then a matter of getting the `Terminal` class to implement all the methods in the new `ITerminalApi` interface that it didn't already have. This was mostly mapping to existing functionality, but there are still a number of methods that I've had to leave as stubs for now. However, what we have is still good enough that I could then nuke the `TerminalDispatch` class from the Terminal code and replace it with `AdaptDispatch`. One oddity that came up in testing, though, was the `AdaptDispatch` implementation of `EraseAll` would push a blank line into the scrollback when called on an empty buffer, whereas the previous terminal implementation did not. That caused problems for the conpty connection, because one of the first things it does on startup is send an `ED 2` sequence. I've now updated the `AdaptDispatch` implementation to match the behavior of the terminal implementation in that regard. Another problem was that the terminal implementation of the color table commands had special handling for the background color to notify the application window that it needed to repaint the background. I didn't want to have to push the color table operations through the `ITerminalApi` interface, so I've instead moved the handling of the background update into the renderer, initiated by a flag on the `TriggerRefreshAll` method. ## Validation Steps Performed Surprisingly this PR didn't require a lot of changes to get the unit tests working again. There were just a few methods used from the original `ITerminalApi` that have now been removed, and which needed an equivalent replacement. Also the updated behavior of the `EraseAll` method in conhost resulted in a change to the expected cursor position in one of the screen buffer tests. In terms of manual testing, I've tried out all the different shells in Windows Terminal to make sure there wasn't anything obviously wrong. And I've run a bunch of the tests from _vttest_ to try and get a wider coverage of the VT functionality, and confirmed everything still works at least as well as it used to. I've also run some of my own tests to verify the operations that had to be copied from `TerminalDispatch` to `AdaptDispatch`.

ghost · 2022-05-24T17:04:00Z

🎉This issue was addressed in #13024, which has now been successfully released as Windows Terminal Preview v1.14.143.:tada:

Handy links:

ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 5, 2019

ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 5, 2019

zadjii-msft added this to the Console Backlog milestone Dec 5, 2019

zadjii-msft mentioned this issue Dec 5, 2019

Add support for XTPUSHSGR / XTPOPSGR #1978

Merged

DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Dec 5, 2019

miniksa mentioned this issue Dec 17, 2019

Make the terminal parser/adapter and related classes use modern, safe structures #3956

Merged

5 tasks

j4james mentioned this issue Dec 20, 2019

Add support for all the line feed control sequences #3271

Merged

5 tasks

j4james mentioned this issue Jan 8, 2020

Remove usages of old safe math #4153

Open

j4james mentioned this issue Jan 23, 2020

Remove unneeded VT-specific control character handling #4289

Merged

5 tasks

j4james mentioned this issue Mar 30, 2020

Reimplement the VT tab stop functionality #5173

Merged

5 tasks

j4james mentioned this issue May 6, 2020

Refactor the SGR implementation in AdaptDispatch #5758

Merged

5 tasks

This was referenced Jun 30, 2020

Refactor the TerminalDispatch graphics rendition to match AdaptDispatch #6725

Closed

Refactor TerminalDispatch (graphics) to match AdaptDispatch #6728

Merged

j4james mentioned this issue Apr 10, 2021

Task: Remove unused methods in ConGetSet and SCREEN_INFORMATION #9771

Closed

j4james mentioned this issue May 22, 2021

Support for XTSAVE and XTRESTORE or DECRQM #10153

Open

j4james mentioned this issue Sep 30, 2021

Consolidate the interfaces for setting VT input modes #11384

Merged

j4james mentioned this issue Oct 8, 2021

Standardise the color table order #11461

Closed

j4james mentioned this issue Oct 24, 2021

Standardize the color table order #11602

Merged

6 tasks

j4james mentioned this issue Nov 18, 2021

Consolidate the color palette APIs #11784

Merged

zadjii-msft modified the milestones: Console Backlog, Engineering Improvements Jan 4, 2022

j4james mentioned this issue Jan 25, 2022

Refactor and simplify the ConGetSet API #12247

Merged

7 tasks

j4james mentioned this issue Feb 22, 2022

Can we get rid of the IRenderTarget interface? #12551

Closed

This was referenced Mar 10, 2022

Further simplification of the ConGetSet interface #12662

Closed

Further refactor and simplify the ConGetSet API #12703

Merged

j4james mentioned this issue May 2, 2022

Merge the TerminalDispatch and AdaptDispatch classes #13024

Merged

6 tasks

ghost added the In-PR This issue has a related PR label May 2, 2022

ghost closed this as completed in #13024 May 4, 2022

ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 4, 2022

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-duplicate code for TerminalDispatch and AdaptDispatch #3849

De-duplicate code for TerminalDispatch and AdaptDispatch #3849

zadjii-msft commented Dec 5, 2019 •

edited by DHowett-MSFT

Loading

miniksa commented Dec 5, 2019

miniksa commented Dec 17, 2019

j4james commented Dec 18, 2019

j4james commented Apr 11, 2021

miniksa commented Jul 9, 2021

ghost commented May 24, 2022

De-duplicate code for TerminalDispatch and AdaptDispatch #3849

De-duplicate code for TerminalDispatch and AdaptDispatch #3849

Comments

zadjii-msft commented Dec 5, 2019 • edited by DHowett-MSFT Loading

miniksa commented Dec 5, 2019

miniksa commented Dec 17, 2019

j4james commented Dec 18, 2019

j4james commented Apr 11, 2021

miniksa commented Jul 9, 2021

ghost commented May 24, 2022

zadjii-msft commented Dec 5, 2019 •

edited by DHowett-MSFT

Loading