Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the newWindow action #9208

Merged
105 commits merged into from
Feb 19, 2021
Merged

Add support for the newWindow action #9208

105 commits merged into from
Feb 19, 2021

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Feb 18, 2021

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

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.
@zadjii-msft
Copy link
Member Author

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

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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).

Comment on lines +91 to +94
// else if (_ProfileIndex)
// {
// ss << fmt::format(L"profile index: {}, ", _ProfileIndex.Value());
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So remove the dead code for index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is valuable to keep.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moved stuff around. Could revert changes to this file

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@carlos-zamora carlos-zamora removed their assignment Feb 19, 2021
@ghost ghost closed this Feb 19, 2021
@ghost ghost deleted the branch main February 19, 2021 21:09
@DHowett DHowett reopened this Feb 19, 2021
@DHowett DHowett changed the base branch from dev/migrie/f/2227-windowingBehavior to main February 19, 2021 21:13
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +91 to +94
// else if (_ProfileIndex)
// {
// ss << fmt::format(L"profile index: {}, ", _ProfileIndex.Value());
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is valuable to keep.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically only works for CascadiaPackage and not WTU

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a blocking comment!

newTerminalArgs = NewTerminalArgs();
}

auto [profileGuid, settings] = TerminalSettings::BuildSettings(_settings,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BIG HAMMER to get a guid from an args

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not blocking)

@ghost 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
  it's now _after_ 5pm on a friday so I'm not doing the rest
@ghost 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 DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 19, 2021
@ghost
Copy link

ghost commented Feb 19, 2021

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 ghost merged commit 049e37e into main Feb 19, 2021
@ghost ghost deleted the dev/migrie/f/1051-newWindow branch February 19, 2021 23:51
@ghost
Copy link

ghost commented Mar 1, 2021

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Key binding for "New Window"
6 participants