Yeah, I'm a little uncomfortable with this partial hard reset concept. If we were using either a plain hard reset, or a plain soft reset that would be fine. It may not be exactly what we want, but they're at least well-defined concepts. This feels like we're hacking in the things we want to reset for a single use case.

But maybe I'm being too picky. We had a similar discussion in PR #18437, which was never fully resolved. So I think we should at least try and make sure that what we're doing here is also going to work for that use case, so we can share the functionality if possible.

I'm also a little worried about how this is going to interact with the conhost state. If we're resetting Windows Terminal, but not resetting conhost, are they not going to end up out of sync now? Do we not have a way to pass that reset through somehow? This is maybe less of an issue with the new conpty, but it could still affect shells that rely on the legacy console APIs. I might be overthinking things though. If you're doing a reset, the terminal is assumedly messed up already, so an imperfect fix is probably better than nothing.

And assuming we do want to keep this HardResetWithoutBufferClear concept, my personal preference would be to drop the additional method, and just add the bool directly to the existing HardReset. But that's just a nitpick. It's fine as it is if you disagree.

Took a look at Dustin's and James' feedback in this tread as well as PR #18437 and issue #18425.

General overview

I'm starting by writing this here because it gives a good overview of what HardReset(false) does and it's a good starting point. The main thing that was bothering me was that we needed ResetInputModes(). I got GH Copilot to spit out a nice comparison table for easy review:

From looking through these and reading Copilot's report, here's my breakdown:

• Definitely worth resetting:

  • alt/main buffer + page buffers: makes sense. New connection should be back on main buffer.
  • character sets: useful if app switched to something like line drawing charset.
  • color table: makes sense. We want the right colors to make sure everything is properly legible.
  • mouse tracking mode + win32/kitty: makes sense to reset, that's the main one that bothered me
  • bracketed paste: paste would be wrapped, so makes sense to reset.
  • focus event reporting: makes sense. Annoying functionality.

• Not sure because I don't know enough about this space, but I think it makes sense based on the notes below:

  • sixel parser: half-parsed sixel image scenario. Not great, but not sure how real this is.
  • c1 controls: useful if 8-bit c1 parsing is enabled.
  • code page: wrong code page -> wrong character interpretation
  • tab stops: "minor, but custom tab stops from dead app are meaningless to new shell". Fair.
  • origin mode: useful, cursor addressing relative to margins
  • macros: leftover macros from dead app

Let me know what you all think. At least here it's all listed out, but I'm sure you'll think of interesting corner cases haha 😅.

Dustin's comment

#19971 (review)

Hmmmm. I don't love how we have to send this all the way down through the RIS handler.
However, it's also the only thing that knows the full gamut of things that need done to
reset the terminal.

If I had nits to pick, they would be:
ResetConnection should be named HardResetWithoutErase,
clearBuffers should be named erase,
BufferClear should be called Erase;

Fixed all of these 😊

later we can also add TermControl::HardReset which allows whoever's driving terminal
to reset it with all the usual ceremony.

Also, why does TerminalCore need to reach into StateMachine? What state is stuck in stateMachine?

The concern here is that the old app wrote a partial escape sequence before dying. I can remove it if you think that's not really valid though.

James' comment

#19971 (comment)

Yeah, I'm a little uncomfortable with this partial hard reset concept. If we were using either
a plain hard reset, or a plain soft reset that would be fine. It may not be exactly what we want,
but they're at least well-defined concepts. This feels like we're hacking in the things we want to
reset for a single use case.

But maybe I'm being too picky. We had a similar discussion in PR #18437, which was never fully resolved.
So I think we should at least try and make sure that what we're doing here is also going to work for that
use case, so we can share the functionality if possible.

Took a look at PR #18437. The new HardResetWithoutErase() call in TerminalPage::_restartPaneConnection() will be hit when somebody follows the "press Enter to restart" message. So yeah, this should be marked to close #18425 (and we should make sure that's fixed appropriately).

I'm going to lean hard on your second paragraph above haha. I'm hoping we can figure out how to make this partial hard reset concept work, but if not, I guess we could do just a plain hard reset? I'm just hesitant to do that because then you lose the buffer contents, and I think that's worth advocating for. Hopefully the list above helps figure out what works and what doesn't.

I'm also a little worried about how this is going to interact with the conhost state. If we're
resetting Windows Terminal, but not resetting conhost, are they not going to end up out of sync now?
Do we not have a way to pass that reset through somehow? This is maybe less of an issue with the new conpty,
but it could still affect shells that rely on the legacy console APIs. I might be overthinking things though.
If you're doing a reset, the terminal is assumedly messed up already, so an imperfect fix is probably better
than nothing.

