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

sotteson1 · 2022-09-22T17:18:10Z

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

carlos-zamora

Just the one thing. This is awesome, thanks!

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

DHowett

Thanks for doing this! I've got a couple thoughts before I dig into the code (whereupon I'm preemptively blocking to have the discussion/avoid it merging while we noodle!)

I'm a little worried about making connections restartable. In our original connection state flow model, connections never regressed and only marched forward to one of the terminal states (finished, failed). There's a few knock-on effects as a result of this change: output threads need to be restartable, and listeners to the state change notification need to be audited to make sure they, too, can handle regressive state flow.
- I wonder, does TerminalControl or TerminalApp have enough information to reconstruct the connection anew?
Should we emit a RIS (hard reset) to the terminal before restarting? Apps like vim and MC leaving the terminal in an inconsistent state when they die is a big cause of user confusion, and this feature might offer similar behavior across application sessions/conpty sessions.
- (I know you worked on cursor inheritance to make this work; however, cursor inheritance has its own set of ... let's call them adorable quirks like History window partially invisible when switching from bash to cmd.exe #3678; what are your thoughts here?)

sotteson1 · 2022-09-27T21:01:06Z

I'm a little worried about making connections restartable. In our original connection state flow model, connections never regressed and only marched forward to one of the terminal states (finished, failed). There's a few knock-on effects as a result of this change: output threads need to be restartable, and listeners to the state change notification need to be audited to make sure they, too, can handle regressive state flow.

I totally get the concern about the connection going back to the beginning from closed/failed. I wasn't sure I could get it to work without changing a lot of stuff, but I think I found a minimal way to do it. Also, I make sure I've closed the output thread and the pipes before I restart. Maybe there's more to it than that. I've only been looking at this code for a week now, so I'm definitely no expert!

(I know you worked on cursor inheritance to make this work; however, cursor inheritance has its own set of ... let's call them adorable quirks like History window partially invisible when switching from bash to cmd.exe #3678; what are your thoughts here?)

As for cursor inheritance, the big thing I had to figure out was to always preserve the changing size (rows, cols), even when closed/failed, and then use them when restarting. Before I figured that out, it was always using the initial size which wasn't always guaranteed to be what the pane was currently using, and weird things happened in the output.

Also, I considered clearing/resetting the buffer at restart instead of inheriting the current cursor. That might be safer, although I saw that people were asking to reuse the buffer when restarting, so that's what I was trying to do.

… Enter to restart terminal

…Enter to restart terminal

vadimkantorov · 2022-10-18T06:08:07Z

One minor comment: Ctrl+D may be a clearer message and match keybinding info in tab tooltips, at least makes sense to add it in brackets ^D (i.e. Ctrl+D)

zadjii-msft · 2022-10-24T20:00:23Z

Initial thought just playing with this: hot DAMN re-using the same terminal feels crisp. I kinda imagined the restart being a whole "discard the existing TermControl and recreate a new one in its place", but this is neat.

I agree that we might want to avoid "restartable connections", and instead make fresh connections. That can totally be an us problem IMO, as a pre-ship codehealth fix (I don't really want to ask you to do that).

…Enter to restart terminal

sotteson1 · 2022-10-25T19:06:11Z

Initial thought just playing with this: hot DAMN re-using the same terminal feels crisp. I kinda imagined the restart being a whole "discard the existing TermControl and recreate a new one in its place", but this is neat.

Yeah, when I got it working, I thought it was pretty slick. I have terminals running PowerShell where it would be nice to say "exit" and then be able to hit Enter to get a fresh terminal with the same command line parameters that started it. Much better than exiting the tab and hunting down the one I want to start a clean one.

zadjii-msft

The code is really straightforward. Yea, lets do this. I'll file the follow up to this, I wouldn't want to bother a non-team member with that kind of code shuffling

zadjii-msft · 2022-11-02T20:46:27Z

src/cascadia/TerminalControl/ControlCore.cpp

+                return true;
+            }
+
+            if (ch == Enter)


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.

zadjii-msft · 2022-11-03T11:25:49Z

src/cascadia/TerminalConnection/ConnectionStateHolder.h

+        void _resetConnectionState()
+        {
+            {
+                std::lock_guard<std::mutex> stateLock{ _stateMutex };


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

src/cascadia/TerminalConnection/ConptyConnection.cpp

sotteson1 · 2022-11-11T23:06:51Z

One minor comment: Ctrl+D may be a clearer message and match keybinding info in tab tooltips, at least makes sense to add it in brackets ^D (i.e. Ctrl+D)

I went ahead and just changed it to Ctrl+D in the text.

sotteson1 · 2022-11-11T23:07:07Z

If you can, please fix merge conflicts too :). Thanks for being so patient with us.

Done!

DHowett · 2022-12-08T01:43:33Z

Moved from PR body

@sotteson1 thanks so much for this, and for your patience!

DHowett

I'm cool with this, but I did have a minor rewording suggestion and a question.

Thanks again!

And thank you Mike for taking on the follow up work 😄

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

ghost · 2022-12-08T12:53:43Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2023-01-24T18:22:58Z

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

A different take on #14548. > We didn't love that a connection could transition back in the state diagram, from Closed -> Start. That felt wrong. To remedy this, we're going to allow the ControlCore to... ASK the app to restart its connection. This is a much more sensible approach, than leaving the ConnectionInfo in the core and having the core do the restart itself. That's mental. Cleanup from #14060 Closes #14327 Obsoletes #14548

Adds an action for immediately restarting the connection. I suspect most folks that wanted #3726 will be happy just with the <kbd>enter</kbd> solution from #14060, but this will work without having to `exit` the client. Just, relaunch whatever the commandline is. Easy peasy. Closes #3726. Obsoletes #14549

sotteson1 added 4 commits September 22, 2022 00:32

Add support to close a failed pain with ctrl+D

21cf69d

Implement ctrl+D as an event that Pane watches

5606291

Fix a couple of typos from the last cleanup

6de50b0

Fix formatting issues

d902eb8

sotteson1 added 3 commits September 23, 2022 11:28

Add support for restarting terminal with Enter

092957f

Add comment to line setting PSEUDOCONSOLE_INHERIT_CURSOR

27096e6

Keep track of rows/cols in connection even when disconnected

ec7dfc5

sotteson1 changed the title ~~Ctrl+d to close a pane when the terminal has exited~~ When the terminal has exited, ctrl+D to close pane, Enter to restart terminal Sep 23, 2022

Fix analysis error

82b6876

carlos-zamora requested changes Sep 26, 2022

View reviewed changes

src/cascadia/TerminalConnection/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 26, 2022

DHowett requested changes Sep 27, 2022

View reviewed changes

Add comment to help with localization.

62888a5

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 27, 2022

carlos-zamora added a commit that referenced this pull request Oct 8, 2022

PRE-MERGE #14060: When the terminal has exited, ctrl+D to close pane,…

7920c8d

… Enter to restart terminal

carlos-zamora added a commit that referenced this pull request Oct 11, 2022

PRE-MERGE #14060 When the terminal has exited, ctrl+D to close pane, …

ad1056c

…Enter to restart terminal

carlos-zamora added a commit that referenced this pull request Oct 25, 2022

PRE-MERGE #14060 When the terminal has exited, ctrl+D to close pane, …

f07fa60

…Enter to restart terminal

zadjii-msft approved these changes Nov 3, 2022

View reviewed changes

zadjii-msft mentioned this pull request Nov 3, 2022

Code Health: follow ups from connection restarting #14327

Closed

3 tasks

zadjii-msft requested review from carlos-zamora and DHowett November 3, 2022 11:37

zadjii-msft assigned DHowett Nov 3, 2022

sotteson1 added 2 commits November 11, 2022 14:25

Update message to use Ctrl+D instead of ^D

899f009

Merge branch 'main' into ctrl-d-support

52c59ed

zadjii-msft mentioned this pull request Nov 23, 2022

[feature request] Add extra information for known error codes from WSL #12410

Closed

zadjii-msft added the zBugBash-Consider label Dec 6, 2022

Merge remote-tracking branch 'origin/main' into ctrl-d-support

0c190c2

This comment has been minimized.

Sign in to view

there's not a better way to name that message

7febffe

This comment has been minimized.

Sign in to view

DHowett approved these changes Dec 8, 2022

View reviewed changes

src/cascadia/TerminalConnection/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

src/cascadia/TerminalConnection/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

nits

634baba

zadjii-msft unassigned DHowett Dec 8, 2022

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 8, 2022

ghost merged commit c0e4689 into microsoft:main Dec 8, 2022

This was referenced Dec 13, 2022

Refactor how connection restarting is handled #14548

Closed

Add an action for immediately restarting a connection #14549

Closed

carlos-zamora mentioned this pull request Mar 8, 2023

Display connection state in tab & add tab context menu entry for restarting connection #14909

Closed

This was referenced Apr 26, 2023

(A better) Refactoring of how connection restarting is handled #15240

Merged

Add an action for immediately restarting a connection #15241

Merged

lhecker mentioned this pull request May 23, 2023

Feature Request: restart/reload terminal #15401

Closed

thernstig mentioned this pull request Jun 28, 2023

Feature Request: Restart/reload terminal via a new command name #15617

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sotteson1 commented Sep 22, 2022 •

edited by DHowett

Loading

carlos-zamora left a comment

DHowett left a comment

sotteson1 commented Sep 27, 2022 •

edited

Loading

vadimkantorov commented Oct 18, 2022

zadjii-msft commented Oct 24, 2022

sotteson1 commented Oct 25, 2022

zadjii-msft left a comment

zadjii-msft Nov 2, 2022

zadjii-msft Nov 3, 2022

sotteson1 commented Nov 11, 2022

sotteson1 commented Nov 11, 2022

This comment has been minimized.

This comment has been minimized.

DHowett commented Dec 8, 2022

DHowett left a comment

ghost commented Dec 8, 2022

ghost commented Jan 24, 2023

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

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

Conversation

sotteson1 commented Sep 22, 2022 • edited by DHowett Loading

Validation Steps Performed

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

sotteson1 commented Sep 27, 2022 • edited Loading

vadimkantorov commented Oct 18, 2022

zadjii-msft commented Oct 24, 2022

sotteson1 commented Oct 25, 2022

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Nov 2, 2022

Choose a reason for hiding this comment

zadjii-msft Nov 3, 2022

Choose a reason for hiding this comment

sotteson1 commented Nov 11, 2022

sotteson1 commented Nov 11, 2022

This comment has been minimized.

This comment has been minimized.

DHowett commented Dec 8, 2022

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Dec 8, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jan 24, 2023

sotteson1 commented Sep 22, 2022 •

edited by DHowett

Loading

sotteson1 commented Sep 27, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.