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

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 <[email protected]>
microsoft · Nov 25, 2019 · 901a1e1 · 901a1e1
1 parent 983f9b5
commit 901a1e1
Show file tree

Hide file tree

Showing 32 changed files with 551 additions and 280 deletions.
diff --git a/doc/cascadia/SettingsSchema.md b/doc/cascadia/SettingsSchema.md
@@ -28,7 +28,7 @@ Properties listed below are specific to each unique profile.
 | `backgroundImageAlignment` | Optional | String | `center` | Sets how the background image aligns to the boundaries of the window. Possible values: `"center"`, `"left"`, `"top"`, `"right"`, `"bottom"`, `"topLeft"`, `"topRight"`, `"bottomLeft"`, `"bottomRight"` |
 | `backgroundImageOpacity` | Optional | Number | `1.0` | Sets the transparency of the background image. Accepts floating point values from 0-1. |
 | `backgroundImageStretchMode` | Optional | String | `uniformToFill` | Sets how the background image is resized to fill the window. Possible values: `"none"`, `"fill"`, `"uniform"`, `"uniformToFill"` |
-| `closeOnExit` | Optional | Boolean | `true` | When set to `true`, the selected tab closes when `exit` is typed. When set to `false`, the tab will remain open when `exit` is typed. |
+| `closeOnExit` | Optional | String | `graceful` | Sets how the profile reacts to termination or failure to launch. Possible values: `"graceful"` (close when `exit` is typed or the process exits normally), `"always"` (always close) and `"never"` (never close). `true` and `false` are accepted as synonyms for `"graceful"` and `"never"` respectively. |
 | `colorScheme` | Optional | String | `Campbell` | Name of the terminal color scheme to use. Color schemes are defined under `schemes`. |
 | `colorTable` | Optional | Array[String] | | Array of colors used in the profile if `colorscheme` is not set. Array follows the format defined in `schemes`. |
 | `commandline` | Optional | String | | Executable used in the profile. |

diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json
@@ -327,9 +327,21 @@
             "type": "string"
           },
           "closeOnExit": {
-            "default": true,
-            "description": "When set to true (default), the selected tab closes when the connected application exits. When set to false, the tab will remain open when the connected application exits.",
-            "type": "boolean"
+            "default": "graceful",
+            "description": "Sets how the profile reacts to termination or failure to launch. Possible values: \"graceful\" (close when exit is typed or the process exits normally), \"always\" (always close) and \"never\" (never close). true and false are accepted as synonyms for \"graceful\" and \"never\" respectively.",
+            "oneOf": [
+              {
+                "enum": [
+                  "never",
+                  "graceful",
+                  "always"
+                ],
+                "type": "string"
+              },
+              {
+                "type": "boolean"
+              }
+            ]
           },
           "colorScheme": {
             "default": "Campbell",

diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
@@ -55,6 +55,9 @@ namespace TerminalAppLocalTests
         TEST_METHOD(TestProfileIconWithEnvVar);
         TEST_METHOD(TestProfileBackgroundImageWithEnvVar);
 
+        TEST_METHOD(TestCloseOnExitParsing);
+        TEST_METHOD(TestCloseOnExitCompatibilityShim);
+
         TEST_CLASS_SETUP(ClassSetup)
         {
             InitializeJsonReader();
@@ -1414,4 +1417,67 @@ namespace TerminalAppLocalTests
         auto terminalSettings = settings._profiles[0].CreateTerminalSettings(globalSettings.GetColorSchemes());
         VERIFY_ARE_EQUAL(expectedPath, terminalSettings.BackgroundImage());
     }
+    void SettingsTests::TestCloseOnExitParsing()
+    {
+        const std::string settingsJson{ R"(
+        {
+            "profiles": [
+                {
+                    "name": "profile0",
+                    "closeOnExit": "graceful"
+                },
+                {
+                    "name": "profile1",
+                    "closeOnExit": "always"
+                },
+                {
+                    "name": "profile2",
+                    "closeOnExit": "never"
+                },
+                {
+                    "name": "profile3",
+                    "closeOnExit": null
+                },
+                {
+                    "name": "profile4",
+                    "closeOnExit": { "clearly": "not a string" }
+                }
+            ]
+        })" };
+
+        VerifyParseSucceeded(settingsJson);
+        CascadiaSettings settings{};
+        settings._ParseJsonString(settingsJson, false);
+        settings.LayerJson(settings._userSettings);
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings._profiles[0].GetCloseOnExitMode());
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Always, settings._profiles[1].GetCloseOnExitMode());
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Never, settings._profiles[2].GetCloseOnExitMode());
+
+        // Unknown modes parse as "Graceful"
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings._profiles[3].GetCloseOnExitMode());
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings._profiles[4].GetCloseOnExitMode());
+    }
+    void SettingsTests::TestCloseOnExitCompatibilityShim()
+    {
+        const std::string settingsJson{ R"(
+        {
+            "profiles": [
+                {
+                    "name": "profile0",
+                    "closeOnExit": true
+                },
+                {
+                    "name": "profile1",
+                    "closeOnExit": false
+                }
+            ]
+        })" };
+
+        VerifyParseSucceeded(settingsJson);
+        CascadiaSettings settings{};
+        settings._ParseJsonString(settingsJson, false);
+        settings.LayerJson(settings._userSettings);
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Graceful, settings._profiles[0].GetCloseOnExitMode());
+        VERIFY_ARE_EQUAL(CloseOnExitMode::Never, settings._profiles[1].GetCloseOnExitMode());
+    }
 }
diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp
@@ -592,6 +592,13 @@ namespace winrt::TerminalApp::implementation
         });
     }
 
+    // Method Description:
+    // - Returns a pointer to the global shared settings.
+    [[nodiscard]] std::shared_ptr<::TerminalApp::CascadiaSettings> AppLogic::GetSettings() const noexcept
+    {
+        return _settings;
+    }
+
     // Method Description:
     // - Update the current theme of the application. This will trigger our
     //   RequestedThemeChanged event, to have our host change the theme of the

diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h
@@ -29,6 +29,7 @@ namespace winrt::TerminalApp::implementation
 
         void Create();
         void LoadSettings();
+        [[nodiscard]] std::shared_ptr<::TerminalApp::CascadiaSettings> GetSettings() const noexcept;
 
         Windows::Foundation::Point GetLaunchDimensions(uint32_t dpi);
         winrt::Windows::Foundation::Point GetLaunchInitialPositions(int32_t defaultInitialX, int32_t defaultInitialY);

diff --git a/src/cascadia/TerminalApp/AzureCloudShellGenerator.cpp b/src/cascadia/TerminalApp/AzureCloudShellGenerator.cpp
@@ -39,7 +39,6 @@ std::vector<TerminalApp::Profile> AzureCloudShellGenerator::GenerateProfiles()
         azureCloudShellProfile.SetColorScheme({ L"Vintage" });
         azureCloudShellProfile.SetAcrylicOpacity(0.6);
         azureCloudShellProfile.SetUseAcrylic(true);
-        azureCloudShellProfile.SetCloseOnExit(false);
         azureCloudShellProfile.SetConnectionType(AzureConnectionType);
         profiles.emplace_back(azureCloudShellProfile);
     }

diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp
@@ -9,6 +9,7 @@
 #include "CascadiaSettings.h"
 #include "../../types/inc/utils.hpp"
 #include "../../inc/DefaultSettings.h"
+#include "AppLogic.h"
 #include "Utils.h"
 
 #include "PowershellCoreProfileGenerator.h"
