[DxD] Add 'Automatic' as a mode for CloseOnExit #13560

PankajBhojwani · 2022-07-21T19:28:16Z

Summary of the Pull Request

Adds a new mode to CloseOnExit: Automatic. In this mode, if a process handed off by defterm terminates for whatever reason, we always close (i.e. we treat the mode as Always), but for processes launched by Terminal we terminate as with the Graceful behaviour.

PR Checklist

Closes [DxD] closeOnExit should be sensible when launched via defterm #13325
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
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

Detailed Description of the Pull Request / Additional comments

Adds a new enum value to CloseOnExit
Adds a new function to Pane: FinalizeConfigurationGivenDefault: this is a function that should be called when the pane is created via default terminal handoff, and can contain any special configurations we should set given that the pane was created via handoff

Validation Steps Performed

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

zadjii-msft

Plumbing wise this is all great, but let's workshop the naming / phrasing

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

carlos-zamora

update the schema plz
update the docs plz
update defaults.json (we're setting closeOnExit there) (check userDefaults.json too)

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/Pane.cpp

…/pabhoj/dxd_close

PankajBhojwani · 2022-07-26T21:46:53Z

@carlos-zamora I've updated the schema and the defaults, will get to the docs after the PR merges

DHowett · 2022-07-29T18:37:32Z

Please make sure you update the pull request title and body to reflect what we settled on / what the PR actually does!

src/cascadia/TerminalApp/Pane.cpp

zadjii-msft

Guys lemme tell you, I missed the discussion where you came up with "automatic" but I friggen love it

…/pabhoj/dxd_close

ghost · 2022-08-01T20:33:59Z

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.

DHowett · 2022-08-01T22:03:44Z

Hey, since you're storing default terminal state on the pane... have you made absolutely certain it propagates to the correct child if you split it? And goes back up to the parent when you un-split it? This seems potentially risky to manage as pane specific state.

DHowett · 2022-08-01T22:04:26Z

I can confirm that no, it doesn't work once you split the pane. ☹️

## Summary of the Pull Request In #13560 we added a member to `Pane` that lets it know if it was spawned as a default terminal session, but did not propagate that value when the pane gets split or when the pane closes. This commit fixes that. ## Validation Steps Performed A session spawned by a def term invocation remembers it even as it goes through splits

## Summary of the Pull Request Adds a new mode to `CloseOnExit`: `Automatic`. In this mode, if a process handed off by defterm terminates for whatever reason, we always close (i.e. we treat the mode as `Always`), but for processes launched by Terminal we terminate as with the `Graceful` behaviour. ## PR Checklist * [x] Closes #13325 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] 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 ## Detailed Description of the Pull Request / Additional comments - Adds a new enum value to `CloseOnExit` - Adds a new function to `Pane`: `FinalizeConfigurationGivenDefault`: this is a function that should be called when the pane is created via default terminal handoff, and can contain any special configurations we should set given that the pane was created via handoff ## Validation Steps Performed (cherry picked from commit 89d57e8) Service-Card-Id: 84836029 Service-Version: 1.15

## Summary of the Pull Request In #13560 we added a member to `Pane` that lets it know if it was spawned as a default terminal session, but did not propagate that value when the pane gets split or when the pane closes. This commit fixes that. ## Validation Steps Performed A session spawned by a def term invocation remembers it even as it goes through splits (cherry picked from commit 1a77834) Service-Card-Id: 84836635 Service-Version: 1.15

ghost · 2022-08-17T23:30:28Z

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

Handy links:

new enum value

23114f8

ghost added Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Jul 21, 2022

PankajBhojwani commented Jul 21, 2022

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Show resolved Hide resolved

PankajBhojwani commented Jul 21, 2022

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

PankajBhojwani commented Jul 21, 2022

View reviewed changes

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw Show resolved Hide resolved

zadjii-msft reviewed Jul 22, 2022

View reviewed changes

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

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw Show resolved Hide resolved

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 22, 2022

carlos-zamora requested changes Jul 22, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 22, 2022

combine nests

2bcd818

This comment has been minimized.

Sign in to view

Rename flag

9ca624f

This comment has been minimized.

Sign in to view

carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 25, 2022

PankajBhojwani added 3 commits July 26, 2022 14:39

automatic

69e6a2b

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

7d906ce

…/pabhoj/dxd_close

update schema

f385cbb

carlos-zamora approved these changes Jul 26, 2022

View reviewed changes

PankajBhojwani changed the title ~~[DxD] Add 'GracefulIfLaunchedByTerminal' as a mode for CloseOnExit~~ [DxD] Add 'Automatic' as a mode for CloseOnExit Jul 29, 2022

DHowett added a commit that referenced this pull request Jul 29, 2022

PRE-MERGE #13560 [DxD] Add 'Automatic' as a mode for CloseOnExit

5fbdccf

lhecker reviewed Jul 30, 2022

View reviewed changes

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

zadjii-msft approved these changes Aug 1, 2022

View reviewed changes

PankajBhojwani added 2 commits August 1, 2022 12:49

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

93b461d

…/pabhoj/dxd_close

update given new bool name, check automatic in non-defterm session too

126a48f

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 1, 2022

ghost merged commit 89d57e8 into main Aug 1, 2022

ghost deleted the dev/pabhoj/dxd_close branch August 1, 2022 21:11

PankajBhojwani mentioned this pull request Aug 1, 2022

Fix _isDefTermSession not propagating upon pane split/close #13649

Merged

ghost mentioned this pull request Aug 17, 2022

[DxD] closeOnExit should be sensible when launched via defterm #13325

Closed

zadjii-msft mentioned this pull request Aug 23, 2022

"cmd.exe" does not exit the terminal properly if the last command didn't exit with 0 #13814

Closed

j4james mentioned this pull request Sep 20, 2022

SettingsModelLocalTests failures are on the rise #14050

Closed

zadjii-msft mentioned this pull request Dec 5, 2022

Graceful shutdown does not seems to happen with default profile that launches PowerShell #13582

Closed

zadjii-msft mentioned this pull request May 4, 2023

"Profile termination behavior" automatic not documented MicrosoftDocs/terminal#661

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DxD] Add 'Automatic' as a mode for CloseOnExit #13560

[DxD] Add 'Automatic' as a mode for CloseOnExit #13560

PankajBhojwani commented Jul 21, 2022 •

edited

Loading

zadjii-msft left a comment

carlos-zamora left a comment •

edited by PankajBhojwani

Loading

This comment has been minimized.

This comment has been minimized.

PankajBhojwani commented Jul 26, 2022

DHowett commented Jul 29, 2022

zadjii-msft left a comment

ghost commented Aug 1, 2022

DHowett commented Aug 1, 2022

DHowett commented Aug 1, 2022

ghost commented Aug 17, 2022

[DxD] Add 'Automatic' as a mode for CloseOnExit #13560

[DxD] Add 'Automatic' as a mode for CloseOnExit #13560

Conversation

PankajBhojwani commented Jul 21, 2022 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment • edited by PankajBhojwani Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

PankajBhojwani commented Jul 26, 2022

DHowett commented Jul 29, 2022

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Aug 1, 2022

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.

DHowett commented Aug 1, 2022

DHowett commented Aug 1, 2022

ghost commented Aug 17, 2022

PankajBhojwani commented Jul 21, 2022 •

edited

Loading

carlos-zamora left a comment •

edited by PankajBhojwani

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.