Add support for new panes with specifc profiles and other settings overrides #3825

zadjii-msft · 2019-12-03T19:37:58Z

Summary of the Pull Request

This enables the user to set a number of extra settings in the NewTab and SplitPane ShortcutActions, that enable customizing how a new terminal is created at runtime. The following four properties were added:

profile
commandline
tabTitle
startingDirectory

profile can be used with either a GUID or the name of a profile, and the action will launch that profile instead of the default.

commandline, tabTitle, and startingDirectory can all be used to override the profile's values of those settings. This will be more useful for #607.

With this PR, you can make bindings like the following:

{ "keys": ["ctrl+a"], "command": { "action": "splitPane", "split": "vertical" } },
{ "keys": ["ctrl+b"], "command": { "action": "splitPane", "split": "vertical", "profile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" } },
{ "keys": ["ctrl+c"], "command": { "action": "splitPane", "split": "vertical", "profile": "profile1" } },
{ "keys": ["ctrl+d"], "command": { "action": "splitPane", "split": "vertical", "profile": "profile2" } },
{ "keys": ["ctrl+e"], "command": { "action": "splitPane", "split": "horizontal", "commandline": "foo.exe" } },
{ "keys": ["ctrl+f"], "command": { "action": "splitPane", "split": "horizontal", "profile": "profile1", "commandline": "foo.exe" } },
{ "keys": ["ctrl+g"], "command": { "action": "newTab" } },
{ "keys": ["ctrl+h"], "command": { "action": "newTab", "startingDirectory": "c:\\foo" } },
{ "keys": ["ctrl+i"], "command": { "action": "newTab", "profile": "profile2", "startingDirectory": "c:\\foo" } },
{ "keys": ["ctrl+j"], "command": { "action": "newTab", "tabTitle": "bar" } },
{ "keys": ["ctrl+k"], "command": { "action": "newTab", "profile": "profile2", "tabTitle": "bar" } },
{ "keys": ["ctrl+l"], "command": { "action": "newTab", "profile": "profile1", "tabTitle": "bar", "startingDirectory": "c:\\foo", "commandline":"foo.exe" } }

References

This is a lot of work that was largely started in pursuit of #607. We want people to be able to override these properties straight from the commandline. While they may not make as much sense as keybindings like this, they'll make more sense as commandline arguments.

PR Checklist

Closes Open panes with a specific profile #998
I work here
Tests added/passed
Requires documentation to be updated

Validation Steps Performed

There are tests 🎉

Manually added some bindings, they opened the correct profiles in panes/tabs

…erride a profile in a newTab command

…ther vertically or horizontally

…ane-action # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.cpp # src/cascadia/TerminalApp/AppKeyBindings.h # src/cascadia/TerminalApp/AppKeyBindings.idl # src/cascadia/TerminalApp/TerminalPage.cpp

…rminal-args

…rminal-args # Conflicts: # src/cascadia/TerminalApp/ActionArgs.idl

Move all the profile determination stuff into CascadiaSettings. Now it's in charge of building the right TerminalSettings based on the index and NewTerminalArgs passed to it. Then, also hook that up to SplitPane. This weirdly makes a new tab _and_ a new pane, which is wrong. Dunno why yet. Ill debug mondeay

…rminal-args # Conflicts: # doc/cascadia/profiles.schema.json # src/cascadia/TerminalApp/ActionArgs.h # src/cascadia/TerminalApp/ActionArgs.idl # src/cascadia/TerminalApp/AppActionHandlers.cpp # src/cascadia/TerminalApp/TerminalPage.cpp # src/cascadia/TerminalApp/TerminalPage.h

(cherry picked from commit b392ba0)

…erminal-args # Conflicts: # src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp # src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp # src/cascadia/LocalTests_TerminalApp/TabTests.cpp # src/cascadia/LocalTests_TerminalApp/TerminalApp.LocalTests.AppxManifest.prototype.xml

…rminal-args # Conflicts: # src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

carlos-zamora · 2019-12-06T00:34:16Z

Can we also have support for profileIndex? That way we can include something like the following in defaults.json

{ "keys": ["ctrl+alt+1"], "command": { "action": "splitPane", "split": "vertical", "profileIndex": 0 } },
{ "keys": ["ctrl+alt+2"], "command": { "action": "splitPane", "split": "vertical", "profileIndex": 1 } },
{ "keys": ["ctrl+alt+3"], "command": { "action": "splitPane", "split": "vertical", "profileIndex": 2 } },