So, in TerminalPage::_restartPaneConnection() we get this:

• _duplicateConnectionForRestart() creates a new ConptyConnection
• ControlCore::Connection(newConnection) --> _closeConnection()--> old ConPTY'sClose()`
• newConnection.Start() --> ConptyCreatePseudoConsole() --> new conhost with clean default state

I think we're good then because we're creating a new ConPTY process, so there's no stale conhost to get out of sync with. That said, let me know if I'm missing something.

And assuming we do want to keep this HardResetWithoutBufferClear concept, my personal preference would
be to drop the additional method, and just add the bool directly to the existing HardReset. But that's
just a nitpick. It's fine as it is if you disagree.

Added 👍

old PR - fixing issue w/ re-entry "Ctrl+D or Enter" message

#18437 (comment)

I don't think this deals with the case where the character set is corrupted, so the user won't even see the message
telling them to press Ctrl+D or Enter. That's why I was saying it would be best if the reset happened before the exit
message. But in that case a hard reset is probably not appropriate, because the user is also likely to want to see the
application output that triggered the process exit.

I looked into this a bit. To make the reset happen before the exit message (printed in ConptyConnection), we'd have to detect that the connection closed down in Terminal. This requires a bit more thought, but I don't think that should block this PR. We could make this a separate issue to track imo.

And even after they've pressed Enter, a hard reset may not be appropriate. I can imagine some people will expect their
session to restart with the scrollback intact, and might find this change annoying. I don't know what the right
solution is, but I think it's worth spending some time figuring out how exactly we want this to work before starting on
the code.

The nice thing with the current approach is that we do retain scrollback. More just including this quote for completeness.

@carlos-zamora Thanks for the detailed write up. I think I'm convinced now that this is a good solution. It does seem reasonable to reset everything in that table, and just preserve the screen/scrollback. And I didn't initially realise we were getting a completely new connection here, so my conhost concerns were nonsense. +1 from me.

I think you were right, though, James, about the conhost thing. We're starting a new connection with an existing buffer, which I think means we need to pass PSEUDOCONSOLE_INHERIT_CURSOR. Unless @lhecker made that no longer required with his recent ConPTY rearchitecture, it was required to ensure the spawned client application had some consistency with the Terminal...

As for the "Press ^D" message... it's always been my goal (soft goal, I guess) to make that a passive TerminalControl UI element rather than rendering it to the display surface. Alas, the to-do list is longer than I'd like

I think you were right, though, James, about the conhost thing. We're starting a new connection with an existing buffer, which I think means we need to pass PSEUDOCONSOLE_INHERIT_CURSOR. Unless @lhecker made that no longer required with his recent ConPTY rearchitecture, it was required to ensure the spawned client application had some consistency with the Terminal...

Oh! Looks like we're good on that end:

1. _restartPaneConnection()
   

   terminal/src/cascadia/TerminalApp/TerminalPage.cpp

   Lines 3735 to 3748 in c562dad

   	void TerminalPage::_restartPaneConnection(
   	const TerminalApp::TerminalPaneContent& paneContent,
   	const winrt::Windows::Foundation::IInspectable&)
   	{
   	// Note: callers are likely passing in `nullptr` as the args here, as
   	// the TermControl.RestartTerminalRequested event doesn't actually pass
   	// any args upwards itself. If we ever change this, make sure you check
   	// for nulls
   	if (const auto& connection{ _duplicateConnectionForRestart(paneContent) })
   	{
   	paneContent.GetTermControl().Connection(connection);
   	connection.Start();
   	}
   	}

2. calls _duplicateConnectionForRestart() (link)
   

   terminal/src/cascadia/TerminalApp/TerminalPage.cpp

   Line 1618 in c562dad

   return _CreateConnectionFromSettings(profile, *controlSettings.DefaultSettings(), true);

3. calls _CreateConnectionFromSettings(profile, *controlSettings.DefaultSettings(), true) (link)
   

   terminal/src/cascadia/TerminalApp/TerminalPage.cpp

   Lines 1543 to 1546 in c562dad

   	if (inheritCursor)
   	{
   	valueSet.Insert(L"inheritCursor", Windows::Foundation::PropertyValue::CreateBoolean(true));
   	}

That last true is inheritCursor, so PSEUDOCONSOLE_INHERIT_CURSOR is set on the ConptyConnection.

then perhaps all that's left to do is update the description to match the new implementation

Done 😊