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

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 <[email protected]> Date: Fri Nov 8 14:16:44 2019 -0800 Merge remote-tracking branch 'github/master' into dev/duhowett/closeonexit commit 4d6e975 Author: Dustin Howett <[email protected]> Date: Wed Nov 6 15:49:38 2019 -0800 further hax commit 1950413 Author: Dustin Howett <[email protected]> Date: Wed Nov 6 07:30:32 2019 -0800 hacks: try to implement the state thing commit 22dd029 Merge: 5763be3 357e835 Author: Dustin Howett <[email protected]> Date: Wed Nov 6 15:15:21 2019 -0800 Merge remote-tracking branch 'github/master' into HEAD commit 5763be3 Author: Dustin Howett <[email protected]> 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

                            {
-                                _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

@@ -156,8 +156,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
        // DON'T CALL _InitializeTerminal here - wait until the swap chain is loaded to do that.

        // 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DHowett-MSFT commented Nov 19, 2019 •

edited

Loading

DHowett-MSFT left a comment

miniksa left a comment

DHowett-MSFT commented Nov 22, 2019

azure-pipelines bot commented Nov 22, 2019

zadjii-msft left a comment

zadjii-msft Nov 22, 2019

DHowett-MSFT Nov 22, 2019

DHowett-MSFT Nov 25, 2019

DHowett-MSFT commented Nov 22, 2019

azure-pipelines bot commented Nov 22, 2019

zadjii-msft left a comment

zadjii-msft Nov 25, 2019

DHowett-MSFT Nov 25, 2019

DHowett-MSFT Nov 25, 2019

zadjii-msft Nov 25, 2019

DHowett-MSFT Nov 25, 2019

ghost commented Jan 14, 2020

mu88 commented Jan 15, 2020

DHowett-MSFT commented Jan 15, 2020

mu88 commented Jan 15, 2020

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

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

Conversation

DHowett-MSFT commented Nov 19, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

DHowett-MSFT left a comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Nov 22, 2019

azure-pipelines bot commented Nov 22, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Nov 22, 2019

azure-pipelines bot commented Nov 22, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 14, 2020

mu88 commented Jan 15, 2020

DHowett-MSFT commented Jan 15, 2020

mu88 commented Jan 15, 2020

DHowett-MSFT commented Nov 19, 2019 •

edited

Loading