{ "keys": ["ctrl+alt+shift+1"], "command": { "action": "splitPane", "split": "horizontal", "profileIndex": 0 } },
{ "keys": ["ctrl+alt+shift+2"], "command": { "action": "splitPane", "split": "horizontal", "profileIndex": 1 } },
{ "keys": ["ctrl+alt+shift+3"], "command": { "action": "splitPane", "split": "horizontal", "profileIndex": 2 } },

Or should this be a separate issue?

zadjii-msft · 2019-12-06T17:14:47Z

@carlos-zamora I actually secretly snuck that in with the refactor to have ProfileIndex be part of NewTerminalArgs. index should just work for splitPane now too :)

# Conflicts: # src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/CascadiaSettings.cpp

DHowett-MSFT · 2019-12-06T20:40:36Z

src/cascadia/TerminalApp/TerminalPage.cpp

                    auto actionAndArgs = winrt::make_self<winrt::TerminalApp::implementation::ActionAndArgs>();
                    actionAndArgs->Action(ShortcutAction::NewTab);
                    auto newTabArgs = winrt::make_self<winrt::TerminalApp::implementation::NewTabArgs>();
-                    newTabArgs->ProfileIndex(profileIndex);
+                    auto newTerminalArgs = winrt::make_self<winrt::TerminalApp::implementation::NewTerminalArgs>();
+                    newTerminalArgs->ProfileIndex(profileIndex);
+                    newTabArgs->TerminalArgs(*newTerminalArgs);
                    actionAndArgs->Args(*newTabArgs);
                    profileKeyChord = keyBindings.GetKeyBindingForActionWithArgs(*actionAndArgs);


This is super expensive, we'll want to maybe not have it be so expensive later.

(One can file a task for this)

I thought make_self was the non-expensive version that just makes a com_ptr for the class, but didn't do the binary boundary hopping

no no it is, but it's still a heap alloc of like four objects for what amounts to a hash table lookup 😄

…rminal-args # Conflicts: # src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

long BYND

ghost · 2019-12-09T13:02:29Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

## Summary of the Pull Request I accidentally did the wrong check here to see if the value exists. For an `IReference`, you need to do `variable != nullptr`. I did `variable.Value()`. ## References Introduced in #3825 ## PR Checklist * [x] Closes #3897 * [x] I work here * [ ] Tests added/passed - wow this was a lot harder than I expected * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments This includes a maybe unrelated fix to make `TerminalPage`'s `ShortcutActionDispatch` a `com_ptr`. While I was messing with the tests for this, I caught that we're not supposed to direct allocate winrt types like that. Ofc, the `TerminalAppLib` project doesn't catch this. ## Validation Steps Performed Ran the terminal manually, instead of just running the tests

## Summary of the Pull Request On this month's episode of "Mike accidentally breaks this": Mike forgets that `==` is defined on a pair of winrt objects as "do these point to the _SAME_ object", not "do they have the same values". This just slipped right through code review. ## References Broken in #3825 ## PR Checklist * [x] Closes #3896 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed Manually checked that these are still there

ghost · 2020-01-14T17:16:22Z

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

runerei

👍

zadjii-msft added 24 commits November 26, 2019 15:51

Basic POC - you can add profile, commandline, startingDirectory to ov…

6752b1a

…erride a profile in a newTab command

Add a 'splitPane' keybinding that can be used for splitting a pane ei…

9474240

…ther vertically or horizontally

Update documentation too

fb93105

Good lord this is important

451aaab

Add a test too, though I have no idea if it works

31a9046

"style" -> "split"

b5a264b

pr comments from carlos

e0cd014

Merge remote-tracking branch 'origin/master' into dev/migrie/f/splitP…

de60a30

…ane-action # Conflicts: # src/cascadia/TerminalApp/AppKeyBindings.cpp # src/cascadia/TerminalApp/AppKeyBindings.h # src/cascadia/TerminalApp/AppKeyBindings.idl # src/cascadia/TerminalApp/TerminalPage.cpp

Merge remote-tracking branch 'origin/master' into dev/migrie/f/new-te…

9f93a53

…rminal-args

Merge branch 'dev/migrie/f/splitPane-action' into dev/migrie/f/new-te…

9eac8a6

…rminal-args # Conflicts: # src/cascadia/TerminalApp/ActionArgs.idl

clean up some chaotically bad code writing

49a546e

