Skip to content

Commit

Permalink
When the terminal has exited, ctrl+D to close pane, Enter to restart …
Browse files Browse the repository at this point in the history
…terminal (#14060)

When a terminal process exits (successful or not) and the profile isn't
set to automatically close the pane, a new message is displayed:

You can now close this terminal with ^D or Enter to restart.

Ctrl+D then is able to close the pane and Enter restarts it.

I originally tried to do this at the ConptyConnection layer by changing
the connection state from Failed to Closed, but then that didn't work
for the case where the process exited successfully but the profile isn't
set to exit automatically. So, I added an event to
ControlCore/TermControl that Pane watches. ControlCore watches to see if
the input is Ctrl+D (0x4) and if the connection is closed or failed, and
then raises the event so that Pane can close itself. As it turned out, I
think this is the better place to have the logic to watch for the Ctrl+D
key. Doing it at the ConptyConnection layer meant I had to parse out the
key from the escaped text passed to ConptyConnection::WriteInput.

## Validation Steps Performed
Tried adding lots of panes and then killing the processes outside of
Terminal. Each showed the new message and I could close them with Ctrl+D
or restart them with Enter. Also set a profile to never close
automatically to make sure Ctrl+D would work when a process exits
successfully.

Closes #12849 
Closes #11570
Closes #4379
  • Loading branch information
sotteson1 authored Dec 8, 2022
1 parent da2b80b commit c0e4689
Show file tree
Hide file tree
Showing 12 changed files with 106 additions and 19 deletions.
2 changes: 2 additions & 0 deletions .github/actions/spelling/allow/allow.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ clickable
clig
CMMI
copyable
CtrlDToClose
cybersecurity
dalet
Dcs
Expand All @@ -22,6 +23,7 @@ downside
downsides
dze
dzhe
DTo
EDDB
EDDC
Enum'd
Expand Down
29 changes: 28 additions & 1 deletion src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Pane::Pane(const Profile& profile, const TermControl& control, const bool lastFo

_connectionStateChangedToken = _control.ConnectionStateChanged({ this, &Pane::_ControlConnectionStateChangedHandler });
_warningBellToken = _control.WarningBell({ this, &Pane::_ControlWarningBellHandler });
_closeTerminalRequestedToken = _control.CloseTerminalRequested({ this, &Pane::_CloseTerminalRequestedHandler });

// 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
Expand Down Expand Up @@ -993,7 +994,7 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
// actually no longer _our_ control, and instead could be a descendant.
//
// When the control's new Pane takes ownership of the control, the new
// parent will register it's own event handler. That event handler will get
// parent will register its own event handler. That event handler will get
// fired after this handler returns, and will properly cleanup state.
if (!_IsLeaf())
{
Expand Down Expand Up @@ -1036,6 +1037,26 @@ void Pane::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundatio
}
}

void Pane::_CloseTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*args*/)
{
std::unique_lock lock{ _createCloseLock };

// It's possible that this event handler started being executed, then before
// we got the lock, another thread created another child. So our control is
// actually no longer _our_ control, and instead could be a descendant.
//
// When the control's new Pane takes ownership of the control, the new
// parent will register its own event handler. That event handler will get
// fired after this handler returns, and will properly cleanup state.
if (!_IsLeaf())
{
return;
}

Close();
}

winrt::fire_and_forget Pane::_playBellSound(winrt::Windows::Foundation::Uri uri)
{
auto weakThis{ weak_from_this() };
Expand Down Expand Up @@ -1586,6 +1607,7 @@ void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
// Add our new event handler before revoking the old one.
_connectionStateChangedToken = _control.ConnectionStateChanged({ this, &Pane::_ControlConnectionStateChangedHandler });
_warningBellToken = _control.WarningBell({ this, &Pane::_ControlWarningBellHandler });
_closeTerminalRequestedToken = _control.CloseTerminalRequested({ this, &Pane::_CloseTerminalRequestedHandler });

// Revoke the old event handlers. Remove both the handlers for the panes
// themselves closing, and remove their handlers for their controls
Expand All @@ -1601,6 +1623,7 @@ void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
{
p->_control.ConnectionStateChanged(p->_connectionStateChangedToken);
p->_control.WarningBell(p->_warningBellToken);
p->_control.CloseTerminalRequested(p->_closeTerminalRequestedToken);
}
});
}
Expand All @@ -1609,6 +1632,7 @@ void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
remainingChild->Closed(remainingChildClosedToken);
remainingChild->_control.ConnectionStateChanged(remainingChild->_connectionStateChangedToken);
remainingChild->_control.WarningBell(remainingChild->_warningBellToken);
remainingChild->_control.CloseTerminalRequested(remainingChild->_closeTerminalRequestedToken);

