spec: propose an evolution of closeOnExit by DHowett-MSFT · Pull Request #2039 · microsoft/terminal

DHowett-MSFT · 2019-07-19T17:45:13Z

Summary of the Pull Request

This pull request introduces a specification for the next generation of closeOnExit.

References

#1364 #1770

PR Checklist

Specs If the default shell can't be launched and closeOnExit=true, Terminal buys the farm #2563
CLA signed
Tests added/passed/it is a spec
Requires documentation to be updated
I've discussed this with core contributors already

Detailed Description of the Pull Request / Additional comments

Look at the spec.

Validation Steps Performed

It is a spec.

DHowett-MSFT · 2019-07-19T17:45:52Z

TODO:

create an issue
specify a followup change to conhost code that will propagate its subprocess's exit code

glen-84 · 2019-07-22T10:06:37Z

Shouldn't != 0 be = 0?

You want it to close when there is a successful (0) exit code? Or am I getting mixed up?

mcpiroman · 2019-07-22T10:26:36Z

I also find this confusing. For me the default should be that
success -> exit
failature -> stay opened.

But you could also support the opposite. Say someone has long running job in the background and if it fails he can immidiately see that terminal / tab closes (it's less important if it has just finished).

DHowett · 2019-07-22T16:19:47Z

@glen-84 correct: if closeOnExit is set to graceful, the tab will automatically close only if the connected application exits "gracefully" or successfully. The idea is that an application that runs and instantly terminates--or an application that stops accepting user input because it's died--probably put something useful on the screen for troubleshooting purposes, but an application that has exited successfully does not need any troubleshooting.

If the default switched the other way (close if != 0, stay open if 0) it would be instantly frustrating, because a user would type exit in their shell of choice and we'd just sit there telling them that the shell exited. Like they requested. Back before we introduced closeOnExit, people complained so much about that! 😁

DHowett-MSFT · 2019-07-22T16:27:07Z

@mcpiroman that is what is suggested here (default=graceful, close tab when exit code == 0)

mcpiroman · 2019-07-22T16:34:08Z

but there could be another option, failature: close when exit code != 0

glen-84 · 2019-07-23T07:42:24Z

@DHowett Since the spec text was not updated, I'm not sure if you realized that I was pointing out an issue on line #22. It should be == 0, and not != 0. 🙂

DHowett-MSFT · 2019-07-23T20:52:19Z

@glen-84 I definitely misread your comment 😄

doc/specs/#xxxx - closeOnExit evolution.md

glen-84 · 2019-07-24T06:53:41Z

doc/specs/#xxxx - closeOnExit evolution.md

+
+1. The existing `closeOnExit` profile key will become an enumerated string key with the following values/behaviours:
+   * `always` - a tab or pane hosting this profile will always be closed when the launched connection terminates.
+   * `graceful` - a tab or pane hosting this profile will be closed **iff** the launched connection closes with an exit code == 0.


DHowett-MSFT · 2019-07-24T06:58:15Z

Shorthand for “if and only if”

glen-84 · 2019-07-24T07:28:29Z

Shorthand for “if and only if”

Oh ... I feel like I should have known that, even though it seems fairly uncommon in programming. 🙂

mu88 · 2019-10-01T09:10:14Z

Is there already an issue that will implement the specified graceful behavior? I'd love to see this feature and an issue to subscribe would be nice.

DHowett-MSFT · 2019-11-06T01:48:20Z

I am clearing the review status of all signed-off reviewers, and adding @mcpiroman.

@mcpiroman: This spec is inspired by your work in #2091, with some minor refinements. Thank you. 😄

stale

doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md

carlos-zamora

Just throwing a few discussion points out there. Looks good :)

doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

zadjii-msft

I presumed that the UI considerations for this feature would be a part of this spec - like what the Terminal does in response to failed connections for graceful/never. That's what I'm really curious about here.

Carlos has some other nits I agree with, but the rest of this seems sane

doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md

zadjii-msft

Yea I like the look of this

