Moved `FindText` to `UiaTextRangeBase`. Now that Search is a shared component (thanks #3279), I can just reuse it basically as-is.
#3279 - Make Search a shared component
#4018 - UiaTextRange Refactor
I removed it from the two different kinds of UiaTextRange and put it in the base class.
I needed a very minor change to ensure we convert from an inclusive end (from Search) to an exclusive end (in UTR).
Worked with `FindText` was globally messed with in windows.h. So we had to do a few weird things there (thanks Michael).
No need for additional tests because it _literally_ just sets up a Searcher and calls it.
## Summary of the Pull Request
This pull request is intended to achieve the following goals...
1) reduce duplicate code
2) remove static functions
3) improve readability
4) improve reliability
5) improve code-coverage for testing
6) establish functioning text buffer navigation in Narrator and NVDA
This also required a change to the wrapper class `XamlUiaTextRange` that has been causing issues with Narrator and NVDA.
See below for additional context.
## References
#3976 - I believe this might have been a result of improperly handling degenerate ranges. Fixed here.
#3895 - reduced the duplicate code. No need to separate into different files
#2160 - same as #3976 above
#1993 - I think just about everything is no longer static
## PR Checklist
* [x] Closes#3895, Closes#1993, Closes#3976, Closes#2160
* [x] CLA signed
* [x] Tests added/passed
## Detailed Description of the Pull Request / Additional comments
### UiaTextRange
- converted endpoints into the COORD system in the TextBuffer coordinate space
- `start` is inclusive, `end` is exclusive. A degenerate range is when start == end.
- all functions are no longer static
- `MoveByUnit()` functions now rely on `MoveEndpointByUnit()` functions
- removed unnecessary typedefs like `Endpoint`, `ScreenInfoRow`, etc..
- relied more heavily on existing functionality from `TextBuffer` and `Viewport`
### XamlUiaTextRange
- `GetAttributeValue()` must return a special HRESULT that signifies that the requested attribute is not supported. This was the cause of a number of inconsistencies between Narrator and NVDA.
- `FindText()` should return `nullptr` if nothing was found. #4373 properly fixes this functionality now that Search is a shared module
### TextBuffer
- Word navigation functionality is entirely in `TextBuffer` for proper abstraction
- a total of 6 functions are now dedicated to word navigation to get a good understanding of the differences between a "word" in Accessibility and a "word" in selection
As an example, consider a buffer with this text in it:
" word other "
In selection, a "word" is defined as the range between two delimiters, so the words in the example include [" ", "word", " ", "other", " "].
In accessibility , a "word" includes the delimiters after a range of readable characters, so the words in the example include ["word ", "other "].
Additionally, accessibility word navigation must be able to detect if it is on the first or last word. This resulted in a slight variant of word navigation functions that return a boolean instead of a COORD.
Ideally, these functions can be consolidated, but that is too risky for a PR of this size as it can have an effect on selection.
### Viewport
- the concept of `EndExclusive` is added. This is used by UiaTextRange's `end` anchor as it is exclusive. To signify that the last character in the buffer is included in this buffer, `end` must be one past the end of the buffer. This is `EndExclusive`
- Since many functions check if the given `COORD` is in bounds, a flag must be set to allow `EndExclusive` as a valid `COORD` that is in bounds.
### Testing
- word navigation testing relies more heavily on TextBuffer tests
- additional testing was created for non-movement focused functions of UiaTextRange
- The results have been compared to Microsoft Word and some have been verified by UiAutomation/Narrator contacts as expected results.
## Validation Steps Performed
Tests pass
Narrator works
NVDA works
## Summary of the Pull Request
#4354 is a pretty complicated PR. It's got a bunch of conpty changes, but what it also has was some critical improvements to the roundtrip test suite. I'm working on some other bugfixes in the same area currently, and need these tests enhancements in those branches _now_. The rest of #4354 is complex enough that I don't trust it will get merged soon (if ever). However, these fixes _should_ be in regardless.
## PR Checklist
* [x] Taken directly from #4354
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
This is four main changes:
* Enable conpty to be fully enabled in unittests. Just setting up a VT renderer isn't enough to trick the host into being in conpty mode - it also needs to have some other flags set.
* Some minor changes to `CommonState` to better configure the common test state for conpty
* Move some of the verify helpers from `ConptyRoundtripTests` into their own helper class, to be shared in multiple tests
* Add a `TerminalBufferTests` class, for testing the Terminal buffer directly (without conpty).
This change is really easier than
would suggest, I promise.
## Summary of the Pull Request
Adds support for commandline arguments to the Windows Terminal, in accordance with the spec in #3495
## References
* Original issue: #607
* Original spec: #3495
## PR Checklist
* [x] Closes#607
* [x] I work here
* [x] Tests added/passed
* [ ] We should probably add some docs on these commands
* [x] The spec (#3495) needs to be merged first!
## Detailed Description of the Pull Request / Additional comments
🛑 **STOP** 🛑 - have you read #3495 yet? If you haven't, go do that now.
This PR adds support for three initial sub-commands to the `wt.exe` application:
* `new-tab`: Used to create a new tab.
* `split-pane`: Used to create a new split.
* `focus-tab`: Moves focus to another tab.
These commands are largely POC to prove that the commandlines work. They're not totally finished, but they work well enough. Follow up work items will be filed to track adding support for additional parameters and subcommands
Important scenarios added:
* `wt -d .`: Open a new wt instance in the current working directory #878
* `wt -p <profile name>`: Create a wt instance running the given profile, to unblock #576, #1357, #2339
* `wt ; new-tab ; split-pane -V`: Launch the terminal with multiple tabs, splits, to unblock #756
## Validation Steps Performed
* Ran tests
* Played with it a bunch
<!-- 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
Let's give it a test drive.
<!-- 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#4162
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Build and run it.
## Summary of the Pull Request
This PR adds two tests:
* First, I started by writing a test where I could write output to the console host and inspect what output came out of conpty. This is the `ConptyOutputTests` in the host unit tests.
* Then I got crazy and thought _"what if I could take that output and dump it straight into the `Terminal`"_? Hence, the `ConptyRoundtripTests` were born, into the TerminalCore unit tests.
## References
Done in pursuit of #4200, but I felt this warranted it's own atomic PR
## PR Checklist
* [x] Doesn't close anything on it's own.
* [x] I work here
* [x] you better believe this adds tests
* [n/a] Requires documentation to be updated
## Detailed Description of the Pull Request / Additional comments
From the comment in `ConptyRoundtripTests`:
> This test class creates an in-proc conpty host as well as a Terminal, to
> validate that strings written to the conpty create the same resopnse on the
> terminal end. Tests can be written that validate both the contents of the
> host buffer as well as the terminal buffer. Everytime that
> `renderer.PaintFrame()` is called, the tests will validate the expected
> output, and then flush the output of the VtEngine straight to th
Also, some other bits had to be updated:
* The renderer needed to be able to survive without a thread, so I hadded a simple check that it actually had a thread before calling `pThread->NotifyPaint`
* Bits in `CommonState` used `NTSTATUS_FROM_HRESULT` which did _not_ work outside the host project. Since the `NTSTATUS` didn't seem that important, I replaced that with a `HRESULT`
* `CommonState` likes to initialize the console to some _weird_ defaults. I added an optional param to let us just use the defaults.
## Summary of the Pull Request
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#4013
* [x] I work here.
* [x] Existing tests should be OK. Real changes, just adding a lib to use.
* [x] Couldn't find any existing docs about intsafe.
* [x] Am core contributor.
## Detailed Description of the Pull Request / Additional comments
* [x] Can we remove min/max completely or rename it in the two projects where it had to be reintroduced? This is now moved into #4152
* [x] How many usages of the old safe math are there? **79**
* [x] If not a ton, can we migrate them here or in a follow on PR? This is now moved into #4153
Files with old safe math:
- TerminalControl: TSFInputControl.cpp
- TerminalCore: TerminalDispatch.cpp
- TerminalCore: TerminalSelection.cpp
- Host: directio.cpp
- RendererGdi: invalidate.cpp
- RendererGdi: math.cpp
- RendererGdi: paint.cpp
- RendererVt: paint.cpp
- TerminalAdapter: adaptDispatch.cpp
- Types: viewport.cpp
- Types: WindowUiaProviderBase.cpp
## Validation Steps Performed
## Summary of the Pull Request
Introduces a type that is basically an array (stack allocated, fixed size) that reports size based on how many elements are actually filled (from the front), iterates only the filled ones, and has some basic vector push/pop semantics.
## PR Checklist
* [x] I work here
* [x] I work here
* [x] I work here
* [ ] I'd love to roll this out to SomeViewports.... maybe in this commit or a follow on one.
* [ ] We need a TIL tests library and I should test this there.
## Detailed Description of the Pull Request / Additional comments
The original gist of this was used for `SomeViewports` which was a struct to hold between 0 and 4 viewports, based on how many were left after subtraction (since rectangle subtraction functions in Windows code simply fail for resultants that yield >=2 rectangle regions.)
I figured now that we're TIL-ifying useful common utility things that this would be best suited to a template because I'm certain there are other circumstances where we would like to iterate a partially filled array and want it to not auto-resize-up like a vector would.
## Validation Steps Performed
* [ ] TIL tests added
The terminal will use the system setting to determine the number of lines to scroll at a time.
This can be overridden by adding rowsToScroll to app global settings file.
terminal will use the system setting if the app setting is 0, or not specified. No restart is needed to reflect setting changes in system or the settings file.
The default was hardcoded to 4 in the code with a todo comment. 1 works better on precision touchpads, where 4 scrolls too fast.
Co-authored-by: Hannes Nel <hannesne@microsoft.com>
<!-- 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
- Enables auditing of Virtual Terminal libraries (input, adapter, parser)
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Rolls audit out to more things
* [x] I work here
* [x] Tests should still pass
* [x] Am core contributor
* [x] Closes#3957
<!-- 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
This is turning on the auditing of these projects (as enabled by the heavier lifting in the other refactor) and then cleaning up the remaining warnings.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
- [x] Built it
- [x] Ran the tests
## Summary of the Pull Request
Refactors parsing/adapting libraries and consumers to use safer and/or more consistent mechanisms for passing information.
## PR Checklist
* [x] I work here
* [x] Tests still pass
* [x] Am a core contributor.
## Detailed Description of the Pull Request / Additional comments
This is in support of hopefully turning audit mode on to more projects. If I turned it on, it would immediately complain about certain classes of issues like pointer and size, pointer math, etc. The changes in this refactoring will eliminate those off the top.
Additionally, this has caught a bunch of comments all over the VT classes that weren't updated to match the parameters lists.
Additionally, this has caught a handful of member variables on classes that were completely unused (and now gone).
Additionally, I'm killing almost all hungarian and shortening variable names. I'm only really leaving 'p' for pointers.
Additionally, this is vaguely in support of a future where we can have "infinite scrollback" in that I'm moving things to size_t across the board. I know it's a bit of a memory cost, but all the casting and moving between types is error prone and unfun to save a couple bytes.
## Validation Steps Performed
- [x] build it
- [x] run all the tests
- [x] everyone looked real hard at it
For our Universal terminal for development purposes, we will use telnet to escape the universal application container and empower developers to debug/diagnose issues with their own machine on loopback to the already-elevated telnet context.
This commit renames the functions in conpty.lib to Conpty* so that they
can be explicitly linked and introduces a header so they can be located.
It also updates the DEF for conpty.dll to reexport them with their
original names.
The crux of the issue here is that TerminalConnection is consuming the
_import_ symbols for the *PseudoConsole family of APIs, which simply
cannot be supplanted by a static library.
Avenues explored: * Exporting __imp_x from the static library to get all
up in kernel32's business. * Using /ALTERNATENAME:__imp_X=StaticX. It
turns out ALTERNATENAME is only consulted when the symbol isn't found
through traditional means.
This, renaming them, is the straightest path forward.
Fixes#3553.
* Adds HasBackgroundImage() and GetExpandedBackgroundImagePath() to
Profiles.cpp/h
* Fills Terminal Settings with expanded path, rather than path value
from profiles.json
* Adds simple regression tests to detect and fail if this fix is
circumvented in the future
Fixes#2922
This commit deletes ConhostConnection and replaces it with
ConptyConnection. The ConptyConnection uses CreatePseudoConsole and
depends on winconpty to override the one from kernel32.
* winconpty must be packageable, so I've added GetPackagingOutputs.
* To validate this, I added conpty.dll to the MSIX regression script.
* I moved the code from conpty-universal that deals with environment
strings into the types library.
This puts us in a way better place to implement #2563, as we can now
separately detect a failure to launch a pseudoconsole, a failure to
CreateProcess, and an unexpected termination of the launched process.
Fixes#1131.
## Summary of the Pull Request
Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does **NOT** add support for those styles to conhost or the terminal.
We will store these "Extended Text Attributes" in a `TextAttribute`. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a `bool` to a single bit in these flags.
Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however.
## References
See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1.
## PR Checklist
* [x] Closes#2554
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
<hr>
* store text with extended attributes too
* Plumb attributes through all the renderers
* parse extended attrs, though we're not renderering them right
* Render these states correctly
* Add a very extensive test
* Cleanup for PR
* a block of PR feedback
* add 512 test cases
* Fix the build
* Fix @carlos-zamora's suggestions
* @miniksa's PR feedback
Adjusts the startup and shutdown behavior of most threads in the console host to alleviate race conditions that are either exacerbated or introduced by the VT PTY threads.
_**This PR targets the #2515 PR**_. It does that for the sake of diffing. When this PR and #2515 are both ready, I'll merge #2515 first, then change the target of this branch, and merge this one.
<!-- 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 PR adds support for "dynamic profiles", in accordance with the [Cascading Settings Spec](https://github.com/microsoft/terminal/blob/master/doc/cascadia/Cascading-Default-Settings.md#dynamic-profiles). Currently, we have three types of default profiles that fit the category of dynamic profile generators. These are profiles that we want to create on behalf of the user, but require runtime information to be able to create correctly. Because they require runtime information, we can't ship a static version of these profiles as a part of `defaults.json`. These three profile generators are:
* The Powershell Core generator
* The WSL Distro generator
* The Azure Cloud Shell generator
<!-- 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#754
* [x] I work here
* [x] look at all these **Tests**
* [x] Requires documentation to be updated - This is done as part of the parent PR
<!-- 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
We want to be able to enable the user to edit dynamic profiles that are generated from DPGs. When dynamic profiles are added, we'll add entries for them to the user's `profiles.json`. We do this _without re-serializing_ the settings. Instead, we insert a partial serialization for the profile into the user's settings.
### Remaining TODOs:
* Make sure that dynamic profiles appear in the right place in the order of profiles -> #2722
* [x] don't serialize the `colorTable` key for dynamic profiles.
* [x] re-parse the user settings string if we've changed it.
* Handle changing the default profile to pwsh if it exists on first launch, or file a follow-up issue -> #2721
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
<hr>
* Create profiles by layering them
* Update test to layer multiple times on the same profile
* Add support for layering an array of profiles, but break a couple tests
* Add a defaults.json to the package
* Layer colorschemes
* Moves tests into individual classes
* adds support for layering a colorscheme on top of another
* Layer an array of color schemes
* oh no, this was missed with #2481
must have committed without staging this change, uh oh. Not like those tests actually work so nbd
* Layer keybindings
* Read settings from defaults.json + profiles.json, layer appropriately
This is like 80% of #754. Needs tests.
* Add tests for keybindings
* add support to unbind a key with `null` or `"unbound"` or `"garbage"`
* Layer or clear optional properties
* Add a helper to get an optional variable for a bunch of different types
In the end, I think we need to ask _was this worth it_
* Do this with the stretch mode too
* Add back in the GUID check for profiles
* Add some tests for global settings layering
* M A D W I T H P O W E R
Add a MsBuild target to auto-generate a header with the defaults.json as a
string in the file. That way, we can _always_ load the defaults. Literally impossible to not.
* When the user's profile.json doesn't exist, create it from a template
* Re-order profiles to match the order set in the user's profiles.json
* Add tests for re-ordering profiles to match user ordering
* Add support for hiding profiles using `"hidden": true`
* Use the hardcoded defaults.json for the exception->"use defaults" case
* Somehow I messed up the git submodules?
* woo documentation
* Fix a Terminal.App.Unit.Tests failure
* signed/unsigned is hard
* Use Alt+Settings button to open the default settings
* Missed a signed/unsigned
* Start dynamically creating profiles
* Give the inbox generators a namespace
and generally hack this a lot less
* Some very preliminary PR feedback
* More PR feedback
Use the wil helper for the exe path
Move jsonutils into their own file
kill some dead code
* Add templates to these bois
* remove some code for generating defaults, reorder defaults.json a tad
* Make guid a std::optional
* Large block of PR feedback
* Remove some dead code
* add some comments
* tag some todos
* stl is love, stl is life
* Serialize the source key
* Make the Azure cloud shell a dynamic profile
* Make the built-in namespaces public
* Add a mechanism for quick-diffing a profile
This will be used to generate the json snippets for dynamically generated profiles.
* Generate partial serializations of dynamic profiles _not_ in the user settings
* Start writing tests for generating dyn profiles
* dyn profiles generate GUIDs based on _source
* we won't run DPGs when they'd disabled?
* Add more DPG tests - TestDontRunDisabledGenerators
* Don't layer profiles with a source that's also different
* Add another test, DoLayerUserProfilesOnDynamicsWhenSourceMatches
* Actually insert new dynamic profiles into the file
* Minor cleanup of `Profile::ShouldBeLayered`
* Migrate legacy profiles gracefully
* using namespace winrt::Windows::UI::Xaml;
* _Only_ layer dynamic profiles from user settings, never create
* Write a test for migrating dynamic profiles
* Comments for dayssssss
* add `-noprofile`
* Fix the crash that dustin found
* -Encoding ASCII
* Set a profile's default scheme to Campbell
* Fix the tests I regressed
* Update UsingJsonSetting.md to reflect that changes from these PRs
* Change how GenerateGuidForProfile works
* Make AppKeyBindings do its own serialization
* Remove leftover dead code from the previous commit
* Fix up an enormous number of PR nits
* Don't layer a profile if the json doesn't have a GUID
* Fix a test I unfixed
* get rid of extraneous bois{};
* Piles of PR feedback
* Collection of PR nits
* PR nits
* Fix a typo; Update the defaults to match #2378
* Tiny nits
* In-den-taition!
* Some typos, PR nits
* Fix this broken defaults case
* Apply suggestions from code review
Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>
* PR nits
* Warn the user when their settings are bad
The start of work on #1348
* Display an error dialog for errors during validation
* Polish for PR
* Add a ton of tests
* Polish the _GetMessageText bits
* Add code to check for duplicate profiles
* Verify that many warnings work at the same time
* comments y'all
* Apply fixes for dustin's thoughts from PR
* Add a proper exception type, use an array instead of a map
* PR Fixes
* Fix x86 build break
* Add a bit on "using the defaults" when we encountering an exception
* remove a redundant variable
* guid->GUID
* Address Michael's PR comments
* Clean up this error text, and catch exceptions better
* Update src/cascadia/TerminalApp/Resources/en-US/Resources.resw
### User Stories:
1. A user wants to be able to use the executable path as their starting title
- Does anyone want this?
2. A user wants to be able to set a custom starting title, but have that title be overridable
3. A user wants to be able to set an overridable starting title, different from the profile name
- Presumably someone will want this
4. A user totally wants to ignore the VT title and use something else
- This will make more sense in the post [#1320] "Support runtime variables in the custom user title" settings
### Solutions:
1. `name`, `startingTitle`, `tabTitle`
* a. `name` is only ever used as the profile name.
* b. If `startingTitle` isn't set, then the executable path is used
* c. If `startingTitle` is set, it's used as the initial title
* d. If `tabTitle` is set, it overrides the title from the terminal
* e. Current users of `tabTitle` need to manually update to the new behavior.
2. `name` as starting title, `tabTitle` as a different starting title
* a. `name` is used as the starting title and the profile name in the dropdown
* b. If `tabTitle` is set, we'll use that as the overridable starting title instead.
* c. In the future, `dynamicTabTitle` or `tabTitleOverride` could be added to support [#1320]
* d. Current users of `tabTitle` automatically get the new (different!) behavior.
* e. User Story 1 is impossible
- Does anyone want the behavior _ever_? Perhaps making that scenario impossible is good?
3. `name` unchanged, `tabTitle` as the starting title
* a. `name` is only ever used as the profile name.
* b. If `tabTitle` is set, we'll use that as the overridable starting title.
* c. In the future, `dynamicTabTitle` or `tabTitleOverride` could be added to support [#1320]
* d. Current users of `tabTitle` automatically get the new (different!) behavior.
4. `name` as starting title, `tabTitle` as different starting title, `suppressApplicationTitle` Boolean to force it to override
* a. `name`, `tabTitle` work as in Solution 2.
* b. When someone wants to be able to statically totally override that title (story 4), they can use `suppressApplicationTitle`
* c. `suppressApplicationTitle` name is WIP
* d. We'll add `suppressApplicationTitle` when someone complains
* e. If you really want story 1, use `tabTitle: c:\path\to\foo.exe` and `suppressApplicationTitle`.
[#1320]: https://github.com/microsoft/terminal/issues/1320
We've decided to pursue path 4.
First, I tried reusing the existing ExpandEnvironmentVariableStrings()
helper in TerminalApp/CascadiaSettings.cpp, but then I realized that
WIL already provides its own wrapper for ExpandEnvironmentStrings(),
so instead I deleted ExpandEnvironmentVariableStrings() and replaced
its usages with wil::ExpandEnvironmentStringsW().
I then used wil::ExpandEnvironmentStringsW() when resolving the
icon path as well. In addition, to allow empty strings,
I made changes to treat empty strings for "icon" the same
as JSON `null` or not setting the property at all.
Co-Authored-By: Michael Niksa <miniksa@microsoft.com>
* Connection: run all pseudoconsole hosts in jobs
This commit also switches the manual resource management in
ConhostConnection to use WIL, and modernizes the constructor to follow
new code style guidelines.
* Terminate conhost before trying to run down the pipes
* Code to add WSLProfiles.
* Updates recomended by miniksa
* Corrections from Mauve
* More updates from miniska (clarified WaitForSingleObject errors, and moved the try block to the calling function)
* Added THROW_LAST_ERROR for WAIT_FAILED instead of passing an unhandled exception.
* Migrate STL dependancies to LibraryIncludes.h
* Renamed function to provide more clarity
* Set WSL starting directory.
* Default Linux icon and brackets on new lines.
* Added system path so we don't rely on execution from the PATH environment variable. Removed incorrect error useage. Removed variable that was not required.
* Remove default directory setting.
…. It will return a replacement character at that point if it was given bad data. #788
<!-- 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 modifies the parser used while inserting text into the underlying data buffer to never return an empty sequence. The empty sequence is invalid as you can't insert a "nothing" into the buffer. The buffer asserted this with a fail fast crash. Now we will instead insert U+FFFD (the Unicode replacement character) � to symbolize that something was invalid and has been replaced.
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#788 and internal MSFT: 20990158
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/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: #788
<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
The solution here isn't perfect and isn't going to solve all of our problems. I was basically trying to stop the crash while not getting in the way of the other things coming down the pipe for the input channels.
I considered the following:
1. Remove the fail fast assertion from the buffer
- I didn't want to do this because it really is invalid to get all the way to placing the text down into the buffer and then request a string of 0 length get inserted. I feel the fail fast is a good indication that something is terribly wrong elsewhere that should be corrected.
2. Update the UTF16 parser in order to stop returning empty strings
- This is what I ultimately did. If it would ever return just a lead, it returns �. If it would ever return just a trail, it returns �. Otherwise it will return them as a pair if they're both there, or it will return a single valid codepoint. I am now assuming that if the parse function is being called in an Output Iterator and doesn't contain a string with all pieces of the data that are needed, that someone at a higher level messed up the data, it is in valid, and it should be repaired into replacements.
- This then will move the philosophy up out of the buffer layer to make folks inserting into the buffer identify half a sequence (if they're sitting on a stream where this circumstance could happen... one `wchar_t` at a time) and hold onto it until the next bit arrives. This is because there can be many different routes into the buffer from many different streams/channels. So buffering it low, right near the insertion point, is bad as it might pair loose `wchar_t` across stream entrypoints.
3. Update the iterator, on creating views, to disallow/transform empty strings.
- I considered this solution as well, but it would have required, under some circumstances, a second parsing of the string to identify lead/trail status from outside the `Utf16Parser` class to realize when to use the � character. So I avoided the double-parse.
4. Change the cooked read classes to identify that they pulled the lead `wchar_t` from a sequence then try to pull another one.
- I was going to attempt this, but @adiviness said that he tried it and it made all sorts of other weirdness happen with the edit line.
- Additionally, @adiviness has an outstanding series of effort to make cooked read significantly less horrible and disgusting. I didn't want to get in the way here.
5. Change the `GetChar` method off of the input buffer queue to return a `char32_t`, a `wstring_view`, transform a standalone lead/trail, etc.
- The `GetChar` method is used by several different accessors and API calls to retrieve information off of the input queue, transforming the Key events into straight up characters. To change this at that level would change them all. Long-term, it is probably warranted to do so as all of those consumers likely need to become aware of handling UTF-16 surrogates before we can declare victory. But two problems.
1. This gets in the way of @adiviness work on cooked read data
2. This goes WAY beyond the scope of what I want to accomplish here as the immediate goal is to stop the crash, not fix the world.
I've validated this by:
1. Writing some additional tests against the Utf16Parser to simulate some of the theoretical sequences that could arrive and need to be corrected into replacement characters per a verbal discussion and whiteboarding with @adiviness.
2. Manually triggered the emoji panel and inserted a bunch of emoji. Then seeked around left and right, deleted assorted points with the backspace key, pressed enter to commit, and used the up-arrow history to recommit them to see what happened. There were no crashes. The behavior is still weird and not great... but outside the scope of no crashy crashy.
* Removed using namespace directive from header files and put these in cpp files where they are used
* Fixed tabbing issues by replacing them with spaces.
Also regrouped the using directives.
* Update src/host/exemain.cpp
Co-Authored-By: Mike Griese <migrie@microsoft.com>
* Update src/interactivity/win32/find.cpp
Co-Authored-By: Mike Griese <migrie@microsoft.com>
* Apply [[nodiscard]] to functions returning error codes
- applied [[nodiscard]] for all HRESULT, LRESULT, and NTSTATUS functions
- fixed IntelliSense declaration complaints leading to function not
implemented warnings
- deleted declared but never implemented functions
- fixed unused parameter warnings
How verified:
- bcz dbg
- opencon
- testcon
- VS2019 debug build
* - use LOG_IF_FAILED where applicable
- remove use of goto
- make MakeAltRasterFont return void
* - add missing [[nodiscard]]
- remove vestigal function declarations
- fix inconsistent function declaration
* set identifying environment variable for new connections
Set a new 'WT_SESSION' environment variable when creating new terminal
connections to allow shells to detect a unique Windows Terminal session.
The value of the variable is a stringified GUID as returned by
CoCreateGuid.
How verified:
- "razzle" & vs debug build
- runut
- manual inspection
* * use winrt::guid type for connection guid
* use Utils::GuidToString for guid stringification
* expose guid parameter in ITerminalConnection idl
* - poke guid through ITerminalConnection
- misc. review fixes
- throw if CreateConPty fails in ConhostConnection::Start
- apply [[nodiscard]] and noexcept in various places
* - simplify environment variable extraction in UpdateEnvironmentMapW
* - use Utils::CreateGuid instead of CoCreateGuid in ConHostConnection()
* fix for historySize=32767 hang (except for historySize=0 case); tests still in progress
* tests run and almost pass - failure is a real bug in my change
* fixed bug that caused tests to fail, but it seems another bug causes the app to crash with a zero row count
* fix the additional bug (at a higher layer) mentioned in previous commit description
* Fix chk build assertion failures in new tests
It seems C++/WinRT doesn't like it when you implement a Windows Runtime
interface but then create instances of the implementing class
with function-call lifetime (aka stack allocation). That makes sense
given that WinRT objects are COM objects, but in my defense I was following
this example where they are just fine instantiating the `App` object
on the stack:
https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/author-apis#if-youre-not-authoring-a-runtime-class
* tabs to spaces
* CR feedback
* fix minor CR feedback (incorrect test log message)