// If we or either of our children was focused, we want to take that
// focus from them.
Expand Down Expand Up @@ -1690,6 +1714,7 @@ void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
{
p->_control.ConnectionStateChanged(p->_connectionStateChangedToken);
p->_control.WarningBell(p->_warningBellToken);
p->_control.CloseTerminalRequested(p->_closeTerminalRequestedToken);
}
});
}
Expand Down Expand Up @@ -2448,6 +2473,8 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitDirect
_connectionStateChangedToken.value = 0;
_control.WarningBell(_warningBellToken);
_warningBellToken.value = 0;
_control.CloseTerminalRequested(_closeTerminalRequestedToken);
_closeTerminalRequestedToken.value = 0;

// Remove our old GotFocus handler from the control. We don't want the
// control telling us that it's now focused, we want it telling its new
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/Pane.h
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ class Pane : public std::enable_shared_from_this<Pane>
winrt::event_token _firstClosedToken{ 0 };
winrt::event_token _secondClosedToken{ 0 };
winrt::event_token _warningBellToken{ 0 };
winrt::event_token _closeTerminalRequestedToken{ 0 };

winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker;
winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker;
Expand Down Expand Up @@ -290,6 +291,7 @@ class Pane : public std::enable_shared_from_this<Pane>
const winrt::Windows::UI::Xaml::RoutedEventArgs& e);
void _ControlLostFocusHandler(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::UI::Xaml::RoutedEventArgs& e);
void _CloseTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/);

std::pair<float, float> _CalcChildrenSizes(const float fullSize) const;
SnapChildrenSizeResult _CalcSnappedChildrenSizes(const bool widthOrHeight, const float fullSize) const;
Expand Down
8 changes: 8 additions & 0 deletions src/cascadia/TerminalConnection/ConnectionStateHolder.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
return _isStateOneOf(ConnectionState::Connected);
}

void _resetConnectionState()
{
{
std::lock_guard<std::mutex> stateLock{ _stateMutex };
_connectionState = ConnectionState::NotConnected;
}
}