This pull request implements the new `ITerminalConnection::ConnectionState` interface (enum, event) and connects it through TerminalControl to Pane, Tab and App as specified in #2039. It does so to implement `closeOnExit` = `graceful` in addition to the other two normal CoE types. It also: * exposes the singleton `CascadiaSettings` through a function that looks it up by using the current Xaml application's `AppLogic`. * In so doing, we've broken up the weird runaround where App tells TerminalSettings to CloseOnExit and then later another part of App _asks TerminalControl_ to tell it what TerminalSettings said App told it earlier. `:crazy_eyes:` * wires up a bunch of connection state points to `AzureConnection`. This required moving the Azure connection's state machine to use another enum name (oops). * ships a helper class for managing connection state transitions. * contains a bunch of template magic. * introduces `WINRT_CALLBACK`, a oneshot callback like `TYPED_EVENT`. * replaces a bunch of disparate `_connecting` and `_closing` members with just one uberstate. * updates the JSON schema and defaults to prefer closeOnExit: graceful * updates all relevant documentation Specified in #2039 Fixes #2563 Co-authored-by: mcpiroman <38111589+mcpiroman@users.noreply.github.com>

DHowett-MSFT added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Product-Terminal The new Windows Terminal. labels Jul 19, 2019

DHowett-MSFT commented Jul 23, 2019

View reviewed changes

doc/specs/#xxxx - closeOnExit evolution.md Outdated Show resolved Hide resolved

glen-84 reviewed Jul 24, 2019

View reviewed changes

carlos-zamora previously approved these changes Jul 25, 2019

View reviewed changes

zadjii-msft previously approved these changes Jul 25, 2019

View reviewed changes

mcpiroman mentioned this pull request Jul 26, 2019

Handle connection's state #2091

Closed

5 tasks

miniksa mentioned this pull request Jul 31, 2019

Needs to show an error (rather than just a blank tab) if a shell is missing #982

Closed

DHowett-MSFT mentioned this pull request Sep 5, 2019

If the default shell can't be launched and closeOnExit=true, Terminal buys the farm #2563

Closed

This was referenced Sep 13, 2019

Feature Request: report errors that occur when starting a new tab #2745

Closed

Terminal crashes on launch - no error in Event Viewer #2535

Closed

Spec: propose an evolution of closeOnExit and TerminalConnection

5763be3

DHowett-MSFT requested a review from zadjii-msft November 6, 2019 01:48

DHowett-MSFT force-pushed the spec/duhowett/closeonexit branch from 1a1d5e0 to 5763be3 Compare November 6, 2019 01:49

DHowett-MSFT commented Nov 6, 2019

View reviewed changes

doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md Outdated Show resolved Hide resolved

miniksa reviewed Nov 6, 2019

View reviewed changes

doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md Outdated Show resolved Hide resolved

miniksa approved these changes Nov 6, 2019

View reviewed changes

carlos-zamora approved these changes Nov 13, 2019

View reviewed changes

Update doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md

6e7eba2

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

zadjii-msft reviewed Nov 14, 2019

View reviewed changes

doc/specs/#2563 - closeOnExit and TerminalConnection evolution.md Outdated Show resolved Hide resolved

PR feedback

5b17972

DHowett-MSFT mentioned this pull request Nov 19, 2019

Implement ConnectionState and closeOnExit=graceful/always/never #3623

Merged

5 tasks

zadjii-msft approved these changes Nov 20, 2019

View reviewed changes

DHowett-MSFT changed the title ~~Spec: propose an evolution of closeOnExit~~ spec: propose an evolution of closeOnExit Nov 20, 2019

DHowett-MSFT merged commit b3ecb73 into master Nov 20, 2019

DHowett-MSFT deleted the spec/duhowett/closeonexit branch November 20, 2019 23:48

Conversation

DHowett-MSFT commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

DHowett-MSFT commented Jul 19, 2019

Uh oh!

glen-84 commented Jul 22, 2019

Uh oh!

mcpiroman commented Jul 22, 2019

Uh oh!

DHowett commented Jul 22, 2019 • edited by DHowett-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett-MSFT commented Jul 22, 2019

Uh oh!

mcpiroman commented Jul 22, 2019

Uh oh!

glen-84 commented Jul 23, 2019

Uh oh!

DHowett-MSFT commented Jul 23, 2019

Uh oh!

Uh oh!

glen-84 Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT commented Jul 24, 2019

Uh oh!

glen-84 commented Jul 24, 2019

Uh oh!

mu88 commented Oct 1, 2019

Uh oh!

DHowett-MSFT commented Nov 6, 2019

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DHowett-MSFT commented Jul 19, 2019 •

edited

Loading

DHowett commented Jul 22, 2019 •

edited by DHowett-MSFT

Loading