Add support for the `newWindow` action #9208

zadjii-msft · 2021-02-18T20:40:22Z

Finally implements the newWindow action. It does so by
ShellExecuteing wt.exe with commandline args corresponding to the
ones that would create the same NewTerminalArgs. This works with #8898
and #9118 to allow new windows (even with windowingBehavior: useExisting)

This is taken from my auto-elevate branch, hence the references to
elevation

References #5000
References projects/5
References #8898
References #9118
Closes #1051

…ompile a String[]

…then back!

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

…safely tear it down. It _seems_ like it.

….dll

…galactic-empire

…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.

zadjii-msft · 2021-02-19T12:51:16Z

(@ everyone - we're trying to get this in for 1.7 today, hence why you're all on the assigned-to line. Thanks ☺️)

carlos-zamora

Update schema
Update docs

Seems to be a lot of "Just moved things around. Can revert changes to this file."

Approving since we have a tight deadline, but please make sure you update the schema (or wait... maybe it's already updated?! idk, check haha).

carlos-zamora · 2021-02-19T18:30:54Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+        // else if (_ProfileIndex)
+        // {
+        //     ss << fmt::format(L"profile index: {}, ", _ProfileIndex.Value());
+        // }


So remove the dead code for index?

I think this is valuable to keep.

carlos-zamora · 2021-02-19T18:32:47Z

src/cascadia/TerminalSettingsModel/KeyMapping.idl

@@ -13,7 +13,6 @@ namespace Microsoft.Terminal.Settings.Model
        OpenNewTabDropdown,
        DuplicateTab,
        NewTab,
-        NewWindow,


Looks like you just moved around these enums. Could probably revert changes here.

FYI I intentionally moved them all to be at the end so that the order was more consistent, and reflective of the order these things were added in. NewWindow already existing in the list was maybe an oversight? I suppose I reserved the key long before it ever was used?

carlos-zamora · 2021-02-19T18:33:52Z

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

@@ -150,6 +150,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        { ShortcutAction::MoveTab, MoveTabArgs::FromJson },
        { ShortcutAction::ToggleCommandPalette, ToggleCommandPaletteArgs::FromJson },
        { ShortcutAction::FindMatch, FindMatchArgs::FromJson },
+        { ShortcutAction::NewWindow, NewWindowArgs::FromJson },


This line seems to be the only relevant change in this file. All of the other ones are just moving stuff around.

carlos-zamora · 2021-02-19T18:34:40Z

src/cascadia/TerminalApp/ShortcutActionDispatch.idl

@@ -53,5 +52,6 @@ namespace TerminalApp
        event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> BreakIntoDebugger;
        event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> FindMatch;
        event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> TogglePaneReadOnly;
+        event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> NewWindow;


Just moved stuff around. Could revert changes to this file

carlos-zamora · 2021-02-19T18:34:55Z

src/cascadia/TerminalApp/ShortcutActionDispatch.h

@@ -67,6 +66,7 @@ namespace winrt::TerminalApp::implementation
        TYPED_EVENT(BreakIntoDebugger,    TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
        TYPED_EVENT(FindMatch,            TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
        TYPED_EVENT(TogglePaneReadOnly,   TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
+        TYPED_EVENT(NewWindow,            TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);


Just moved things around. Can revert changes to this file.

carlos-zamora · 2021-02-19T18:35:14Z

src/cascadia/TerminalApp/ShortcutActionDispatch.cpp

@@ -266,6 +260,11 @@ namespace winrt::TerminalApp::implementation
            _TogglePaneReadOnlyHandlers(*this, eventArgs);
            break;
        }
+        case ShortcutAction::NewWindow:


Just moved things around. Can revert changes to this file.

…1051-newWindow

…Window

DHowett

The only thing I am blocking on is that header filled with bare functions. They have to be _TIL_INLINEPREFIX or put into a cpp that is compiled and linked into WinRTUtils, or made safe in some other way.

DHowett · 2021-02-19T22:21:55Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+        // else if (_ProfileIndex)
+        // {
+        //     ss << fmt::format(L"profile index: {}, ", _ProfileIndex.Value());
+        // }


I think this is valuable to keep.

DHowett · 2021-02-19T22:22:24Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+
+        if (!_Commandline.empty())
+        {
+            ss << fmt::format(L"-- \"{}\" ", _Commandline);


i think there's a better way to do this (using format_to), but i am not blocking on it

DHowett · 2021-02-19T22:23:41Z

src/cascadia/WinRTUtils/inc/WtExeUtils.h

+// - <none>
+// Return Value:
+// - true if we believe this extension is being run in the dev build package.
+bool IsDevBuild()


we can't do this. Including bare functions in a header is a way to get hurt during linking if two files in the same project #include it.

DHowett · 2021-02-19T22:24:31Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+        // This will get us the correct exe for dev/preview/release. If you
+        // don't stick this in a local, it'll get mangled by ShellExecute. I
+        // have no idea why.
+        const auto exePath{ GetWtExePath() };


technically only works for CascadiaPackage and not WTU

if we were doing this to the gills we would propagate the event out of AppLogic and to the win32 host so the win32 host could spawn another copy of itself

this is not a blocking comment!

DHowett · 2021-02-19T22:25:40Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+            newTerminalArgs = NewTerminalArgs();
+        }
+
+        auto [profileGuid, settings] = TerminalSettings::BuildSettings(_settings,


BIG HAMMER to get a guid from an args

(not blocking)

it's now _after_ 5pm on a friday so I'm not doing the rest

ghost · 2021-02-19T23:51:08Z

Hello @DHowett!

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.

ghost · 2021-03-01T18:04:18Z

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 30 commits December 16, 2020 07:59

Get all the projects created and hooked up to the sln

3bef7bb

Yank all the M/P files, this builds?!

1f52d35

Shockingly, this works, it works elevated, and it works unpackaged

5a9cdc8

This won't work, but I'm committing this becaus I finally got it to c…

36539cf

…ompile a String[]

whoop, we pass the commandline from the peasant, to the monarch, and …

27ace16

…then back!

HOLY SHIT I GOT THE COMANDLINE TO EXECUTE IN THE CURRENT WINDOW!!!!!!…

9a41647

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

add a note to future me

5cabcfb

This works as a unittest, but not a local test. That's batty

03bfc6e

this macro makes me feel dirty

590b9ff

LocalTests_Remoting -> UnitTests_Remoting

0579b24

finish renaming this test

a3faed6

Clean up a lot for review

9c6eac4

cleanup Peasant for review

0103331

Final cleanup for review

b4fe1bf

_final_ final cleanup for review

d08e65c

pr nits

e101efd

This seems to work to create a thread listening for the monarch, and …

e1402d8

…safely tear it down. It _seems_ like it.

it's hard to believe he's gone / were it so easy

f978a9c

Oh yea actually remove the thing

5939636

last commit before the holidays

c088895

Merge remote-tracking branch 'origin/main' into dev/migrie/f/remoting…

fa2df47

….dll

Enable audit mode

0f5c24f

nits

921d915

Merge branch 'dev/migrie/f/remoting.dll' into dev/migrie/f/the-first-…

00184e7

…galactic-empire

Ask the TerminalApp to parse the commandline, and tell us what the wi…

658db6b

…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.

Holy bajesus, this works like a charm

977db46

Add a note about commiting suicide

bcbef34

notes, comments, cleanup

813dbc6

activate windows

0f0df5e

add some tracelogging

9fc2f0e

spellbot

c62a86c

zadjii-msft assigned DHowett, carlos-zamora, miniksa and PankajBhojwani Feb 19, 2021

zadjii-msft mentioned this pull request Feb 19, 2021

Scenario: Windows Terminal 2.0 Process Model Improvements #5000

Closed

80 tasks

carlos-zamora approved these changes Feb 19, 2021

View reviewed changes

carlos-zamora removed their assignment Feb 19, 2021

zadjii-msft added 3 commits February 19, 2021 14:44

words r hard

fa96b5d

Merge branch 'dev/migrie/f/2227-windowingBehavior' into dev/migrie/f/…

eca3125

…1051-newWindow

Add to the schema too

14f027c

zadjii-msft mentioned this pull request Feb 19, 2021

Add docs for newWindow action MicrosoftDocs/terminal#259

Merged

ghost closed this Feb 19, 2021

ghost deleted the branch main February 19, 2021 21:09

DHowett reopened this Feb 19, 2021

DHowett changed the base branch from dev/migrie/f/2227-windowingBehavior to main February 19, 2021 21:13

Merge remote-tracking branch 'origin/main' into dev/migrie/f/1051-new…

a34f6be

…Window

DHowett requested changes Feb 19, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 19, 2021

This was the only thing Dustin was blocking on

34e0d0c

it's now _after_ 5pm on a friday so I'm not doing the rest

zadjii-msft requested a review from DHowett February 19, 2021 23:08

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 19, 2021

DHowett approved these changes Feb 19, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 19, 2021

ghost merged commit 049e37e into main Feb 19, 2021

ghost deleted the dev/migrie/f/1051-newWindow branch February 19, 2021 23:51

ghost mentioned this pull request Mar 1, 2021

Feature Request: Key binding for "New Window" #1051

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the `newWindow` action #9208

Add support for the `newWindow` action #9208

zadjii-msft commented Feb 18, 2021 •

edited by DHowett

zadjii-msft commented Feb 19, 2021

carlos-zamora left a comment •

edited by zadjii-msft

carlos-zamora Feb 19, 2021

DHowett Feb 19, 2021

carlos-zamora Feb 19, 2021

zadjii-msft Feb 19, 2021

carlos-zamora Feb 19, 2021

carlos-zamora Feb 19, 2021

carlos-zamora Feb 19, 2021

carlos-zamora Feb 19, 2021

DHowett left a comment

DHowett Feb 19, 2021

DHowett Feb 19, 2021

DHowett Feb 19, 2021

DHowett Feb 19, 2021

DHowett Feb 19, 2021

DHowett Feb 19, 2021

DHowett Feb 19, 2021

DHowett Feb 19, 2021

ghost commented Feb 19, 2021

ghost commented Mar 1, 2021

Add support for the newWindow action #9208

Add support for the newWindow action #9208

Conversation

zadjii-msft commented Feb 18, 2021 • edited by DHowett

zadjii-msft commented Feb 19, 2021

carlos-zamora left a comment • edited by zadjii-msft

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 19, 2021

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 Mar 1, 2021

Add support for the `newWindow` action #9208

Add support for the `newWindow` action #9208

zadjii-msft commented Feb 18, 2021 •

edited by DHowett

carlos-zamora left a comment •

edited by zadjii-msft

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.