This commit reduces the code surface that interacts with raw JSON data,
reducing code complexity and improving maintainability.
Files that needed to be changed drastically were additionally
cleaned up to remove any code cruft that has accrued over time.
In order to facility this the following changes were made:
* Move JSON handling from `CascadiaSettings` into `SettingsLoader`
This allows us to use STL containers for data model instances.
For instance profiles are now added to a hashmap for O(1) lookup.
* JSON parsing within `SettingsLoader` doesn't differentiate between user,
inbox and fragment JSON data, reducing code complexity and size.
It also centralizes common concerns, like profile deduplication and
ensuring that all profiles are assigned a GUID.
* Direct JSON modification, like the insertion of dynamic profiles into
settings.json were removed. This vastly reduces code complexity,
but unfortunately removes support for comments in JSON on first start.
* `ColorScheme`s cannot be layered. As such its `LayerJson` API was replaced
with `FromJson`, allowing us to remove JSON-based color scheme validation.
* `Profile`s used to test their wish to layer using `ShouldBeLayered`, which
was replaced with a GUID-based hashmap lookup on previously parsed profiles.
Further changes were made as improvements upon the previous changes:
* Compact the JSON files embedded binary, saving 28kB
* Prevent double-initialization of the color table in `ColorScheme`
* Making `til::color` getters `constexpr`, allow better optimizations
The result is a reduction of:
* 48kB binary size for the Settings.Model.dll
* 5-10% startup duration
* 26% code for the `CascadiaSettings` class
* 1% overall code in this project
Furthermore this results in the following breaking changes:
* The long deprecated "globals" settings object will not be detected and no
warning will be created during load.
* The initial creation of a new settings.json will not produce helpful comments.
Both cases are caused by the removal of manual JSON handling and the
move to representing the settings file with model objects instead
## PR Checklist
* [x] Closes#5276
* [x] Closes#7421
* [x] I work here
* [x] Tests added/passed
## Validation Steps Performed
* Out-of-box-experience is identical to before ✔️
(Except for the settings.json file lacking comments.)
* Existing user settings load correctly ✔️
* New WSL instances are added to user settings ✔️
* New fragments are added to user settings ✔️
* All profiles are assigned GUIDs ✔️
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Adds directional modifiers for SplitState and convert those to the appropriate horizontal/vertical when splitting a pane.
<!-- 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#4340
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [x] 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
"vertical" and "horizontal" splits were removed from `defaults.json`, but code was added to parse those as `right` and `down` respectively. It is also the case that if a user has a custom hotkey for `split: vertical` it will override the default for `split: right`.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Split the pane using each of the new directional movements
This commit partially reverts d465a47 and introduces an alternative approach by adding Hash and Equals methods to the KeyChords class. Those methods will now favor any existing Vkeys over ScanCodes.
## PR Checklist
* [x] Closes#10933
* [x] I work here
* [x] Tests added/passed
## Validation Steps Performed
* Added a new test, which is ✔️
* Various standard commands still work ✔️
* Hash() returns the same value for all KeyChords that are Equals() ✔️
The quake mode keybinding is bound to a scancode. This made it
impossible to override it with a vkey-based one like "win+\`".
This commit fixes the issue by making sure that a `KeyChord` always has a vkey,
and leveraging this fact inside ActionMap, which now ignores the scan-code.
## PR Checklist
* [x] Closes#10875
* [x] I work here
* [x] Tests added/passed
## Validation Steps Performed
* quake mode and other keybinding still work ✔️
* Repro settings from #10875 work correctly ✔️
## Summary of the Pull Request
This isn't a fix for #10875, but it is logging that help identify the root cause here. The logging may additionally be helpful for some of the other issues we're seeing elsewhere in the repo, namely #10340.
@lhecker is actually working on the fix for #10875, so hopefully this test will help validate.
## References
* Regressed in #10666.
* logging for #8888
## PR Checklist
* [x] Closes nothing
* [x] I work here
* [x] Tests added, and they absolutely fail, but they're localtests, so ¯\\\_(ツ)_/¯
* [n/a] Requires documentation to be updated
## details
While I was here, I noticed that `KeyBindingsTests::KeyChords` has been broken for some time now. So I fixed that too.
<!-- 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 implements/solves #7125. Concretely: two requests regarding alt+space were posted there:
1. Disabling the alt+space menu when the keychord explicitly unbound - and forwarding the keystroke to the terminal
2. Disabling the alt+space menu when the keychord is bound to an action
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References
Not that I know
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes#7125
* [x] CLA signed.
* [x] Tests added/passed
* [x] Documentation updated. N/A
* [x] 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.
The issue was marked Help-Wanted. I am happy to change the implementation to better fit your (planned) architecture.
<!-- 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
While researching the solution, I noticed that the XAML system was always opening the system menu after Alt+Space, even when explicitly setting the event to be handled according to the documentation. The only solution I could find was to hook into the "XAML bypass" already in place for F7 KeyDown, and Alt KeyUp keystrokes. This bypass sends the keystroke to the AppHost immediately. This bypass method will "fall back" to the normal XAML routing when the keystroke is not handled.
The implemented behaviour is as follows:
- Default: same as normal; system menu is working since the bypass does not handle the keystroke
- Alt+Space explicitly unbound: bypass passes the keystroke to the terminal and marks it as handled
- Alt+Space bound to command: bypass invokes the command and marks it as handled
Concretely, added a method to the KeyBindings and ActionMap interfaces to check whether a keychord is explicitly unbound. The implementation for `_GetActionByKeyChordInternal` already distinguishes between explicitly unbound and lack of binding, however this distinction is not carried over to the public methods. I decided not to change this existing method, to avoid breaking other stuff and to make the API more explicit.
Furthermore, there were some checks against Alt+Space further down in the code, preventing this keystroke from being entered in the terminal. Since the check for this keystroke is now done at a "higher" level, I thought I could safely remove these checks as otherwise the keystroke could never be sent to the terminal itself. Please correct me if I'm wrong.
Note that when alt+space is bound to an action that opens the command pallette (such as tab search), then a second press of the key combination does still open the system menu. This is because at that point, the "bypass" is cancelled (called "not a good implementation" in #4031). I don't think this can easily be solved for now, but this is a very minor bug/inconvenience.
<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Added tests for the new method. Performed manual checking:
* [x] Default configuration still opens system menu like normal
* [x] Binding alt+space to an action performs the action and does not show the system menu
* [x] Explicitly unbinding alt+space no longer shows the system menu and sends the keystroke to the terminal. I was unable to run the debug tap (it crashed my instance - same thing happening on preview and release builds) to check for sure, but behaviour was identical to native linux terminals.
This commit introduces an alternative to specifying key bindings as a combination of key modifiers and a character. It allows you to specify an explicit virtual key as `vk(nnn)`.
Additionally this commit makes it possible to bind actions to scan codes. As scan code 41 appears to be the button below the Escape key on virtually all keyboards, we'll be able to bind the quake mode hotkey to `win+sc(41)` and have it work consistently across most if not all keyboard layouts.
## PR Checklist
* [x] Closes#7539, Closes#10203
* [x] I work here
* [x] Tests added/passed
## Validation Steps Performed
The following was tested both on US and DE keyboard layouts:
* Ctrl+, opens settings ✔️
* Win+` opens quake mode window ✔️
* Ctrl+plus/minus increase/decrease font size ✔️
Replaces `KeyModifiers` with the pretty much equivalent
`VirtualKeyModifiers` enum in winrt.
After doing this I noticed #10593 which changes the KeyChords a lot, but
it seems these PRs are still compatible
The issue also mentions replacing Vkey with
`Windows::System::VirtualKey`, but I chose not to because that enum only
includes a subset of the keys terminal supports here (no VK_OEM_* keys)
## Validation Steps Performed
Changed key bind in config, and confirmed it still works after
restarting terminal
Closes#877
## Summary of the Pull Request
#10297 found a bug in `ActionMap` where the `ToggleCommandPalette` key chord could not be found using `GetKeyBindingForAction`.
This was caused by the following:
- `AddAction`: when adding the action, the `ActionAndArgs` is basically added as `{ToggleCommandPalette, ToggleCommandLineArgs{}}` (Note the default ctor used for the action args)
- `GetKeyBindingForAction`: we're searching for an `ActionAndArgs` structured as `{ToggleCommandPalette, nullptr}`
- Since these are _technically_ two different actions, we are unable to find it.
This issue was fixed by making the `Hash(ActionAndArgs)` function smarter! If the `ActionAndArgs` has no args, but the `ShortcutAction` _supports_ args, generate the args using the default ctor.
By making `Hash()` smarter, everybody benefits from this logic! We can basically now enforce that `ActionAndArgs{ <X>, nullptr } == ActionAndArgs{ <X>, <default_ctor> }`.
## Validation Steps Performed
- Added a test.
- Tested this on #10297's branch and this does fix the bug
This entirely removes `KeyMapping` from the settings model, and builds on the work done in #9543 to consolidate all actions (key bindings and commands) into a unified data structure (`ActionMap`).
## References
#9428 - Spec
#6900 - Actions page
Closes#7441
## Detailed Description of the Pull Request / Additional comments
The important thing here is to remember that we're shifting our philosophy of how to interact/represent actions. Prior to this, the actions arrays in the JSON would be deserialized twice: once for key bindings, and again for commands. By thinking of every entry in the relevant JSON as a `Command`, we can remove a lot of the context switching between working with a key binding vs a command palette item.
#9543 allows us to make that shift. Given the work in that PR, we can now deserialize all of the relevant information from each JSON action item. This allows us to simplify `ActionMap::FromJson` to simply iterate over each JSON action item, deserialize it, and add it to our `ActionMap`.
Internally, our `ActionMap` operates as discussed in #9428 by maintaining a `_KeyMap` that points to an action ID, and using that action ID to retrieve the `Command` from the `_ActionMap`. Adding actions to the `ActionMap` automatically accounts for name/key-chord collisions. A `NameMap` can be constructed when requested; this is for the Command Palette.
Querying the `ActionMap` is fairly straightforward. Helper functions were needed to be able to distinguish an explicit unbinding vs the command not being found in the current layer. Internally, we store explicitly unbound names/key-chords as `ShortcutAction::Invalid` commands. However, we return `nullptr` when a query points to an unbound command. This is done to hide this complexity away from any caller.
The command palette still needs special handling for nested and iterable commands. Thankfully, the expansion of iterable commands is performed on an `IMapView`, so we can just expose `NameMap` as a consolidation of `ActionMap`'s `NameMap` with its parents. The same can be said for exposing key chords in nested commands.
## Validation Steps Performed
All local tests pass.
**BE NOT AFRAID**. I know that there's 107 files in this PR, but almost
all of it is just find/replacing `TerminalControl` with `Control`.
This is the start of the work to move TermControl into multiple pieces,
for #5000. The PR starts this work by:
* Splits `TerminalControl` into separate lib and dll projects. We'll
want control tests in the future, and for that, we'll need a lib.
* Moves `ICoreSettings` back into the `Microsoft.Terminal.Core`
namespace. We'll have other types in there soon too.
* I could not tell you why this works suddenly. New VS versions? New
cppwinrt version? Maybe we're just better at dealing with mdmerge
bugs these days.
* RENAMES `Microsoft.Terminal.TerminalControl` to
`Microsoft.Terminal.Control`. This touches pretty much every file in
the sln. Sorry about that (not sorry).
An upcoming PR will move much of the logic in TermControl into a new
`ControlCore` class that we'll add in `Microsoft.Terminal.Core`.
`ControlCore` will then be unittest-able in the
`UnitTests_TerminalCore`, which will help prevent regressions like #9455
## Detailed Description of the Pull Request / Additional comments
You're really gonna want to clean the sln first, then merge this into
your branch, then rebuild. It's very likely that old winmds will get
left behind. If you see something like
```
Error MDM2007 Cannot create type
Microsoft.Terminal.TerminalControl.KeyModifiers in read-only metadata
file Microsoft.Terminal.TerminalControl.
```
then that's what happened to you.
## Summary of the Pull Request
Introduces a new command called `moveTab`
This command has a single mandatory argument with values of `forward` and `backward`
## PR Checklist
* [x] Closes https://github.com/microsoft/terminal/issues/3593
* [x] CLA signed.
* [x] Tests added/passed
* [x] Documentation updated here: https://github.com/MicrosoftDocs/terminal/pull/198
* [x] Schema updated
* [x] I've discussed this with core contributors already.
## Detailed Description of the Pull Request / Additional comments
Went for the straightforward solution of moving the tab and the tabViewItem.
## Validation Steps Performed
* Manual testing
* Add a tabColor parameter to the `new-tab` and `split-panes` command
* Add --tabColor to the command line, to allow bootstrapping with tabs
of different colors
Add another field to NewTerminalArgs. Use this field to set
StartingTabColor in Terminal. This color gets overridden by the color
defined by the profile / VT, however can be overridden with the color
picker.
Since the color is the property of the Terminal, when defined for the
tab this color is associated only with the first pane/terminal of the
tab. Additional panes will not inherit this color (to prevent advanced
resolution, where we need to resolve between the inherited color and the
one specified for the pane).
## Validation Steps Performed
* UT for parameters parsing
* Running system with several tabs of different colors.
* Adding custom actions with colors
* Performing operations like split pane, duplicate and so on
Closes#8075
The JsonUtils changes in #8018 revealed that we need more robust,
configurable optional handling. We learned that there's a class of
values that was previously underrepresented in our API: _strings that
have an explicit empty value_.
The Settings model supports starting directory, icon, background image
et al values that are empty. That emptiness _overrides_ a value set in a
lower layer, so it is not sufficient to represent the empty value for
any one of those fields as an unset optional.
There are a couple other settings for which we've implemented a
hand-rolled option type (for roughly the same reason): foreground,
background, any color fields that override values from the color scheme
_or_ the lower layer profile.
These requirements are best fulfilled by better optional support in
JsonUtils. Where the library would originally detect known types of
optional and pre-filter them out during `GetValue` and `SetValue`, it
will now defer to another conversion trait.
This commit introduces a helper conversion trait and an "option oracle".
The conversion trait will use the option oracle to detect emptiness,
generate empty option values, and read values out of option types. In so
doing, the trait is insulated from the implementation details of any
specific option type.
Any special logic for handling JSON null and option types has been
stripped from GetValue. Due to this, there is an express change in
behavior for some converters:
* `GetValue<T>(jsonNull)` where `T` is **not** an option type[1] has
been upgraded from a silent no-op to an exception.
Further, I took the opportunity to replace NullableSetting with
std::optional<std::optional<T>>, which accurately represents "setting
that the user might explicitly clear". I've added a test to
JsonUtilsTests to make sure it can serialize/deserialize double
optionals the way we expect it to.
Tests (Local, Unit for TerminalApp/SettingsModel):
Summary: Total=140, Passed=140, Failed=0, Blocked=0, Not Run=0, Skipped=0
[1]: Explicitly, if `T` is not an option type _and the converter does
not support null_.
- The number of lines to move upon scroll up scroll down can be defined
in ScrollUp and ScrollDown commands (parameter is called
"rowsToScroll").
- If the number are not provided, use the system default (the one we are
using for mouse scrolls), rather than 1 line.
## Validation Steps Performed
* Manual testing
* Added custom bindings for scroll commands with different values,
verified they and the default appear and behave as expected
* Checked that invalid values are not allowed
Closes#5078
Introduces a new TerminalSettingsModel (TSM) project. This project is
responsible for (de)serializing and exposing Windows Terminal's settings
as WinRT objects.
## References
#885: TSM epic
#1564: Settings UI is dependent on this for data binding and settings access
#6904: TSM Spec
In the process of ripping out TSM from TerminalApp, a few other changes
were made to make this possible:
1. AppLogic's `ApplicationDisplayName` and `ApplicationVersion` was
moved to `CascadiaSettings`
- These are defined as static functions. They also no longer check if
`AppLogic::Current()` is nullptr.
2. `enum LaunchMode` was moved from TerminalApp to TSM
3. `AzureConnectionType` and `TelnetConnectionType` were moved from the
profile generators to their respective TerminalConnections
4. CascadiaSettings' `SettingsPath` and `DefaultSettingsPath` are
exposed as `hstring` instead of `std::filesystem::path`
5. `Command::ExpandCommands()` was exposed via the IDL
- This required some of the warnings to be saved to an `IVector`
instead of `std::vector`, among some other small changes.
6. The localization resources had to be split into two halves.
- Resource file linked in init.cpp. Verified at runtime thanks to the
StaticResourceLoader.
7. Added constructors to some `ActionArgs`
8. Utils.h/cpp were moved to `cascadia/inc`. `JsonKey()` was moved to
`JsonUtils`. Both TermApp and TSM need access to Utils.h/cpp.
A large amount of work includes moving to the new namespace
(`TerminalApp` --> `Microsoft::Terminal::Settings::Model`).
Fixing the tests had its own complications. Testing required us to split
up TSM into a DLL and LIB, similar to TermApp. Discussion on creating a
non-local test variant can be found in #7743.
Closes#885
2020-10-06 09:56:59 -07:00
Renamed from src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp (Browse further)
