Implement ConnectionState and closeOnExit=graceful/always/never by DHowett-MSFT · Pull Request #3623 · microsoft/terminal

DHowett-MSFT · 2019-11-19T01:42:29Z

Summary of the Pull Request

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.
... contains a TODO (whoops, sorry, if you point it out you'll be removed from the selfhost program)
... introduces WINRT_CALLBACK, a oneshot callback like TYPED_EVENT.
... replaces a bunch of disparate _connecting and _closing members with just one uberstate.

References

Specification is in PR #2039.

PR Checklist

Closes If the default shell can't be launched and closeOnExit=true, Terminal buys the farm #2563, Closes Needs to show an error (rather than just a blank tab) if a shell is missing #982
CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already.

commit c36b2482c04070529cbb009b09ec60cea70da8bd Merge: 4d6e975 c274b38 Author: Dustin Howett <duhowett@microsoft.com> Date: Fri Nov 8 14:16:44 2019 -0800 Merge remote-tracking branch 'github/master' into dev/duhowett/closeonexit commit 4d6e975 Author: Dustin Howett <duhowett@microsoft.com> Date: Wed Nov 6 15:49:38 2019 -0800 further hax commit 1950413 Author: Dustin Howett <duhowett@microsoft.com> Date: Wed Nov 6 07:30:32 2019 -0800 hacks: try to implement the state thing commit 22dd029 Merge: 5763be3 357e835 Author: Dustin Howett <duhowett@microsoft.com> Date: Wed Nov 6 15:15:21 2019 -0800 Merge remote-tracking branch 'github/master' into HEAD commit 5763be3 Author: Dustin Howett <duhowett@microsoft.com> Date: Tue Nov 5 17:47:22 2019 -0800 Spec: propose an evolution of closeOnExit and TerminalConnection

…YWHERE

doc/cascadia/profiles.schema.json

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp

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

src/cascadia/inc/cppwinrt_utils.h

src/cascadia/TerminalApp/Profile.cpp

src/cascadia/TerminalConnection/ConptyConnection.h

DHowett-MSFT

…nexit

miniksa

Looks good to me.

DHowett-MSFT · 2019-11-22T23:05:04Z

/azp run

azure-pipelines · 2019-11-22T23:05:14Z

Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft

I'm 27/32, and it's 5pm here, so unfortunately this is going to have to wait till monday from me :(

Nothing major so far though

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/inc/cppwinrt_utils.h

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

src/cascadia/TerminalConnection/EchoConnection.h

zadjii-msft · 2019-11-22T23:12:43Z

src/cascadia/TerminalConnection/ConnectionStateHolder.h

+            std::lock_guard<std::mutex> stateLock{ _stateMutex };
+            // dark magic: this is a C++17 fold expression that expands into
+            // state == 1 || state == 2 || state == 3 ...
+            return (... || (_connectionState == args));


I still don't understand what this dark magic does. is it equivalent to the following:

_connectionState == args[0] || _connectionState == args[1] || _connectionState == args[2] ...

(if you could index into the params like that?)

YES! Exactly.

And the one above it is like

typeof(args[0]) == ConnectionState && typeof(args[1] == ConnectionState) && ...

I'm going to expand these since i have to make a comment revision anyway

DHowett-MSFT · 2019-11-22T23:45:41Z

/azp run

azure-pipelines · 2019-11-22T23:45:55Z

Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft

I suppose none of my comments are more than nits.

zadjii-msft · 2019-11-25T15:07:39Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+                            // This should result in our termination.
+                            if (_transitionToState(ConnectionState::Closed))
                            {
-                                _state = State::NoConnect;


Should we just remove the AzureState::NoConnect state entirely now?

I'm working on the AZ conn in my free time, I'll defer this to then.

er defer this to that work

src/cascadia/TerminalConnection/ConptyConnection.cpp

zadjii-msft · 2019-11-25T15:18:33Z

src/cascadia/TerminalControl/TermControl.cpp

        // Subscribe to the connection's disconnected event and call our connection closed handlers.
-        _connection.TerminalDisconnected([=]() {
-            _connectionClosedHandlers();
+        _connectionStateChangedRevoker = _connection.StateChanged(winrt::auto_revoke, [weakThis = get_weak()](auto&& /*s*/, auto&& /*v*/) {


This looks like a pattern we should probably be using in other places, right? Where the event handler doesn't capture a strong ref to the handler object? (maybe we should file a CodeHealth task for this)

Yes, absolutely.

ghost · 2020-01-14T17:16:49Z

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

mu88 · 2020-01-15T07:21:57Z

I curiously tried the new graceful behavior and one question came up to me. I've updated to v0.8.10091.0 and have the following profile:

{
    "name" : "Build Script",
    "closeOnExit": "graceful",
    "guid" : "{65869F08-7651-44A5-9EAB-982A1577BC65}",
    "commandline" : "cmd.exe /k Batch\\Build.bat",
    "startingDirectory" : "C:\\Projects\\Code",
    "fontFace" : "Fira Code",
    "fontSize" : 10,
    "icon" : "C:\\Projects\\Code\\DotNet\\icon.ico"
}

When opening a new tab with Build Script, the script Build.bat gets triggered and succeeds. When executing echo Exit Code is %errorlevel% in the opened tab, 0 is returned. Nonetheless, the tab remains open. I would have expected the tab to close. Is my assumption wrong or is there any issue?

DHowett-MSFT · 2020-01-15T08:27:26Z

if I recall correctly, You want cmd /c. /k means “continue running after you evaluate the command.” You can pretend it means “[k]eep running”.

mu88 · 2020-01-15T11:33:07Z

You are absolutely right, thank you!

DHowett added 9 commits November 18, 2019 15:40

Gotta admit, I don't love it.

f3a010b

Add a connection state helper

5a11dcf

xyxy

11b557f

doc comments

0577fe0

invoke-codeformat

410bf59

do the schema dance

4661db8

and the doc?

1d537ae

couple more azure fail points

f38b736

DHowett-MSFT changed the title ~~Implement closeOnExit=graceful/always/never~~ Implement ITerminalConnection::ConnectionState and closeOnExit=graceful/always/never Nov 19, 2019

DHowett added 2 commits November 18, 2019 17:48

I guess serialize the closeyboi?

270fb73

but do it RIGHT

17f1025

DHowett-MSFT force-pushed the dev/duhowett/closeonexit branch from f8fae5a to 17f1025 Compare November 19, 2019 01:49

DHowett added 2 commits November 18, 2019 17:51

one also must update the default settings -- wow, closeOnExit is EVER…

0f82786

…YWHERE

guess we could also test CoE parsing

2af2827

DHowett-MSFT marked this pull request as ready for review November 19, 2019 01:56

beviu reviewed Nov 19, 2019

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

do the thing greg pointed out

9cc256f

carlos-zamora requested changes Nov 20, 2019

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 Nov 20, 2019

DHowett-MSFT commented Nov 21, 2019

View reviewed changes

DHowett added 3 commits November 22, 2019 13:46

PR feedback from carlos

7f3b83c

testos.

eb0ff76

Merge remote-tracking branch 'github/master' into dev/duhowett/closeo…

e62f425

…nexit

miniksa approved these changes Nov 22, 2019

View reviewed changes

DHowett-MSFT requested a review from zadjii-msft November 22, 2019 22:19

zadjii-msft reviewed Nov 22, 2019

View reviewed changes

zadjii-msft approved these changes Nov 25, 2019

View reviewed changes

carlos-zamora approved these changes Nov 25, 2019

View reviewed changes

comments for days

9193867

DHowett-MSFT changed the title ~~Implement ITerminalConnection::ConnectionState and closeOnExit=graceful/always/never~~ Implement ConnectionState and closeOnExit=graceful/always/never Nov 25, 2019

DHowett-MSFT merged commit 901a1e1 into master Nov 25, 2019

DHowett-MSFT deleted the dev/duhowett/closeonexit branch November 25, 2019 22:22

DHowett-MSFT mentioned this pull request Jan 10, 2020

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

Closed

ghost mentioned this pull request Jan 14, 2020

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

Closed

zadjii-msft mentioned this pull request Mar 16, 2020

Reproducible crash (null pointer deref in pane.cpp) when using ctrl+mouse_wheel to change text size #3713

Closed

j4james mentioned this pull request Feb 6, 2022

Terminal v1.13 seems to shut down slower compared to 1.12 #12394

Closed

Conversation

DHowett-MSFT commented Nov 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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT commented Nov 22, 2019

Uh oh!

azure-pipelines bot commented Nov 22, 2019

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT commented Nov 22, 2019

Uh oh!

azure-pipelines bot commented Nov 22, 2019

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 14, 2020

Uh oh!

mu88 commented Jan 15, 2020

Uh oh!

DHowett-MSFT commented Jan 15, 2020

Uh oh!

mu88 commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DHowett-MSFT commented Nov 19, 2019 •

edited

Loading