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 windowingBehavior setting #9118

Merged
91 commits merged into from
Feb 19, 2021

Conversation

zadjii-msft
Copy link
Member

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

Adds support for the windowingBehavior global setting. This setting
controls how mutiple instances of wt behave in the absence of the -w
parameter. This setting has three values:

  • "useNew": (default) Multiple wt invocations (without the -w
    param) always create new windows.
  • "useAnyExisting": When starting a new wt, we'll instead default to
    any existing windows. wt -w -1 will still create new windows.
  • "useExisting": Similar to useAnyExisting, but limits to
    windows on the current desktop.

The IVirtualDesktopManager interface is very limited. Hence why we
have to track the HWNDs manually, and ask if they're on the current
desktop.

Validation Steps Performed

I've been playing with it for a week now.

References #5000
References projects/5
References #8898
Spec'd in #8135
Closes #2227
Closes https://github.com/microsoft/terminal/projects/5#card-51431448
Closes https://github.com/microsoft/terminal/projects/5#card-51431433

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
…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

Now the setting works right in the UI as well:
image

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.

This is great! Just tagged Kayla on one thing because I don't want us to have to revisit the wordsmithing conversation later.

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
Comment on lines 235 to 249
<data name="Globals_WindowingBehavior.Header" xml:space="preserve">
<value>New windows open in...</value>
</data>
<data name="Globals_WindowingBehavior.HelpText" xml:space="preserve">
<value>Controls how new terminal instances attach to existing windows.</value>
</data>
<data name="Globals_WindowingBehaviorUseNew.Content" xml:space="preserve">
<value>a new window</value>
</data>
<data name="Globals_WindowingBehaviorUseAnyExisting.Content" xml:space="preserve">
<value>the most recently used window</value>
</data>
<data name="Globals_WindowingBehaviorUseExisting.Content" xml:space="preserve">
<value>the most recently used window on this desktop</value>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

@cinnamon-msft please take a look here. Easier to get the wording correct now than have to get it localized and change it later.

@DHowett
Copy link
Member

DHowett commented Feb 18, 2021

minor minor nit: PR body needs updating to reflect current enum names for things

@DHowett
Copy link
Member

DHowett commented Feb 18, 2021

DO NOT AUTOMERGE

@@ -232,6 +232,22 @@
<data name="Globals_LaunchModeMaximized.Content" xml:space="preserve">
<value>Maximized</value>
</data>
<data name="Globals_WindowingBehavior.Header" xml:space="preserve">
<value>New windows open in...</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>New windows open in...</value>
<value>wt.exe window behavior</value>

Copy link
Member

Choose a reason for hiding this comment

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

Hmmmmm... this will be weird when Def Term is in

Copy link
Member Author

Choose a reason for hiding this comment

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

@cinnamon-msft Yea I concur. Plus I'm just not sure that the users will always think "ah yes, I'm running wt.exe". Like when running from the start menu - that's also going to behave this way.

I'm fine flipping these back since what's proposed is basically what I had originally - I just want to make sure we're sure, and I'm not so sure about mentioning "wt.exe"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Launch window behavior?

<value>Controls how new terminal instances attach to existing windows.</value>
</data>
<data name="Globals_WindowingBehaviorUseNew.Content" xml:space="preserve">
<value>a new window</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>a new window</value>
<value>Create a new window</value>

<value>a new window</value>
</data>
<data name="Globals_WindowingBehaviorUseAnyExisting.Content" xml:space="preserve">
<value>the most recently used window</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>the most recently used window</value>
<value>Attach to the most recently used window</value>

<value>the most recently used window</value>
</data>
<data name="Globals_WindowingBehaviorUseExisting.Content" xml:space="preserve">
<value>the most recently used window on this desktop</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>the most recently used window on this desktop</value>
<value>Attach to the most recently used window on this desktop</value>

<comment>This text is a fragment of a sentence that indicates the options below it, in a user interface. It is a header, but it should be read like a sentence that ends with one of {Globals_WindowingBehaviorUseNew.Content}, {Globals_WindowingBehaviorUseAnyExisting.Content}, or {Globals_WindowingBehaviorUseExisting.Content}.</comment>
</data>
<data name="Globals_WindowingBehavior.HelpText" xml:space="preserve">
<value>Controls how new terminal instances attach to existing windows.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>Controls how new terminal instances attach to existing windows.</value>
<value>Controls how new terminal instances launched from wt.exe attach to existing windows.</value>

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels a little redundant though, right? Like, all the windows are ones spawned from wt.exe. Do we really need to specify that?

@DHowett
Copy link
Member

DHowett commented Feb 19, 2021

(I personally love the sentenceized version of it ("new windows open in..."))

@DHowett
Copy link
Member

DHowett commented Feb 19, 2021

Demo gif moved from main body

windowingBehavior-useExisting-000

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.

LOVE THIS.

@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 69318d3 into main Feb 19, 2021
@ghost ghost deleted the dev/migrie/f/2227-windowingBehavior branch February 19, 2021 21:09
@DHowett
Copy link
Member

DHowett commented Feb 19, 2021

I guess I am incredibly bad at reading and got too excited AND AUTOMERGED THE "DO NOT AUTOMERGE" PR.

ghost pushed a commit that referenced this pull request Feb 19, 2021
Finally implements the `newWindow` action. It does so by
`ShellExecute`ing `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
@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-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Option to only allow one instance
5 participants