Add title to the terminal args too

22f76e4

Try adding a test - remember that everything is a horrifying nightmare

65f10e9

I this restores the functionality of the local winrt objects, not XAML

b392ba0

I this restores the functionality of the local winrt objects, not XAML

3be37fd

(cherry picked from commit b392ba0)

Fix the unit tests, add comments

8a0bb8d

That's what I get for not building before pushing

1760de2

write a big honkin test

93db1ed

Comments are important

2073869

update the schema too

9226042

Merge remote-tracking branch 'origin/master' into dev/migrie/f/new-te…

919f626

…rminal-args # Conflicts: # src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Dec 3, 2019

zadjii-msft requested review from carlos-zamora, DHowett-MSFT, miniksa and leonMSFT December 3, 2019 19:37

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 5, 2019

Suggestions from Dustin's review

484e444

zadjii-msft added 2 commits December 6, 2019 11:52

Merge branch 'master' into dev/migrie/f/new-terminal-args

6e02caa

# Conflicts: # src/cascadia/TerminalApp/TerminalPage.cpp

update the schema too

73715e6

zadjii-msft requested a review from DHowett-MSFT December 6, 2019 17:55

DHowett-MSFT reviewed Dec 6, 2019

View reviewed changes

src/cascadia/TerminalApp/CascadiaSettings.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/CascadiaSettings.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/CascadiaSettings.cpp Outdated Show resolved Hide resolved

Some nits from Dustin

fcdd2cf

DHowett-MSFT approved these changes Dec 6, 2019

View reviewed changes

zadjii-msft added 2 commits December 6, 2019 15:05

Merge remote-tracking branch 'origin/master' into dev/migrie/f/new-te…

4a99051

…rminal-args # Conflicts: # src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

Don't take a trip to beeftown on negative parameters

b8eecb6

long BYND

carlos-zamora approved these changes Dec 6, 2019

View reviewed changes

DHowett-MSFT approved these changes Dec 6, 2019

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 9, 2019

ghost merged commit dd1c7b3 into master Dec 9, 2019

ghost deleted the dev/migrie/f/new-terminal-args branch December 9, 2019 13:02

DHowett-MSFT mentioned this pull request Dec 10, 2019

Dropdown menu no longer displays bound keys again (after #3825) #3896

Closed

This was referenced Dec 10, 2019

Fix a crash when opening a new tab with args #3901

Merged

Re-add the keychords to the new tab flyout #3903

Merged

mcpiroman mentioned this pull request Dec 11, 2019

Snap to character grid when resizing window #3181

Merged

5 tasks

zadjii-msft mentioned this pull request Dec 17, 2019

Come up with a better solution for reloading tabTitle #3989

Closed

ghost mentioned this pull request Jan 14, 2020

Open panes with a specific profile #998

Closed

4 tasks

zadjii-msft mentioned this pull request Jan 17, 2020

Scenario: Add support for panes #1000

Open

59 tasks

runerei reviewed Jan 22, 2020

View reviewed changes

zadjii-msft mentioned this pull request May 4, 2020

settings.json - where it is located? #5732

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for new panes with specifc profiles and other settings overrides #3825

Add support for new panes with specifc profiles and other settings overrides #3825

zadjii-msft commented Dec 3, 2019

carlos-zamora commented Dec 6, 2019

zadjii-msft commented Dec 6, 2019 •

edited

Loading

DHowett-MSFT Dec 6, 2019

DHowett-MSFT Dec 6, 2019

zadjii-msft Dec 6, 2019

DHowett-MSFT Dec 6, 2019

ghost commented Dec 9, 2019

ghost commented Jan 14, 2020

runerei left a comment

Add support for new panes with specifc profiles and other settings overrides #3825

Add support for new panes with specifc profiles and other settings overrides #3825

Conversation

zadjii-msft commented Dec 3, 2019

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

carlos-zamora commented Dec 6, 2019

zadjii-msft commented Dec 6, 2019 • edited Loading

DHowett-MSFT Dec 6, 2019

Choose a reason for hiding this comment

DHowett-MSFT Dec 6, 2019

Choose a reason for hiding this comment

zadjii-msft Dec 6, 2019

Choose a reason for hiding this comment

DHowett-MSFT Dec 6, 2019

Choose a reason for hiding this comment

ghost commented Dec 9, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jan 14, 2020

runerei left a comment

Choose a reason for hiding this comment

zadjii-msft commented Dec 6, 2019 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.