Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When the terminal has exited, ctrl+D to close pane, Enter to restart terminal #14060

Merged
14 commits merged into from
Dec 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: removing this delta should be part of the codehealth follow-up

_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)
sotteson1 marked this conversation as resolved.
Show resolved Hide resolved
{
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: file a blocking codehealth task, to turn this into an event for the control to request from the app a new connection.

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