@@ -26,6 +27,21 @@ static constexpr std::wstring_view PACKAGED_PROFILE_ICON_PATH{ L"ms-appx:///Prof
 static constexpr std::wstring_view PACKAGED_PROFILE_ICON_EXTENSION{ L".png" };
 static constexpr std::wstring_view DEFAULT_LINUX_ICON_GUID{ L"{9acb9455-ca41-5af7-950f-6bca1bc9722f}" };
 
+// Method Description:
+// - Returns the settings currently in use by the entire Terminal application.
+// Throws:
+// - HR E_INVALIDARG if the app isn't up and running.
+const CascadiaSettings& CascadiaSettings::GetCurrentAppSettings()
+{
+    auto currentXamlApp{ winrt::Windows::UI::Xaml::Application::Current().as<winrt::TerminalApp::App>() };
+    THROW_HR_IF_NULL(E_INVALIDARG, currentXamlApp);
+
+    auto appLogic = winrt::get_self<winrt::TerminalApp::implementation::AppLogic>(currentXamlApp.Logic());
+    THROW_HR_IF_NULL(E_INVALIDARG, appLogic);
+
+    return *(appLogic->GetSettings());
+}
+
 CascadiaSettings::CascadiaSettings() :
     CascadiaSettings(true)
 {

diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h
@@ -49,6 +49,8 @@ class TerminalApp::CascadiaSettings final
     static std::unique_ptr<CascadiaSettings> LoadDefaults();
     static std::unique_ptr<CascadiaSettings> LoadAll();
 
+    static const CascadiaSettings& GetCurrentAppSettings();
+
     winrt::Microsoft::Terminal::Settings::TerminalSettings MakeSettings(std::optional<GUID> profileGuid) const;
 
     GlobalAppSettings& GlobalSettings();

diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp
@@ -3,6 +3,10 @@
 
 #include "pch.h"
 #include "Pane.h"
+#include "Profile.h"
+#include "CascadiaSettings.h"
+
+#include "winrt/Microsoft.Terminal.TerminalConnection.h"
 
 using namespace winrt::Windows::Foundation;
 using namespace winrt::Windows::UI;
@@ -11,7 +15,9 @@ using namespace winrt::Windows::UI::Core;
 using namespace winrt::Windows::UI::Xaml::Media;
 using namespace winrt::Microsoft::Terminal::Settings;
 using namespace winrt::Microsoft::Terminal::TerminalControl;
+using namespace winrt::Microsoft::Terminal::TerminalConnection;
 using namespace winrt::TerminalApp;
+using namespace TerminalApp;
 
 static const int PaneBorderSize = 2;
 static const int CombinedPaneBorderSize = 2 * PaneBorderSize;
@@ -28,7 +34,7 @@ Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocus
     _root.Children().Append(_border);
     _border.Child(_control);
 
-    _connectionClosedToken = _control.ConnectionClosed({ this, &Pane::_ControlClosedHandler });
+    _connectionStateChangedToken = _control.ConnectionStateChanged({ this, &Pane::_ControlConnectionStateChangedHandler });
 
     // On the first Pane's creation, lookup resources we'll use to theme the
     // Pane, including the brushed to use for the focused/unfocused border
@@ -302,7 +308,7 @@ bool Pane::NavigateFocus(const Direction& direction)
 // - <none>
 // Return Value:
 // - <none>
-void Pane::_ControlClosedHandler()
+void Pane::_ControlConnectionStateChangedHandler(const TermControl& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*args*/)
 {
     std::unique_lock lock{ _createCloseLock };
     // It's possible that this event handler started being executed, then before
@@ -317,10 +323,24 @@ void Pane::_ControlClosedHandler()
         return;
     }
 
-    if (_control.ShouldCloseOnExit())
+    const auto newConnectionState = _control.ConnectionState();
+
+    if (newConnectionState < ConnectionState::Closed)
+    {
+        // Pane doesn't care if the connection isn't entering a terminal state.
+        return;
+    }
+
+    const auto& settings = CascadiaSettings::GetCurrentAppSettings();
+    auto paneProfile = settings.FindProfile(_profile.value());
+    if (paneProfile)
     {
-        // Fire our Closed event to tell our parent that we should be removed.
-        _closedHandlers();
+        auto mode = paneProfile->GetCloseOnExitMode();
+        if ((mode == CloseOnExitMode::Always) ||
+            (mode == CloseOnExitMode::Graceful && newConnectionState == ConnectionState::Closed))
+        {
+            _ClosedHandlers(nullptr, nullptr);
+        }
     }
 }
 
@@ -333,7 +353,7 @@ void Pane::_ControlClosedHandler()
 void Pane::Close()
 {
     // Fire our Closed event to tell our parent that we should be removed.
-    _closedHandlers();
+    _ClosedHandlers(nullptr, nullptr);
 }
 
 // Method Description:
@@ -570,16 +590,16 @@ void Pane::_CloseChild(const bool closeFirst)
         _profile = remainingChild->_profile;
 
         // Add our new event handler before revoking the old one.
-        _connectionClosedToken = _control.ConnectionClosed({ this, &Pane::_ControlClosedHandler });
+        _connectionStateChangedToken = _control.ConnectionStateChanged({ this, &Pane::_ControlConnectionStateChangedHandler });
 
         // Revoke the old event handlers. Remove both the handlers for the panes
         // themselves closing, and remove their handlers for their controls
         // closing. At this point, if the remaining child's control is closed,
         // they'll trigger only our event handler for the control's close.
         _firstChild->Closed(_firstClosedToken);
         _secondChild->Closed(_secondClosedToken);
-        closedChild->_control.ConnectionClosed(closedChild->_connectionClosedToken);
-        remainingChild->_control.ConnectionClosed(remainingChild->_connectionClosedToken);
+        closedChild->_control.ConnectionStateChanged(closedChild->_connectionStateChangedToken);
+        remainingChild->_control.ConnectionStateChanged(remainingChild->_connectionStateChangedToken);
 
         // If either of our children was focused, we want to take that focus from
         // them.
@@ -659,7 +679,7 @@ void Pane::_CloseChild(const bool closeFirst)
         // Revoke event handlers on old panes and controls
         oldFirst->Closed(oldFirstToken);
         oldSecond->Closed(oldSecondToken);
-        closedChild->_control.ConnectionClosed(closedChild->_connectionClosedToken);
+        closedChild->_control.ConnectionStateChanged(closedChild->_connectionStateChangedToken);
 
         // Reset our UI:
         _root.Children().Clear();
@@ -720,13 +740,13 @@ void Pane::_CloseChild(const bool closeFirst)
 // - <none>
 void Pane::_SetupChildCloseHandlers()
 {
-    _firstClosedToken = _firstChild->Closed([this]() {
+    _firstClosedToken = _firstChild->Closed([this](auto&& /*s*/, auto&& /*e*/) {
         _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [=]() {
             _CloseChild(true);
         });
     });
 
-    _secondClosedToken = _secondChild->Closed([this]() {
+    _secondClosedToken = _secondChild->Closed([this](auto&& /*s*/, auto&& /*e*/) {
         _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [=]() {
             _CloseChild(false);
         });
@@ -965,8 +985,8 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
     std::unique_lock lock{ _createCloseLock };
 
     // revoke our handler - the child will take care of the control now.
-    _control.ConnectionClosed(_connectionClosedToken);
-    _connectionClosedToken.value = 0;
+    _control.ConnectionStateChanged(_connectionStateChangedToken);
+    _connectionStateChangedToken.value = 0;
 
     // Remove our old GotFocus handler from the control. We don't what the
     // control telling us that it's now focused, we want it telling its new
@@ -1124,5 +1144,4 @@ void Pane::_SetupResources()
     }
 }
 
-DEFINE_EVENT(Pane, Closed, _closedHandlers, ConnectionClosedEventArgs);
 DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h
@@ -71,7 +71,7 @@ class Pane : public std::enable_shared_from_this<Pane>
 
     void Close();
 
-    DECLARE_EVENT(Closed, _closedHandlers, winrt::Microsoft::Terminal::TerminalControl::ConnectionClosedEventArgs);
+    WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
     DECLARE_EVENT(GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
 
 private:
@@ -89,7 +89,7 @@ class Pane : public std::enable_shared_from_this<Pane>
 
     bool _lastActive{ false };
     std::optional<GUID> _profile{ std::nullopt };
-    winrt::event_token _connectionClosedToken{ 0 };
+    winrt::event_token _connectionStateChangedToken{ 0 };
     winrt::event_token _firstClosedToken{ 0 };
     winrt::event_token _secondClosedToken{ 0 };
 
@@ -119,7 +119,7 @@ class Pane : public std::enable_shared_from_this<Pane>
     void _CloseChild(const bool closeFirst);
 
     void _FocusFirstChild();
-    void _ControlClosedHandler();
+    void _ControlConnectionStateChangedHandler(const winrt::Microsoft::Terminal::TerminalControl::TermControl& sender, const winrt::Windows::Foundation::IInspectable& /*args*/);
 
     std::pair<float, float> _GetPaneSizes(const float& fullSize);