Spec for tab tear off and default app #2080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…fault application scenarios.
miniksa
added
Issue-Feature
miniksa
requested review from
bitcrazed,
carlos-zamora,
DHowett-MSFT and
zadjii-msft
|
A fair amount of what's written applies specifically to conhost connections/having conpty handles. How much of it is translatable to other types of connections/situations where we do not have conpty in between? |
zadjii-msft
reviewed
Jul 25, 2019
| If a `wt.exe` is already started, `conhost.exe` would find the running instance and just add a new tab at the end of the tab bar by the same mechanism. | ||
|
|
||
| #### Multiple WTs already started | ||
| If multiple `wt.exe`s are already started, `conhost.exe` would have to find the foreground one, the active one, or the primary/manager one and send the tab there. I'm not sure how other tabbing things to do this. We could research/study. |
There was a problem hiding this comment.
Presumably whenever a wt is activated, it'd communicate to the manager that it's the last active one. The manager would track that one. I'm not sure how we'd communicate that state when an old manager dies and a new wt takes over, unless we have a separate manager process entirely.
There was a problem hiding this comment.
@zadjii-msft can't you achieve this simply with named shared memory region? No need for a separate process at all. o-O
There was a problem hiding this comment.
@lostmsu, oh probably. Much of writing out the spec is to get people who have an idea like yours to pipe up and inform us of a better way. So thank you!
| I believe the action plan here would be to implement what we can, observe the state of the world, and correct going forward. We don't have a solid understanding of how many client applications might be impacted by this apparent change. It also might be perfectly OK because the client applications will always remain parented to the same `conhost.exe` even if those `conhost.exe`s don't report up to the correct `wt.exe`. | ||
|
|
||
| It is also unclear whether someone might want to write a utility from the outside to discover this hierarchy. I would be inclined to not provide a way to do this without a strong case otherwise because attempting to understand the local machine process hierarchy is a great way to box yourself in when attempting to expand later to encompass remote connections. | ||
|
|
There was a problem hiding this comment.
Here's a crazy idea: If there's a manager for both the wt instances, they could both be children of the manager. When wt_A creates a new tab, it actually asks the manager to CreateProcess, and the manager hands the connection back to wt_A. Then all the conhosts are children of the manager, not a particular wt instance. When you drag and drop to wt_B, the conhost doesn't need to get re-parented or anything, only the TermControl does.
There was a problem hiding this comment.
If the manager does, do all Terminals go down? Or do those conhosts get orphaned/retrieved/reshuffled into a new one?
| 1. `AttachConsole` and `DetachConsole` and `AllocConsole` - I don't have the slightest idea what happens for these APIs. We would have to explore. `AttachConsole` has restrictions based on the process hierarchy. It would likely behave in interesting ways with the strange parenting order and might be a driver to why we would have to adjust the parenting of the processes (or change the API under the hood). `DetachConsole` might create an issue where a tab disappears out of the terminal and the job object causes everything to die. `AttachConsole` wouldn't necessarily be guaranteed to go back into the same `wt.exe` or a `wt.exe` at all. | ||
|
|
||
| ## Future considerations | ||
|
|
There was a problem hiding this comment.
Another question, re #1080. How would we handle dragging a tab that doesn't have a terminal in it out? We'd need to somehow move the entire XAML tree of the tab, and all it's panes to the new process. That might be possible with winrt (COM) types.
There was a problem hiding this comment.
I think that is a component UI problem as well.
OK |
3 tasks
zadjii-msft
added a commit
that referenced
this pull request
Jan 15, 2020
## Summary of the Pull Request This is the spec for adding commandline arguments to the Windows Terminal. This includes design work for a powerful future version of the commandline args for the Terminal, as well as a way that system could be implemented during 1.0 to provide basic functionality, while creating commandlines that will work without modification in (a future Windows Terminal version). ## References Referenced in the course of this spec: * #607 Feature Request: wt.exe supports command line arguments (profile, command, directory, etc.) * #1060 Add "open Windows terminal here" into right-click context menu * #576 Feature Request: Task Bar jumplist should show items from profile * #1357 Draft spec for adding profiles to the Windows jumplist * #2080 Spec for tab tear off and default app * #632 [Question] Configuring Windows Terminal profile to always launch elevated * #2068 New window key binding not working ## PR Checklist * [x] Specs #607 * [x] I work here * [x] _it's a spec_ ## Detailed Description of the Pull Request / Additional comments Read the spec. ----------------------------------------------------- * Let's commit this bewfore I go hog-wild on new-window * new-window vs new-tab discussion * Well, this is ready for a review * -P -> -% for --percent * Big note on powershell of course, powershell has to use `;` as the command seperator * Minor typos * This is a lot of feedback from PR bigly, it's focus-pane and focus-tab * Add notes on implementation, based on investigation * Apply suggestions from @miniksa * some updates after actually implementing the thing * some minor things from PR * Apply suggestions from code review Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com> * comments from dustin's latest review * more comments from dustin * mostly just typos Co-authored-by: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
jsoref
reviewed
Mar 26, 2020
| ## Inspiration | ||
|
|
||
| Two main drivers: | ||
| 1. We want the ability to tear off a tab from one Windows Terminal instance and send it to another Windows Terminal instance |
There was a problem hiding this comment.
Suggested change
| 1. We want the ability to tear off a tab from one Windows Terminal instance and send it to another Windows Terminal instance | |
| 1. We want the ability to tear off a tab from one Windows Terminal instance and send it to another Windows Terminal instance. |
|
|
||
| All other `wt.exe` processes starting after the primary should detect the existance of the server manager process and wait on the mutex handle. When the primary disappears, the OS scheduler should choose one of the others to wake up first on the mutex. It can take the lock and then set up the primary management channel. | ||
|
|
||
| Alternatively, if the manager process is completely isolated and we expect all `wt.exe`s to have to remain connected at all times, we can make it such that when the connections are broken between the individual processes and the manager that they all shut down. I would prefer that it is resilient (the previous option) over this one, but browsers must have a good reason for preferring this way. |
There was a problem hiding this comment.
I think one of the reasons is security. If something happens because someone is trying to escape a browser sandbox, you're much happier with a Shut them all down! model.
|
|
||
| For default application launches, `conhost.exe` will have to attempt to transfer the incoming connection to the registered terminal handler instead of launching its own UI window. | ||
|
|
||
| If the registered handler fails to start the connection, there is no registered handler, or any part of this mechanism fails. The `conhost.exe` needs to fall back to doing whatever it would have done prior to this development (launching a window if necessary, being hidden, etc.) |
There was a problem hiding this comment.
Suggested change
| If the registered handler fails to start the connection, there is no registered handler, or any part of this mechanism fails. The `conhost.exe` needs to fall back to doing whatever it would have done prior to this development (launching a window if necessary, being hidden, etc.) | |
| If the registered handler fails to start the connection, there is no registered handler, or any part of this mechanism fails. The `conhost.exe` needs to fall back to doing whatever it would have done prior to this development (launching a window if necessary, being hidden, etc.). |
| The upside here is that most of the startup connection flow happens normally, the `conhost.exe` that was given the server handle is the one that will continue to service it for the lifetime of the command-line application session. I can then discard any concerns about how the driver reacts and how the applications grovel for the relationship between processes as it will be normal. | ||
|
|
||
| The downside here is that launching command-line applications from shortcuts, the shell, or the runbox (as is what triggers the default application scenario) will be using an old version of the PTY. It is possible and even probable that we will make improvements to the PTY that we would want to leverage if they're on the system already inside the app package. However, if we try to transit the server connection to the PTY in the package, we will have to deal with: | ||
| 1. Potentially leaving the original conhost.exe open until the other one exits in case someone is waiting on the process |
There was a problem hiding this comment.
I'm not sure what the style is for these long thoughts, it feels like most of them are sentences.
Suggested change
| 1. Potentially leaving the original conhost.exe open until the other one exits in case someone is waiting on the process | |
| 1. Potentially leaving the original conhost.exe open until the other one exits in case someone is waiting on the process. |
| - To make this work anywhere but the most recent OS build, we'd have to service downlevel. Given `kernelbase.dll` is fundamental to literally everything, there's virtually no chance that we would be allowed to service it backwards in time for the sake of adding a feature. It's too risky by any stretch of the imagination. It's even risky to change `kernelbase.dll` for an upcoming release edition given how fundamental it is. End of thought experiment. | ||
|
|
||
| 3. Updating conhost.exe to look up the launch preference and/or to launch another console host via a protocol handler | ||
| - This would allow the `C:\windows\system32\conhost.exe` to effectively delegate the session to another `conhost.exe` that is hopefully newer than the inbox one. Given that the driver protocol in the box doesn't change and hasn't changed and we don't intend to change it, the forward/backward compatibility story is great here. Additionally, if for whatever reason the delegated `conhost.exe` fails to launch, we can just fall back and launch the old one like we would have prior to the change. It is significantly more likely, but still challenging, to argue for servicing `conhost.exe` back several versions in Windows to make this light up better for all folks. It might be especially more possible if it is a very targeted code snippet that can drop in to all the old versions of the `conhost.exe` code. We would still have the argument about spending resources developing for OS versions that are supposed to be dropped in favor of latest, but it's still a lesser argument than upending all of `kernelbase.dll`. |
There was a problem hiding this comment.
Suggested change
| - This would allow the `C:\windows\system32\conhost.exe` to effectively delegate the session to another `conhost.exe` that is hopefully newer than the inbox one. Given that the driver protocol in the box doesn't change and hasn't changed and we don't intend to change it, the forward/backward compatibility story is great here. Additionally, if for whatever reason the delegated `conhost.exe` fails to launch, we can just fall back and launch the old one like we would have prior to the change. It is significantly more likely, but still challenging, to argue for servicing `conhost.exe` back several versions in Windows to make this light up better for all folks. It might be especially more possible if it is a very targeted code snippet that can drop in to all the old versions of the `conhost.exe` code. We would still have the argument about spending resources developing for OS versions that are supposed to be dropped in favor of latest, but it's still a lesser argument than upending all of `kernelbase.dll`. | |
| - This would allow the `C:\windows\system32\conhost.exe` to effectively delegate the session to another `conhost.exe` that is hopefully newer than the inbox one. Given that the driver protocol in the box doesn't change and hasn't changed and we don't intend to change it, the forward/backward compatibility story is great here. Additionally, if for whatever reason the delegated `conhost.exe` fails to launch, we can just fall back and launch the old one like we would have prior to the change. It is significantly more likely, but still challenging, to argue for servicing `conhost.exe` back several versions in Windows to make this light up better for all folks. It might be especially more possible if it is a very targeted code snippet that can drop into all the old versions of the `conhost.exe` code. We would still have the argument about spending resources developing for OS versions that are supposed to be dropped in favor of latest, but it's still a lesser argument than upending all of `kernelbase.dll`. |
|
|
||
| 3. Updating conhost.exe to look up the launch preference and/or to launch another console host via a protocol handler | ||
| - This would allow the `C:\windows\system32\conhost.exe` to effectively delegate the session to another `conhost.exe` that is hopefully newer than the inbox one. Given that the driver protocol in the box doesn't change and hasn't changed and we don't intend to change it, the forward/backward compatibility story is great here. Additionally, if for whatever reason the delegated `conhost.exe` fails to launch, we can just fall back and launch the old one like we would have prior to the change. It is significantly more likely, but still challenging, to argue for servicing `conhost.exe` back several versions in Windows to make this light up better for all folks. It might be especially more possible if it is a very targeted code snippet that can drop in to all the old versions of the `conhost.exe` code. We would still have the argument about spending resources developing for OS versions that are supposed to be dropped in favor of latest, but it's still a lesser argument than upending all of `kernelbase.dll`. | ||
| - A protocol handler is also well understood and relatively well handled/tested in Windows. Old apps can handle protocols. New apps can handle protocols. Protocol handlers can take arguments. We don't have to lean on any other team to get them to help change the way the rest of the OS works. |
There was a problem hiding this comment.
Suggested change
| - A protocol handler is also well understood and relatively well handled/tested in Windows. Old apps can handle protocols. New apps can handle protocols. Protocol handlers can take arguments. We don't have to lean on any other team to get them to help change the way the rest of the OS works. | |
| - A protocol handler is also well understood and relatively well handled/tested in Windows. Old apps can handle protocols. New apps can handle protocols. Protocol handlers can take arguments. We don't have to lean on any other team to get them to help change the way the rest of the OS works. | |
|
|
||
| I believe the action plan here would be to implement what we can, observe the state of the world, and correct going forward. We don't have a solid understanding of how many client applications might be impacted by this apparent change. It also might be perfectly OK because the client applications will always remain parented to the same `conhost.exe` even if those `conhost.exe`s don't report up to the correct `wt.exe`. | ||
|
|
||
| It is also unclear whether someone might want to write a utility from the outside to discover this hierarchy. I would be inclined to not provide a way to do this without a strong case otherwise because attempting to understand the local machine process hierarchy is a great way to box yourself in when attempting to expand later to encompass remote connections. |
There was a problem hiding this comment.
So, the conceptual equivalent of this is screen. screen -list doesn't precisely show who is the connection client. But if one wants to, one can use pstree to figure out which screen session is tied to which terminal (because each connecting terminal is running a client -- a "plug" -- something like screen -x foo or screen -r foo).
| ## Potential Issues | ||
|
|
||
| I've listed most of the issues above in their individual sections. The primary highlights are: | ||
| 1. Process tree layout - The processes in hierarchy may not make sense to someone inspecting them either visually with a tool or programatically |
There was a problem hiding this comment.
Suggested change
| 1. Process tree layout - The processes in hierarchy may not make sense to someone inspecting them either visually with a tool or programatically | |
| 1. Process tree layout - The processes in hierarchy may not make sense to someone inspecting them either visually with a tool or programatically. |
|
|
||
| I've listed most of the issues above in their individual sections. The primary highlights are: | ||
| 1. Process tree layout - The processes in hierarchy may not make sense to someone inspecting them either visually with a tool or programatically | ||
| 1. Process and kernel object lifetime - Applications may be counting on a specific process or object lifetime in regards to their hosting window and we might be tampering with that in how we apply job objects or shuffle around ownership to make tabs happen |
There was a problem hiding this comment.
Suggested change
| 1. Process and kernel object lifetime - Applications may be counting on a specific process or object lifetime in regards to their hosting window and we might be tampering with that in how we apply job objects or shuffle around ownership to make tabs happen | |
| 1. Process and kernel object lifetime - Applications may be counting on a specific process or object lifetime in regards to their hosting window and we might be tampering with that in how we apply job objects or shuffle around ownership to make tabs happen. |
|
|
||
| ## Future considerations | ||
|
|
||
| This might unlock some sort of isolation for extensions as well. Extensions of some sort our on our own long term roadmap, but they're inherently risky to the stability and integrity of the application. If we have to go through a lot of gyrations to enable process containerization and an interprocess communication model for tab tear off and default application work, we might also be able to contain extensions the same way. This derives further from the idea of what browsers do. |
There was a problem hiding this comment.
Suggested change
| This might unlock some sort of isolation for extensions as well. Extensions of some sort our on our own long term roadmap, but they're inherently risky to the stability and integrity of the application. If we have to go through a lot of gyrations to enable process containerization and an interprocess communication model for tab tear off and default application work, we might also be able to contain extensions the same way. This derives further from the idea of what browsers do. | |
| This might unlock some sort of isolation for extensions as well. Extensions of some sort our on our own long-term roadmap, but they're inherently risky to the stability and integrity of the application. If we have to go through a lot of gyrations to enable process containerization and an interprocess communication model for tab tear off and default application work, we might also be able to contain extensions the same way. This derives further from the idea of what browsers do. |
80 tasks
|
I'm putting this into drafts and closing it as this has been open forever. It can be brought up again in the future and we can reference the closed PR. I just want it out of the queue and off my plate. |
ThunderPower
added a commit
to ThunderPower/terminal
that referenced
this pull request
Sep 5, 2020
* Draw the cursor underneath text, and above the background (#6337)
## Summary of the Pull Request
This is the plan that @miniksa suggested to me. Instead of trying to do lots of work in all the renderers to do backgrounds as one pass, and foregrounds as another, we can localize this change to basically just the DX renderer.
1. First, we give the DX engine a "heads up" on where the cursor is going to be drawn during the frame, in `PrepareRenderInfo`.
- This function is left unimplemented in the other render engines.
2. While printing runs of text, the DX renderer will try to paint the cursor in `CustomTextRenderer::DrawGlyphRun` INSTEAD of `DxEngine::PaintCursor`. This lets us weave the cursor background between the text background and the text.
## References
* #6151 was a spec in this general area. I should probably go back and update it, and we should probably approve that first.
* #6193 is also right up in this mess
## PR Checklist
* [x] Closes #1203
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
* This is essentially `"cursorTextColor": "textForeground"` from #6151.
* A follow up work item is needed to add support for the current behavior, (`"cursorTextColor": null`), and hooking up that setting to the renderer.
* Add a test to ensure support for trailing commas in `settings.json` (#6312)
## Summary of the Pull Request
Adds support for trailing commas in our json files.
## References
* Enabled due to the excellent work over in https://github.com/open-source-parsers/jsoncpp/pull/1098
## PR Checklist
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
* Improve support for VT character sets (#4496)
This PR improves our VT character set support, enabling the [`SCS`]
escape sequences to designate into all four G-sets with both 94- and
96-character sets, and supports invoking those G-sets into both the GL
and GR areas of the code table, with [locking shifts] and [single
shifts]. It also adds [`DOCS`] sequences to switch between UTF-8 and the
ISO-2022 coding system (which is what the VT character sets require),
and adds support for a lot more characters sets, up to around the level
of a VT510.
[`SCS`]: https://vt100.net/docs/vt510-rm/SCS.html
[locking shifts]: https://vt100.net/docs/vt510-rm/LS.html
[single shifts]: https://vt100.net/docs/vt510-rm/SS.html
[`DOCS`]: https://en.wikipedia.org/wiki/ISO/IEC_2022#Interaction_with_other_coding_systems
## Detailed Description of the Pull Request / Additional comments
To make it easier for us to declare a bunch of character sets, I've made
a little `constexpr` class that can build up a mapping table from a base
character set (ASCII or Latin1), along with a collection of mappings for
the characters the deviate from the base set. Many of the character sets
are simple variations of ASCII, so they're easy to define this way.
This class then casts directly to a `wstring_view` which is how the
translation tables are represented in most of the code. We have an array
of four of these tables representing the four G-sets, two instances for
the active left and right tables, and one instance for the single shift
table.
Initially we had just one `DesignateCharset` method, which could select
the active character set. We now have two designate methods (for 94- and
96- character sets), and each takes a G-set number specifying the target
of the designation, and a pair of characters identifying the character
set that will be designated (at the higher VT levels, character sets are
often identified by more than one character).
There are then two new `LockingShift` methods to invoke these G-sets
into either the GL or GR area of the code table, and a `SingleShift`
method which invokes a G-set temporarily (for just the next character
that is output).
I should mention here that I had to make some changes to the state
machine to make these single shift sequences work. The problem is that
the input state machine treats `SS3` as the start of a control sequence,
while the output state machine needs it to be dispatched immediately
(it's literally the _Single Shift 3_ escape sequence). To make that
work, I've added a `ParseControlSequenceAfterSs3` callback in the
`IStateMachineEngine` interface to decide which behavior is appropriate.
When it comes to mapping a character, it's simply an array reference
into the appropriate `wstring_view` table. If the single shift table is
set, that takes preference. Otherwise the GL table is used for
characters in the range 0x20 to 0x7F, and the GR table for characters
0xA0 to 0xFF (technically some character sets will only map up to 0x7E
and 0xFE, but that's easily controlled by the length of the
`wstring_view`).
The `DEL` character is a bit of a special case. By default it's meant to
be ignored like the `NUL` character (it's essentially a time-fill
character). However, it's possible that it could be remapped to a
printable character in a 96-character set, so we need to check for that
after the translation. This is handled in the `AdaptDispatch::Print`
method, so it doesn't interfere with the primary `PrintString` code
path.
The biggest problem with this whole process, though, is that the GR
mappings only really make sense if you have access to the raw output,
but by the time the output gets to us, it would already have been
translated to Unicode by the active code page. And in the case of UTF-8,
the characters we eventually receive may originally have been composed
from two or more code points.
The way I've dealt with this was to disable the GR translations by
default, and then added support for a pair of ISO-2022 `DOCS` sequences,
which can switch the code page between UTF-8 and ISO-8859-1. When the
code page is ISO-8859-1, we're essentially receiving the raw output
bytes, so it's safe to enable the GR translations. This is not strictly
correct ISO-2022 behavior, and there are edge cases where it's not going
to work, but it's the best solution I could come up with.
## Validation Steps Performed
As a result of the `SS3` changes in the state machine engine, I've had
to move the existing `SS3` tests from the `OutputEngineTest` to the
`InputEngineTest`, otherwise they would now fail (technically they
should never have been output tests).
I've added no additional unit tests, but I have done a lot of manual
testing, and made sure we passed all the character set tests in Vttest
(at least for the character sets we currently support). Note that this
required a slightly hacked version of the app, since by default it
doesn't expose a lot of the test to low-level terminals, and we
currently identify as a VT100.
Closes #3377
Closes #3487
* Reflect OS build fixes on 7b489128ac back to inbox
Retrieved from https://microsoft.visualstudio.com os OS official/rs_onecore_dep_uxp 677d15a4e298a0e1e3ce093bc1dff8d832e3c2b1
Related work items: #26765368
* Allow Ctrl+Alt <> AltGr aliasing to be disabled (#6212)
## Summary of the Pull Request
Some people wish to use Ctrl+Alt combinations without Windows treating those as an alias for AltGr combinations. This PR adds a new `altGrAliasing` setting allowing one to control this behavior.
## PR Checklist
* [x] Closes #6211
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Manual testing
* [x] Requires documentation to be updated: https://github.com/MicrosoftDocs/terminal/issues/50
* [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
## Validation Steps Performed
* Choose a German keyboard layout
* Using `showkey -a` ensured that both `Ctrl+Alt+Q/E` and `AltGr+Q/E` produce `@/€`
* Added `"altGrAliasing": false` to the WSL profile
* Using `showkey -a` ensured `Ctrl+Alt+Q/E` now produces `^[^Q/E` while `AltGr+Q/E` continues to produce `@/€`
* Move to Microsoft.UI.Xaml 2.4.0 (#5778)
This brings support for "Compact" tab sizing, which compresses all inactive tabs to just the size of their icons plus the close button. Neat!
It also just keeps us generally up-to-date and good citizens.
* Spec for tab tear off and default app (#2080)
docs have no build, cleaning out draft spec from PR queue
* First draft of a spec for panes with non-terminal content (#1080)
Co-authored-by: Michael Niksa <miniksa@microsoft.com>
putting outdated draft spec into drafts folder and closing PR. CI has no function on docs.
* fix spell things I forced into master.
* Add support for `compact` tab sizing (#5787)
## Summary of the Pull Request
Really couldn't be more starightforward. MUX 2.4 added support for "compact" sized tabs. This PR (targeting the 2.4 PR currently, will move to `master` when that merges) enables users to specify `"tabWidthMode": "compact"` in their global settings to get this behavior.
## References
* #5778 - PR to move to MUX 2.4
* [microsoft-ui-xaml#2016](https://github.com/microsoft/microsoft-ui-xaml/pull/2016) - the MUX PR for compact tab sizing.
* #597 - Tab sizing options?
## PR Checklist
* [x] I don't think we have an issue for this, though I could be wrong.
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
In this screenshot, I'm hovering over tab 2, but the ubuntu tab is focused:
In this screenshot, tab 2 is focused:
* spelling I
* spelling II
* Last round of master spelling fixes
* Update the tab's close button color to match the tab text color (#5789)
## Summary of the Pull Request
When we select a color for the tab, we update the foreground color of the text so that it maintains acceptable contrast with the new tab color. However, we weren't also updating the foreground color of the close button.
This is understandable though, because apparently this wasn't fixable until MUX 2.4 arrived. I'm not a XAML expert, but I know that setting this key only works when we're using MUX 2.4, so I'm assuming something about the TabView implementation changed in that release. _This PR is marked as a draft until #5778 is merged, then I'll re-target to master._
## References
* #5778 - PR to move to MUX 2.4
* This bug was introduced with the tab color picker in #3789
## PR Checklist
* [x] Closes #5780
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
## Validation Steps Performed
A light tab color:
A dark tab color:
* Make the conversion from WORD to TextAttribute explicit (#6380)
In Windows, we build with /Zc:wchar_t- (which makes wchar_t an unsigned
short typedef.) This causes build breaks when we compare two wchar_t
values (or a wchar_t and an enum class that's of type wchar_t) and the
compiler decides that it might want to _promote them to TextAttribute_
before doing the comparison.
* Reflect OS build fixes back from f90f3bf99
Retrieved from https://microsoft.visualstudio.com os OS official/rs_onecore_dep_uxp 272dfa1c4ad5e4202c4c56f3db7a445dc0b003cf
* Improve the legacy color conversions (#6358)
This PR provides a faster algorithm for converting 8-bit and 24-bit
colors into the 4-bit legacy values that are required by the Win32
console APIs. It also fixes areas of the code that were incorrectly
using a simple 16-color conversion that didn't handle 8-bit and 24-bit
values.
The faster conversion algorithm should be an improvement for issues #783
and #3950.
One of the main points of this PR was to fix the
`ReadConsoleOutputAttribute` API, which was using a simplified legacy
color conversion (the original `TextAttribute:GetLegacyAttributes`
method), which could only handle values from the 16-color table. RGB
values, and colors from the 256-color table, would be mapped to
completely nonsensical values. This API has now been updated to use the
more correct `Settings::GenerateLegacyAttributes` method.
But there were also a couple of other places in the code that were using
`GetLegacyAttributes` when they really had no reason to be working with
legacy attributes at all. This could result in colors being downgraded
to 4-bit values (often badly, as explained above), when the code was
already perfectly capable of displaying the full 24-bits.
This included the fill colors in the IME composer (in `ConsoleImeInfo`),
and the construction of the highlighting colors in the color
search/selection handler (`Selection::_HandleColorSelection`). I also
got rid of some legacy attribute code in the `Popup` class, which was
originally intended to update colors below the popup when the settings
changed, but actually caused more problems than it solved.
The other major goal of this PR was to improve the performance of the
`GenerateLegacyAttributes` method, since the existing implementation
could be quite slow when dealing with RGB values.
The simple cases are handled much the same as they were before. For an
`IsDefault` color, we get the default index from the
`Settings::_wFillAttribute` field. For an `IsIndex16` color, the index
can just be returned as is.
For an `IsRgb` color, the RGB components are compressed down to 8 bits
(3 red, 3 green, 2 blue), simply by dropping the least significant bits.
This 8-bit value is then used to lookup a representative 16-color value
from a hard-coded table. An `IsIndex256` color is also converted with a
lookup table, just using the existing 8-bit index.
The RGB mapping table was calculated by taking each compressed 8-bit
color, and picking a entry from the _Campbell_ palette that best
approximated that color. This was done by looking at a range of 24-bit
colors that mapped to the 8-bit value, finding the best _Campbell_ match
for each of them (using a [CIEDE2000] color difference calculation), and
then the most common match became the index that the 8-bit value would
map to.
The 256-color table was just a simpler version of this process. For each
entry in the table, we take the default RGB palette value, and find it's
closest match in the _Campbell_ palette.
Because these tables are hard-coded, the results won't adjust to changes
in the palette. However, they should still produce reasonable results
for palettes that follow the standard ANSI color range. And since
they're only a very loose approximation of the colors anyway, the exact
value really isn't that important.
That said, I have tried to make sure that if you take an RGB value for a
particular index in a reasonable color scheme, then the legacy color
mapped from that value should ideally match the same index. This will
never be possible for all color schemes, but I have tweaked a few of the
table entries to improve the results for some of the common schemes.
One other point worth making regarding the hard-coded tables: even if we
wanted to take the active palette into account, that wouldn't actually
be possible over a conpty connection, because we can't easily know what
color scheme the client application is using. At least this way the
results in conhost are guaranteed to be the same as in the Windows
Terminal.
[CIEDE2000]: https://en.wikipedia.org/wiki/Color_difference#CIEDE2000
## Validation Steps Performed
This code still passes the `TextAttributeTests` that check the basic
`GetLegacyAttribute` behaviour and verify the all legacy attributes
roundtrip correctly. However, some of the values in the `RgbColorTests`
had to be updated, since we're now intentionally returning different
values as a result of the changes to the RGB conversion algorithm.
I haven't added additional unit tests, but I have done a lot of manual
testing to see how well the new algorithm works with a range of colors
and a variety of different color schemes. It's not perfect in every
situation, but I think it works well enough for the purpose it serves.
I've also confirmed that the issues reported in #5940 and #6247 are now
fixed by these changes.
Closes #5940
Closes #6247
* Move all wildcards into targets or expand them (#6406)
Wildcards are not allowed in toplevel ItemGroups in vcxproj; they must
be generated by targets.
We mostly use wildcards for pulling in PRI files that are dumped on disk
by the translation tool. We don't want to check those in, so we can't
expand references to them.
To that end, I've introduced a new target that will take a list of
folders containing resw files and expand wildcards under them.
All[1] other wildcards have been moved into their respective targets
_or_ simply expanded.
[1]: Nothing has complained about the resource wildcards in
CascadiaResources.build.items, so I haven't exploded it yet.
Fixes #6214.
* Remove the shell extension from directory backgrounds (#6415)
We're removing this because of MSFT:24623699, which prevents us from being able to do the right thing when we're called on the background of a directory for a range of OS builds.
#6414 will track re-adding this to the Terminal when the original issue is closed.
* [x] closes #6245
* I work here
* Add support for win32-input-mode to conhost, ConPTY, Terminal (#6309)
Adds support for `win32-input-mode` to conhost, conpty, and the Windows
Terminal.
* The shared `terminalInput` class supports sending these sequences when
a VT client application requests this mode.
* ConPTY supports synthesizing `INPUT_RECORD`s from the input sent to it
from a terminal
* ConPTY requests this mode immediately on startup (if started with a
new flag, `PSEUDOCONSOLE_WIN32_INPUT_MODE`)
* The Terminal now supports sending this input as well, when conpty asks
for it.
Also adds a new ConPTY flag `PSEUDOCONSOLE_WIN32_INPUT_MODE` which
requests this functionality from conpty, and the Terminal requests this
by default.
Also adds `experimental.input.forceVT` as a global setting to let a user
opt-out of this behavior, if they don't want it / this ends up breaking
horribly.
## Validation Steps Performed
* played with this mode in vtpipeterm
* played with this mode in Terminal
* checked a bunch of scenarios, as outlined in a [comment] on #4999
[comment]: https://github.com/microsoft/terminal/issues/4999#issuecomment-628718631
References #4999: The megathread
References #5887: The spec
Closes #879
Closes #2865
Closes #530
Closes #3079
Closes #1119
Closes #1694
Closes #3608
Closes #4334
Closes #4446
* Remove parentheses from Preview and Dev build (#6418)
## Summary of the Pull Request
Remove parentheses from the Preview and Dev build. Now they're called Windows Terminal Preview and Windows Terminal Dev Build respectively.
Also removed them from other identifiers of Terminal for consistency.
## PR Checklist
* [X] Closes #5974
* Upload Windows Terminal 2.0 roadmap (#6419)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Upload the roadmap for Windows Terminal 2.0 and link to it on the README.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [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
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
* wpf: add a .NET Core WPF Test project for the WPF Control (#6441)
This commit introduces a new project that lets you F5 a working instance
of the Wpf Terminal Control.
To make the experience as seamless as possible, I've introduced another
solution platform called "DotNet_x64Test". It is set to build the WPF
projects for "Any CPU" and every project that PublicTerminalCore
requires (including itself) for "x64". This is the only way to ensure
that when you press F5, all of the native and managed dependencies get
updated.
It's all quite cool when it works.
* Set tab title as early as possible (#6433)
When opening a new tab, it takes a few milliseconds before title to
appears. This PR makes it instantaneous.
* Updated the Terminal so that it can load the title from the settings
before it is initialized.
* Load terminal settings in TermControl constructor before the terminal
is initialized (see above).
* Update Tab so that it sets the TabViewItem's title in the constructor
(in Tab::_MakeTabViewItem) instead of waiting for the VT sequence to
set the title (from what I understand).
NOTE 1: there is a similar problem with the tabview icon which is not
fixed by this PR.
NOTE 2: This is only a problem with animations disabled because
otherwise the title fades in so there is enough time for it to be set
when it becomes visible.
## Validation
I ran the terminal and opened a new tab. The title appears instantly.
* Don't snap on input nor dismiss selection for just a modifier key (#6431)
Does what it says on the label. Pure modifier keys weren't making it
this far at all prior to #6309. This PR changes these methods to make
sure that we only dismiss a selection or snap on input when the key
pressed isn't a modifier key.
## References
* regressed in #6309
## PR Checklist
* [x] Closes #6423
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
## Validation Steps Performed
* Tried to repro this in the Terminal, couldn't anymore.
* Don't send a manual F7 keyup (#6442)
## Summary of the Pull Request
When someone asked "Do we need to send a F7 keyup too" in #6309, the right answer was actually _no_. Turns out that while XAML will eat the F7 key**down**, it _won't_ eat the F7 key**up**.
## References
* regressed in #6309
## PR Checklist
* [x] Closes #6438
* [x] I work here
* [ ] Tested manually
* [n/a] Requires documentation to be updated
## Validation Steps Performed
* Checked this with the debug tap
* Tie up some A11y loose threads (#6417)
This pull request moves WindowUiaProvider back into Win32 interactivity
and deletes all mention of it from Windows Terminal. Terminal does not
have a single toplevel window that requires Console-like UIA, as each
Xaml control inside it is in charge of its own destiny.
I've also merged `IUiaWindow` and `IConsoleWindow` back together, as
well as `WindowUiaProviderBase` and `WindowUiaProvider`.
Things look a lot more like they did before we tore them apart.
## PR Checklist
* [x] Closes #3564
* [x] CLA
* [x] Tests added/passed (manual)
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already
## Validation
Carlos validated conhost and terminal on this branch.
* WpfTest: Add an x86/Win32 build, make DPI aware (#6455)
This matches more closely how Visual Studio uses the WPF control.
It comes in the form of _another platform_ (sorry), `DotNet_x86Test`.
* Improve perf by avoiding vector reallocation in renderer clusters and VT output graphics options (#6420)
## Summary of the Pull Request
Caches vectors in the class and uses a new helper to opportunistically shrink/grow as viewport sizes change in order to save performance on alloc/free of commonly used vectors.
## PR Checklist
* [x] Scratches a perf itch.
* [x] I work here.
* [x] wil tests added
* [x] No add'l doc.
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
Two fixes:
1. For outputting lots of text, the base renderer class spent a lot of time allocating and freeing and reallocating the `Cluster` vector that adapts the text buffer information into render clusters. I've now cached this vector in the base render class itself and I shrink/grow it based on the viewport update that happens at the top of every frame. To prevent too much thrashing in the downward/shrink direction, I wrote the `til::manage_vector` helper that contains a threshold to only shrink if it asks for small enough of a size relative to the existing one. I used 80% of the existing size as the threshold for this one.
2. For outputting lots of changing colors, the VT graphics output engine spent a bunch of time allocating and reallocating the vector for `GraphicsOptions`. This one doesn't really have a predictable size, but I never expect it to get extremely big. So I just held it in the base class.
## Validation Steps Performed
* [x] Ran the til unit test
* [x] Checked render cluster vector time before/after against `big.txt` from #1064
* [x] Checked VT graphics output vector time before/after against `cacafire`
Case | Before | After
---|---|---|
`big.txt` |  | 
`cacafire` |  | 
* Reduce latency with DXGI 1.3 GetFrameLatencyWaitableObject (#6435)
This pull request reduces input lag, especially with selection, by using
`IDXGISwapChain2::GetFrameLatencyWaitableObject`.
This is based on the [DXGI 1.3 documentation].
Excerpt from the [DXGI 1.3 improvement list]:
> The following functionality has been added in Microsoft DirectX
Graphics Infrastructure (DXGI) 1.3, which is included starting in
Windows 8.1.
Before, during rendering:
1. render frame
2. call `Present` on swap chain:
2.a. blocks until it can present
2.b. meanwhile, selection/text in terminal might have changed, but
we're still using the frame that we rendered before blocking
2.c. presents
After, during rendering:
1. block until we can present
2. render frame with latest data
3. call `Present` on swap chain:
3.a. present without blocking
[DXGI 1.3 documentation]: https://docs.microsoft.com/en-us/windows/uwp/gaming/reduce-latency-with-dxgi-1-3-swap-chains
[DXGI 1.3 improvement list]: https://docs.microsoft.com/en-us/windows/win32/direct3ddxgi/dxgi-1-3-improvements:
* Open the system menu when user right clicks the drag bar (#6443)
Related to #1375 ("Click/Right click icon should display
Minimize/Maximize/Close menu")
* Deps: Bump Newtonsoft.Json from 10.0.3 to 12.0.3 (#6369)
Bump Newtonsoft.Json from 10.0.3 to 12.0.3
## References
Part of #5297
## PR Checklist
* [ ] Closes (none)
* [x] CLA signed
* [ ] Tests added/passed N/A
* [ ] Requires documentation to be updated N/A
* [ ] 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
CI builds successfully
* Fix the x86 build and re-enable x86 CI (#6467)
This was a miss.
* Add Mini-Spec for openSettings (#5915)
The spec introduces a keybinding argument of 'target' to be able to open a specific settings file. When the Settings UI gets implemented, it will also become an option.
Alternative designs were presented but the 'target' model was decided on.
* Fix 3 different bugs in the WPF control (#6464)
* [wpf] WM_KEYUP crashes on x64 #6444
- Turns out that doing the `(uint)lParam` cast worked fine for the
keydowns, because the value of lParam usually didn't have super
high-order bits set. That's not the case for keyups, where the 30th
bit is _always_ set. This is fixed by explicitly getting the byte
with the scancode in it.
* [wpf] WM_KEYUP generates wrong value in Win32 input mode #6445
- This was fixed by basically the same thing as the above.
* [wpf] WPF control crashes on startup trying to render cursor #6446
- This was a regression from #6337. I forgot to initialize the brush
used to paint the cursor, because the UWP version always uses color
(but the WPF one relies on the text foreground color).
* Also adds a minor change to the WPF test app, so that the user can
actually exit `win32-input-mode`.
* #6337 regressed #6446
* #6309 regressed the other two.
Closes #6444
Closes #6445
Closes #6446
* Finalize Command Palette Spec (#5674)
## Summary of the Pull Request
This PR aims to move the command palette spec out of the draft state and into a finalized state for inclusion in the 2.0 version of the Windows Terminal.
Notably, I've added sections regarding the ability to run `wt` commandlines using the Command Palette UI, something we hadn't considered in the original draft, because `wt` commandlines didn't land for like _4 months_ after this first draft.
## References
* #2046 - the original command palette thread
* #2193 - the original draft PR
* #5400 - the new command palette megathread for WT 2.0, which I'll be updating with follow-up tasks as we work on implementing this.
## PR Checklist
* [x] Specs #2046
* [x] I work here
* [x] Is documentation
## Detailed Description of the Pull Request / Additional comments
_read the spec_
* Pass <Alt> to the application (#6461)
For mysterious reasons lost to the sands of time, XAML will _never_ pass
us a VK_MENU event. This is something that'll probably get fixed in
WinUI 3, but considering we're stuck on system XAML for the time being,
the only way to work around this bug is to pass the event through
manually. This change generalizes the F7 handler into a "direct key
event" handler that uses the same focus and tunneling method to send
different key events, and then uses it to send VK_MENU.
## Validation Steps Performed
Opened the debug tap, verified that I was seeing alt key ups.
Also used some alt keybindings to make sure I didn't break them.
Closes #6421
* Throttle scrollbar updates in TermControl to ~one per 8ms (#4608)
In addition to the below (original) description, this commit introduces
a ThrottledFunc template that can throttle _any_ function. It applies
that type to muffle updates to the scrollbar.
---
Redo #3531 but without the bug that it caused (#3622) which is why it
was reverted.
I'm sorry if I explain this badly. If you don't understand a part, make
sure to let me know and I will explain it better.
### Explanation
How it worked before: `Terminal` signals that viewport changed ->
`TermControl::_TerminalScrollPositionChanged` gets called on the
terminal thread -> it dispatches work for later to be ran the UI thread
to updates the scrollbar's values
Why it's bad:
* If we have many viewport changes, it will create a long stack of
operations to run. Instead, we should just update the scroll bar with
the most recent information that we know.
* Imagine if the rate that the work gets pushed on the UI thread is
greater than the rate that it can handle: it might freeze?
* No need to be real time, we can wait just a little bit (8ms) to
accumulate viewport changes before we actually change the scroll bar's
value because it appears to be expensive (see perf below).
Now: `Terminal` signals that viewport changed ->
`TermControl::_TerminalScrollPositionChanged` gets called on the
terminal thread -> it tells the `ScrollBarUpdater` about a new update ->
the `ScrollBarUpdater` only runs one job (I don't know if that's the
right term) on the UI thread at a time. If a job is already running but
hasn't updated the scroll bar yet, it changes the setting in the already
existing job to update the scroll bar with the new values. A job "waits"
some time before doing the update to throttle updates because we don't
need real time scroll bar updates. -> eventually, it updates the scroll
bar If the user scrolls when a scroll bar update is pending, we keep the
scroll bar's Maximum and Minimum but let the user choose its new Value
with the `CancelPendingValueChange` method.
### Note
Also I changed a little bit the code from the Terminal to notify the
TermControl less often when possible.
I tried to scroll with the scroll bar, with the mouse wheel. I tried to
scroll while content is being outputted.
I tried to reproduce the crash from #2248 without success (good).
Co-authored-by: Leonard Hecker <leonard@hecker.io>
Closes #3622
* Add keybinding arg to openSettings (#6299)
## Summary of the Pull Request
Adds the `target` keybinding arg to `openSettings`. Possible values include: `defaultsFile`, `settingsFile`, and `allFiles`.
## References
#5915 - mini-spec
## PR Checklist
* [x] Closes #2557
* [x] Tests added/passed
## Detailed Description of the Pull Request / Additional comments
Implemented as discussed in the attached spec. A new enum will be added for the SettingsUI when it becomes available.
## Validation Steps Performed
Added the following to my settings.json:
```json
{ "command": "openSettings", "keys":... },
{ "command": { "action": "openSettings" }, "keys":... },
{ "command": { "action": "openSettings", "target": "settingsFile" }, "keys":... },
{ "command": { "action": "openSettings", "target": "defaultsFile" }, "keys":... },
{ "command": { "action": "openSettings", "target": "allFiles" }, "keys":... }
```
* Add fast path to til::bitmap::translate using bitshifts (#6493)
This commit adds a fast path to `til::bitmap::translate`: use bit shifts
when the delta is vertical.
Performance while printing the content of a big file, with the patch
from #6492 which hasn't been merged yet, in Release mode:
Before:
* translate represents 13.08% of samples in InvalidateScroll
After:
* translate represents 0.32% of samples in InvalidateScroll
## Validation
Tests passed.
* Clear cached runs after translate_y (#6501)
"While re-reading the code, I found out that I forgot to do clear cached runs
after translate_y in c360b7588ff8d389b49a4ed60cdee51401a5e172."
* Reintroduce the check for VT_INPUT_MODE in AdaptDispatch (#6485)
This commit reverts the removal of the "SSH hack" in #5383. It was
originally added as a solution to #4911, when we realized that SSH would
request the SS3 cursor key encoding but we weren't equipped to handle
it.
A number of folks have filed issues that, in summary, say "when I use
SSH, I can't select/copy/paste text". It turns out that SSH will _also_
pass through requests for mouse input. Terminal dutifully responds to
those requests, of course, by disabling mouse selection/copy/paste. SSH
is **NOT** actually in VT_INPUT_MODE, so it will never receive the mouse
messages.
It's important to note that even with #376 fixed, we are still required
to keep this check. With the closure of #376, we'll be able to convert
VT mouse input back into Win32 mouse input for Win32 applications . . .
but SSH also doesn't know how to handle Win32 mouse input.
Fixes #6476.
Fixes #6196.
Fixes #5704.
Fixes #5608.
* Extract `ActionAndArgs::FromJson` into its own class (#6351)
## Summary of the Pull Request
Pulls the `ActionAndArgs` deserializing into its own class, separate from `AppKeyBindings`. Some 2.0 features are going to need to re-use these actions in their json, so we'll want one unified way of deserializing them.
## References
* Done primarily as part of the work on #2046/#5400/#5674
* Also related: #1571/#5888
* Will aggressively conflict with any open PRs that introduced keybindings (looking at #6299)
## PR Checklist
* [x] Closes nothing, this is code refactoring
* [x] I work here
* [x] Current tests passed
* [n/a] Requires documentation to be updated
* Fix sending a NUL on alt key up (#6516)
## Summary of the Pull Request
Make sure to set the scancode for the manual alt-up's we're sending. If you don't, then terminalInput in the conpty is going to treat that keypress as an actual NUL, and send that to the connected client.
## References
* regressed in #6421
## PR Checklist
* [x] Closes #6513
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
## Validation Steps Performed
Tested `showkeys -a`
* Only snap on key _downs_ (#6517)
## Summary of the Pull Request
Prior to #6309, we'd only snap on input for non-modifier key_down_ events. #6423 fixed this for modifier keys, but didn't fix this for keyups.
## References
* #6423 was an incomplete fix to this problem, which caused this regression
## PR Checklist
* [x] Closes #6481
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated
* Fix openSettings misspelling in defaults (#6520)
Fixes #6486.
* Remove the WinTelnetEngine (#6526)
Nobody was using it.
Discussed in #2661.
* Spec: Advanced Tab Switcher (#3753)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
This is the spec for the Advanced Tab Switcher. This would allow the user to navigate through a vertical list of tabs through a UI, similar to those found in VSCode and Visual Studio.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
#1502: Feature Request: Advanced Tab Switcher
#973: Ctrl+Tab toggling between two tabs
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Spec for #1502
* [x] CLA signed.
* Introduce JsonUtilsNew as documented in #5875 (#6355)
Read the [JsonUtils Spec] for more details.
This pull request introduces the next version of JsonUtils. It is in a
separate file for ease of review and testing.
JsonUtilsNew will be renamed in a subsequent commit that rewrites our
JSON deserializers.
### Implementer's Notes
I went with telescoping exceptions for the key parsing code, because
it's totally possible that you can be five keys deep and encounter a
type error. This lets us encode information about all failures in the
chain instead of just the topmost one.
The original JsonUtilsNew code changed to use `decay` everywhere because
the tests wouldn't compile. We want to treat `GetValue<const guid>` _the
same as_ `GetValue<guid>`, and this lets us do so. `decay` is awesome.
I've been developing this with a shim that redirects `JsonUtils.h` to
`JsonUtilsNew.h`. I am not comfortable deleting the original until we've
moved off of it, and that _will_ be the subject of a followup PR.
## Validation Steps Performed
So many tests.
[JsonUtils Spec]: https://github.com/microsoft/terminal/blob/master/doc/cascadia/Json-Utility-API.md
Refs #2550
* Fixed #6377: TerminalCore::_altGrAliasing is undefined by default (#6571)
## Summary of the Pull Request
Fixes #6377. `TerminalCore` does not initialize `_altGrAliasing`. The impact is minimized in WT because it defaults to `true` in higher layers. It's not initialized when WPF is driving.
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #6377
* [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
* Use early returns in TermControl::_KeyHandler (#6575)
## Summary of the Pull Request
This PR changes `TermControl::_KeyHandler` to use early returns, which you can think of as "guard clauses".
This has the benefit of a reduced nesting level, easier to understand control flow and opens op the way to more complex conditions.
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
## Validation Steps Performed
Everything still works as expected.
* doc: copyedit the roadmap a little bit (#6587)
* Add 1.1 blog post to 2.0 roadmap (#6581)
* Update PR template with link to docs (#6583)
* Improved ATTR_ROW::ReplaceAttrs performance (#6573)
## Summary of the Pull Request
Improve `ATTR_ROW::ReplaceAttrs` performance by only reserving the necessary capacity instead of resizing the new run.
That way `TextAttributeRun`s are only instantiated once instead of twice.
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
## Detailed Description of the Pull Request / Additional comments
Performance could be further improved by directly moving `TextAttributeRun`s into the new vector, but I considered this out of scope for this PR.
## Validation Steps Performed
CPU usage when running `cacafire` is slightly reduced.
* Add schema check to PR template (#6599)
This adds a check for updating the schema, and rewords the documentation checkbox to match the wording of the others.
* Enable hot reload of renderer settings that aren't already hot reload capable (#6551)
## Summary of the Pull Request
## PR Checklist
* [x] Closes #3927
* [x] I work here.
* [x] Tested manually.
* [x] Requires documentation to be updated: (generate doc bug here)
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
- I found four settings that weren't hot reloadable with the 3927 comment above them:
1. Experimental retro terminal effect
2. Experimental software rendering
3. Experimental full repaint rendering
4. Antialiasing settings for text
I made them all hot reloadable by telling the `TermControl` to propagate them on settings change to the `DxEngine`.
Then I set up the `DxEngine` inside the setters to only set them if they changed. And if they do change, to trigger a full repaint and/or a complete drop and recreate of the entire DX device chain (as would happen if it were lost for another reason like a user-mode graphics failure, disconnected display, etc.)
I made the boolean an atomic because the settings can be coming in off of another thread (the XAML eventing one) and the renderer is picking the status up on its thread at the top of the BeginPaint frame.
## Validation Steps Performed
- [x] Opened it up and toggled all the settings while staring at PowerShell
- [x] Opened it up and toggled all the settings while staring at something intensive like a `cacafire` fire
* Improve bitmap::_calculateArea performance (#6572)
`bitmap::_calculateArea` performance can be improved by leveraging the
optimized `find_first`/`find_next` methods instead of iterating through
the bitmap manually.
* Use D2DDeviceContext and friends over D2DRenderTarget (#6527)
I was told that the DeviceContext version supercedes the RenderTarget
one. This moves us to it so we can gain access to a higher level of
control over the various pieces in our pipeline as we continue to evolve
the renderer.
The underlying motivation here is to potentially use a
`ID2D1CommandList` to batch our commands and run them all later outside
the lock. That can only really be done with this more granular level of
control over the pipeline. So this moves to that in a single step that
is easily findable in history should we have problems
I discussed this with @NiklasBorson of the Direct2D/DirectWrite team as
well as with @DHowett before doing it.
## Validation
- [x] Checked docs to make sure that these work on Windows 7 with
Platform Update
- [x] Manual smoke test real quick
- [ ] Try running on Win7 + Platform Update after change
- [x] Probably do more than just a smoke test manually or otherwise
Closes #6525
* Always use the dark window borders (#6624)
* Recycle assorted rendering components to accelerate drawing (#6483)
This saves an awful lot of construction/destruction and memory
allocation, especially with text that changes a lot (see: cacafire).
Three things:
1. Recycling the text layouts. This holds onto the `CustomTextLayout` so
all the things that don't change related to drawing targets and
whatnot aren't freed and recreated every frame.
2. Reordering the runs in place. This saves a vector
allocation/copy/delete every time OrderRuns is called. They can be
rearranged in place.
3. Only clip once per row. This reduces the clip push/pop to only one
time per row. Since we're always redrawing an entire row at a time,
this saves a lot of alloc/free of the clip frame, dramatically
reduces queued commands, and makes less work on the flush since
clipping requires staging the drawing and then bringing it back to
the main surface.
* Double-click a tab to rename it (#6628)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
When the user double clicks on a tab, show the tab rename box
as if they right clicked on the tab and clicked on "Rename".
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #6600
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
I added a handler for the `DoubleTapped` event on the tab view item
when we are constructing it for the tab (in `Tab::_MakeTabViewItem`).
The code for that handler was copied the "rename tab menu item" click
handler.
I did not extract the code into a member function because it is very
short (only 2 lines of code) and only used twice so it is not worth
it IMO.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
* Spec for unified keybindings and commands, and synthesized action names (#6532)
## Summary of the Pull Request
This is another iteration on the Command Palette spec, from #5674. These were some ideas that were tossed around by @DHowett, @cinnamon-msft and myself, formalized here. I proposed this as an addendum to the original spec, since I think the first made sense atomically, and this only makes sense as a set of changes to the original. I didn't want to go hacking up the original doc to add this set of changes.
**There are two proposals in this spec - they should be viewed as two atomic units. They can be accepted or rejected independently. I'm suggesting we approve both. They work _together_. I'm realizing now that this is worded confusingly, and it's on me to fix that.**
## PR Checklist
* [x] Another spec in the #2046 / #5400 saga
* [x] I work here
* [x] _is a doc_
> ## Abstract
>
> This document is intended to serve as an addition to the [Command Palette Spec].
> While that spec is complete in it's own right, subsequent discussion revealed
> additional ways to improve the functionality and usability of the command
> palette. This document builds largely on the topics already introduced in the
> original spec, so readers should first familiarize themselves with that
> document.
>
> One point of note from the original document was that the original specification
> was entirely too verbose when defining both keybindings and commands for
> actions. Consider, for instance, a user that wants to bind the action "duplicate
> the current pane". In that spec, they need to add both a keybinding and a
> command:
>
> ```json
> {
> "keybindings": [
> { "keys": [ "ctrl+alt+t" ], "command": { "action": "splitPane", "split":"auto", "splitMode": "duplicate" } },
> ],
> "commands": [
> { "name": "Duplicate Pane", "action": { "action": "splitPane", "split":"auto", "splitMode": "duplicate" }, "icon": null },
> ]
> }
> ```
>
> These two entries are practically the same, except for two key differentiators:
> * the keybinding has a `keys` property, indicating which key chord activates the
> action.
> * The command has a `name` property, indicating what name to display for the
> command in the Command Palette.
>
> What if the user didn't have to duplicate this action? What if the user could
> just add this action once, in their `keybindings` or `commands`, and have it
> work both as a keybinding AND a command?
>
* Optimize booleans (#6548)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Many places in this codebase has an equality comparison to the boolean FALSE. This adds unneeded complexity as C and C++ has a NOT operand for use of these in if statements. This makes the code more readable in those areas.
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [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
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
One boolean being compared to FALSE was only used once, with the boolean name being "b", so it is better off not existing at all.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Unit Testing passed, compiler refactoring
* Replace std::map with std::unordered_map (#6640)
Replace std::map with std::unordered_map when the order doesn't matter
and hash functions are provided. Simple optimizations, but I expect the
performance should be strictly better, especially for
CodepointWidthDetector.hpp.
* Update _TerminalCursorPositionChanged to use ThrottledFunc (#6492)
* Update _TerminalCursorPositionChanged to use ThrottledFunc.
* Rename previous ThrottledFunc to ThrottledArgFunc because now
ThrottledFunc is for functions that do not take an argument.
* Update ThrottledFunc and ThrottledArgFunc to accept a CoreDispatcher
on which the function should be called for convenience.
* Don't use coroutines/winrt::fire_and_forget in
ThrottledFunc/ThrottledArgFunc because they are too slow (see PR).
_AdjustCursorPosition went from 17% of samples to 3% in performance
testing.
* version: bump to 1.2 on master
* Add keybinding to rename tab (#6557)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Add keybinding for renaming a tab
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [X] Fulfills format requirements set by #6567
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [X] Tests passed
* [X] Requires documentation to be updated
* [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: #6567 and here (#6557)
This no longer c loses #6256, as the spec changed.
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
* Revert "Skip ... analysis when the ... text is simple (6206)" (#6665)
This reverts commit 94eab6e391a099814ce8f9a08c8cccf9f588b7d1.
We'll reintroduce this again after making sure it plays nicely with
recycling and box drawing glyphs.
Fixes #6488
Fixes #6664
* Implement Shift+MultiClick Selection Expansion (#6322)
This pull request implements shift+double/triple click. Proper behavior
(as described in #4557) is to only expand one selection point, not both.
Adding the `bool targetStart` was a bit weird. I decided on this being
the cleanest approach though because I still want `PivotSelection` to be
its own helper function. Otherwise, the concept of "pivoting" gets kinda
messy.
## Validation Steps Performed
Manual testing as described on attached issue.
Tests were added for Shift+Click and pivoting the selection too.
Closes #4557
* Add `setTabColor` and `openTabColorPicker` actions (#6567)
## Summary of the Pull Request
Adds a pair of `ShortcutAction`s for setting the tab color.
* `setTabColor`: This changes the color of the current tab to the provided color, or can be used to clear the color.
* `openTabColorPicker`: This keybinding immediately activates the tab color picker for the currently focused tab.
## References
## PR Checklist
* [x] scratches my own itch
* [x] I work here
* [x] Tests added/passed
* [x] https://github.com/MicrosoftDocs/terminal/pull/69
## Detailed Description of the Pull Request / Additional comments
## Validation Steps Performed
* hey look there are tests
* Tested with the following:
```json
// { "command": "setTabColor", "keys": [ "alt+c" ] },
{ "keys": "ctrl+alt+c", "command": { "action": "setTabColor", "color": "#123456" } },
{ "keys": "alt+shift+c", "command": { "action": "setTabColor", "color": null} },
{ "keys": "alt+c", "command": "openTabColorPicker" },
```
* When we add a new tab in compact mode, re-enforce Compact mode (#6670)
This workaround was suggested by @chingucoding in
microsoft/microsoft-ui-xaml#2711
Fixes #6570
## References
microsoft/microsoft-ui-xaml#2711
#6570
## PR Checklist
* [x] Closes an issue
* [x] CLA
* [x] Tested
* [x] Docs not required
* [x] Schema not required
* Hardcode the paths to Windows PowerShell and CMD (#6684)
Occasionally, we get users with corrupt PATH environment variables: they
can't lauch PowerShell, because for some reason it's dropped off their
PATH. We also get users who have stray applications named
`powershell.exe` just lying around in random system directories.
We can combat both of these issues by simply hardcoding where we expect
PowerShell and CMD to live. %SystemRoot% was chosen over %WINDIR%
because apparently (according to Stack Overflow), SystemPath is
read-only and WINDIR isn't.
Refs #6039, #4390, #4228 (powershell was not found)
Refs #4682, Fixes #6082 (stray powershell.exe)
* Add support for the Command Palette (#6635)
## Summary of the Pull Request
This adds a first iteration on the command palette. Notable missing features are:
* Commandline mode: This will be a follow-up PR, following the merge of #6537
* nested and iterable commands: These will additionally be a follow-up PR.
This is also additionally based off the addenda in #6532.
This does not bind a key for the palette by default. That will be done when the above follow-ups are completed.
## References
* #2046 - The original command palette thread
* #5400 - This is the megathread for all command palette issues, which is tracking a bunch of additional follow up work
* #5674 and #6532 - specs
* #6537 - related
## PR Checklist
* [x] Closes #2046
- incidentally also closes #6645
* [x] I work here
* [x] Tests added/passed
* [ ] Requires documentation to be updated - delaying this until it's more polished.
## Detailed Description of the Pull Request / Additional comments
* There's a lot of code for autogenerating command names. That's all in `ActionArgs.cpp`, because each case is so _not_ boilerplate, unlike the rest of the code in `ActionArgs.h`.
## Validation Steps Performed
* I've been playing with this for months.
* Tests
* Selfhost with the team
* Restore simple text runs, correct for crashes (#6695)
Restores the simple text run analysis and skipping of most of the
shaping/layout steps. Corrects one of the fast-path steps to ensure that
offsets and clusters are assigned.
## References
- Bug #6488
- Bug #6664
- Simple run PR #6206
- Simple run revert PR #6665
- Recycle glyph runs PR #6483
The "correction" functions, by which box drawing analysis is one of
them, is dependent on the steps coming before it properly assigning the
four main vectors of the text layout glyphs: indices, advances, offsets,
and clusters. When the fast path is identified by the code from #6206,
only two of those are fully updated: indices and advances. The offsets
doesn't tend to cause a problem because offsets are rarely used so
they're pretty much always 0 already (but this PR enforces that they're
zero for the simple/fast path.) The clusters, however, were not mapped
for the fast path. This showed itself in one of two ways:
1. Before the recycled runs PR #6483, the cluster map had a 0 in every
field for the stock initialized vector.
2. After the recycled runs PR #6483, the cluster map had the previous
run's mapping in it.
This meant that when we reached the steps where glyph runs were
potentially split during the correction phase for box drawing
characters, unexpected values were present to map the glyph indices to
clusters and were corrected, adjusted, or split in an unexpected
fashion.
For instance, the index out of range bug could appear when the default 0
values appended to the end of the clusters vector were decremented down
to a negative value during the run splitter as the true DWrite cluster
mapper doesn't generate that sort of pattern in the slow path case
without also breaking the run itself.
The resolution here is therefore to ensure that all of the fields
related to glyph layout are populated even in the fast path. This
doesn't affect the slow path because that one always populated all
fields by asking DWrite to do it. The fast path just skips a bunch of
DWrite steps because it can implicitly identify patterns and save a
bunch of time.
I've also identified a few vectors that weren't cleared on reset/reuse
of the layout. I'm clearing those now so the `.resize()` operations
performed on them to get to the correct lengths will fill them with
fresh and empty values instead of hanging on to ones that may have been
from the previous. This should be OK memory perf wise because the act of
`.clear()` on a vector shouldn't free anything, just mark it invalid.
And doing `.resize()` from an empty one should just default construct
them into already allocated space (which ought to be super quick).
## Validation
* [x] Far.exe doesn't crash and looks fine
* [x] "\e[30;47m\u{2500} What \u{2500}\e[m" from #6488 appears
appropriately antialiased
* [x] Validate the "\e[30;47m\u{2500} What \u{2500}\e[m" still works
when `FillGeometry` is nerfed as a quick test that the runs are split
correctly.
* [x] Put `u{fffd} into Powershell Core to make a replacement char in
the output. Then press enter a few times and see that shrunken initial
characters on random rows. Verify this is gone.
Closes #6668
Closes #6669
Co-Authored-By: Chester Liu <skyline75489@outlook.com>
* Update Cascadia Code to 2007.01 (#6721)
* Add support for OSC 52 (copy-to-clipboard) (#5823)
With this commit, terminal will be able to copy text to the system
clipboard by using OSC 52 MANIPULATE SELECTION DAATA.
We chose not to implement the clipboard querying functionality offered
by OSC 52, as sending the clipboard text to an application without the
user's knowledge or consent is an immense security hole.
We do not currently support the clipboard specifier Pc to specify which
clipboard buffer should be filled
# Base64 encoded `foo`
$ echo -en "\e]52;;Zm9v\a"
# Multiple lines
# Base64 encoded `foo\r\nbar`
$ echo -en "\e]52;;Zm9vDQpiYXI=\a"
Closes #2946.
* Replace old C headers (xxx.h) with modern ones (cxxx) (#5080)
* Improve conpty rendering of default colors in legacy apps (#6698)
Essentially what this does is map the default legacy foreground and
background attributes (typically white on black) to the `IsDefault`
color type in the `TextColor` class. As a result, we can now initialize
the buffer for "legacy" shells (like PowerShell and cmd.exe) with
default colors, instead of white on black. This fixes the startup
rendering in conpty clients, which expect an initial default background
color. It also makes these colors update appropriately when the default
palette values change.
One complication in getting this to work, is that the console permits
users to change which color indices are designated as defaults, so we
can't assume they'll always be white on black. This means that the
legacy-to-`TextAttribute` conversion will need access to those default
values.
Unfortunately the defaults are stored in the conhost `Settings` class
(the `_wFillAttribute` field), which isn't easily accessible to all the
code that needs to construct a `TextAttribute` from a legacy value. The
`OutputCellIterator` is particularly problematic, because some iterator
types need to generate a new `TextAttribute` on every iteration.
So after trying a couple of different approaches, I decided that the
least worst option would be to add a pair of static properties for the
legacy defaults in the `TextAttribute` class itself, then refresh those
values from the `Settings` class whenever the defaults changed (this
only happens on startup, or when the conhost _Properties_ dialog is
edited).
And once the `TextAttribute` class had access to those defaults, it was
fairly easy to adapt the constructor to handle the conversion of default
values …
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I spent some time prototyping what it would take to relay connection information between processes to power both default app changing and tab tear off.
I've associated this with the tab tear-off issue for now, but it could really be either.
I'm marking this as not-draft PR because I'd like to merge it in even though the spec as a whole remains in a drafty/not-totally-complete state. I just want to keep track of everything I learned as of this point in time in a permanent state and use it as fuel for future work/features that fully implement this.