private:
std::atomic<ConnectionState> _connectionState{ ConnectionState::NotConnected };
mutable std::mutex _stateMutex;
Expand Down
52 changes: 36 additions & 16 deletions src/cascadia/TerminalConnection/ConptyConnection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
const HANDLE hServerProcess,
const HANDLE hClientProcess,
TERMINAL_STARTUP_INFO startupInfo) :
_initialRows{ 25 },
_initialCols{ 80 },
_rows{ 25 },
_cols{ 80 },
_guid{ Utils::CreateGuid() },
_inPipe{ hIn },
_outPipe{ hOut }
Expand Down Expand Up @@ -268,8 +268,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
_commandline = winrt::unbox_value_or<winrt::hstring>(settings.TryLookup(L"commandline").try_as<Windows::Foundation::IPropertyValue>(), _commandline);
_startingDirectory = winrt::unbox_value_or<winrt::hstring>(settings.TryLookup(L"startingDirectory").try_as<Windows::Foundation::IPropertyValue>(), _startingDirectory);
_startingTitle = winrt::unbox_value_or<winrt::hstring>(settings.TryLookup(L"startingTitle").try_as<Windows::Foundation::IPropertyValue>(), _startingTitle);
_initialRows = winrt::unbox_value_or<uint32_t>(settings.TryLookup(L"initialRows").try_as<Windows::Foundation::IPropertyValue>(), _initialRows);
_initialCols = winrt::unbox_value_or<uint32_t>(settings.TryLookup(L"initialCols").try_as<Windows::Foundation::IPropertyValue>(), _initialCols);
_rows = winrt::unbox_value_or<uint32_t>(settings.TryLookup(L"initialRows").try_as<Windows::Foundation::IPropertyValue>(), _rows);
_cols = winrt::unbox_value_or<uint32_t>(settings.TryLookup(L"initialCols").try_as<Windows::Foundation::IPropertyValue>(), _cols);
_guid = winrt::unbox_value_or<winrt::guid>(settings.TryLookup(L"guid").try_as<Windows::Foundation::IPropertyValue>(), _guid);
_environment = settings.TryLookup(L"environment").try_as<Windows::Foundation::Collections::ValueSet>();
if constexpr (Feature_VtPassthroughMode::IsEnabled())
Expand Down Expand Up @@ -307,16 +307,33 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
void ConptyConnection::Start()
try
{
bool usingExistingBuffer = false;
if (_isStateAtOrBeyond(ConnectionState::Closed))
{
_resetConnectionState();
usingExistingBuffer = true;
}

_transitionToState(ConnectionState::Connecting);

const til::size dimensions{ gsl::narrow<til::CoordType>(_initialCols), gsl::narrow<til::CoordType>(_initialRows) };
const til::size dimensions{ gsl::narrow<til::CoordType>(_cols), gsl::narrow<til::CoordType>(_rows) };

// If we do not have pipes already, then this is a fresh connection... not an inbound one that is a received
// handoff from an already-started PTY process.
if (!_inPipe)
{
DWORD flags = PSEUDOCONSOLE_RESIZE_QUIRK | PSEUDOCONSOLE_WIN32_INPUT_MODE;

// If we're using an existing buffer, we want the new connection
// to reuse the existing cursor. When not setting this flag, the
// PseudoConsole sends a clear screen VT code which our renderer
// interprets into making all the previous lines be outside the
// current viewport.
if (usingExistingBuffer)
{
flags |= PSEUDOCONSOLE_INHERIT_CURSOR;
}

if constexpr (Feature_VtPassthroughMode::IsEnabled())
{
if (_passthroughMode)
Expand Down Expand Up @@ -440,6 +457,9 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
winrt::hstring exitText{ fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status)) };
_TerminalOutputHandlers(L"\r\n");
_TerminalOutputHandlers(exitText);
_TerminalOutputHandlers(L"\r\n");
_TerminalOutputHandlers(RS_(L"CtrlDToClose"));
_TerminalOutputHandlers(L"\r\n");
}
CATCH_LOG();
}
Expand Down Expand Up @@ -492,15 +512,11 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation

void ConptyConnection::Resize(uint32_t rows, uint32_t columns)
{
// If we haven't started connecting at all, it's still fair to update
// the initial rows and columns before we set things up.
if (!_isStateAtOrBeyond(ConnectionState::Connecting))
{
_initialRows = rows;
_initialCols = columns;
}
// Otherwise, we can really only dispatch a resize if we're already connected.
else if (_isConnected())
// Always keep these in case we ever want to disconnect/restart
_rows = rows;
_cols = columns;

if (_isConnected())
{
THROW_IF_FAILED(ConptyResizePseudoConsole(_hPC.get(), { Utils::ClampToShortMax(columns, 1), Utils::ClampToShortMax(rows, 1) }));
}
Expand Down Expand Up @@ -548,7 +564,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
void ConptyConnection::Close() noexcept
try
{
if (_transitionToState(ConnectionState::Closing))
const bool isClosing = _transitionToState(ConnectionState::Closing);
if (isClosing || _isStateAtOrBeyond(ConnectionState::Closed))
{
// EXIT POINT

Expand Down Expand Up @@ -579,7 +596,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
_hOutputThread.reset();
}

_transitionToState(ConnectionState::Closed);
if (isClosing)
{
_transitionToState(ConnectionState::Closed);
}
}
}
CATCH_LOG()
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalConnection/ConptyConnection.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
void _indicateExitWithStatus(unsigned int status) noexcept;
void _ClientTerminated() noexcept;

til::CoordType _initialRows{};
til::CoordType _initialCols{};
til::CoordType _rows{};
til::CoordType _cols{};
uint64_t _initialParentHwnd{ 0 };
hstring _commandline{};
hstring _startingDirectory{};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,10 @@
<value>[process exited with code {0}]</value>
<comment>The first argument {0} is the error code of the process. When there is no error, the number ZERO will be displayed. </comment>
</data>
<data name="CtrlDToClose" xml:space="preserve">
<value>You can now close this terminal with Ctrl+D, or press Enter to restart.</value>
<comment>"Ctrl+D" and "Enter" represent keys the user will press (control+D and Enter).</comment>
</data>
<data name="ProcessFailedToLaunch" xml:space="preserve">
<value>[error {0} when launching `{1}']</value>
<comment>The first argument {0} is the error code. The second argument {1} is the user-specified path to a program.
Expand Down
19 changes: 19 additions & 0 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,25 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const WORD scanCode,
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers)
{
const wchar_t CtrlD = 0x4;
const wchar_t Enter = '\r';

if (_connection.State() >= winrt::Microsoft::Terminal::TerminalConnection::ConnectionState::Closed)
{
if (ch == CtrlD)
{
_CloseTerminalRequestedHandlers(*this, nullptr);
return true;
}

if (ch == Enter)
{
_connection.Close();
_connection.Start();
return true;
}
}

if (ch == L'\x3') // Ctrl+C or Ctrl+Break
{
_handleControlC();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TYPED_EVENT(ShowWindowChanged, IInspectable, Control::ShowWindowArgs);
TYPED_EVENT(UpdateSelectionMarkers, IInspectable, Control::UpdateSelectionMarkersEventArgs);
TYPED_EVENT(OpenHyperlink, IInspectable, Control::OpenHyperlinkEventArgs);
TYPED_EVENT(CloseTerminalRequested, IInspectable, IInspectable);
// clang-format on

private:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/ControlCore.idl
Original file line number Diff line number Diff line change
Expand Up @@ -161,5 +161,6 @@ namespace Microsoft.Terminal.Control
event Windows.Foundation.TypedEventHandler<Object, ShowWindowArgs> ShowWindowChanged;
event Windows.Foundation.TypedEventHandler<Object, UpdateSelectionMarkersEventArgs> UpdateSelectionMarkers;
event Windows.Foundation.TypedEventHandler<Object, OpenHyperlinkEventArgs> OpenHyperlink;
event Windows.Foundation.TypedEventHandler<Object, Object> CloseTerminalRequested;
};
}
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
PROJECTED_FORWARDED_TYPED_EVENT(SetTaskbarProgress, IInspectable, IInspectable, _core, TaskbarProgressChanged);
PROJECTED_FORWARDED_TYPED_EVENT(ConnectionStateChanged, IInspectable, IInspectable, _core, ConnectionStateChanged);
PROJECTED_FORWARDED_TYPED_EVENT(ShowWindowChanged, IInspectable, Control::ShowWindowArgs, _core, ShowWindowChanged);
PROJECTED_FORWARDED_TYPED_EVENT(CloseTerminalRequested, IInspectable, IInspectable, _core, CloseTerminalRequested);

PROJECTED_FORWARDED_TYPED_EVENT(PasteFromClipboard, IInspectable, Control::PasteFromClipboardEventArgs, _interactivity, PasteFromClipboard);

Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalControl/TermControl.idl
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ namespace Microsoft.Terminal.Control

event Windows.Foundation.TypedEventHandler<Object, ShowWindowArgs> ShowWindowChanged;

event Windows.Foundation.TypedEventHandler<Object, Object> CloseTerminalRequested;

Boolean CopySelectionToClipboard(Boolean singleLine, Windows.Foundation.IReference<CopyFormat> formats);
void PasteTextFromClipboard();
void SelectAll();
Expand Down

0 comments on commit c0e4689

Please sign in to comment.