Merge the PrintString functionality into AdaptDispatch

[j4james]

The main purpose of this PR was to merge the ITerminalApi::PrintString
implementations into a shared method in AdaptDispatch, and avoid the
VT code path depending on WriteCharsLegacy. But this refactoring has
also fixed some bugs that existed in the original implementations.

This helps to close the gap between the Conhost and Terminal (#13408).

I started by taking the WriteCharsLegacy implementation, and stripping
out everything that didn't apply to the VT code path. What was left was
a fairly simple loop with the following steps:

1. Check if delayed wrap is set, and if so, move to the next line.
2. Write out as much of the string as will fit on the current line.
3. If we reach the end of the line, set the delayed wrap flag again.
4. Repeat the loop until the entire string has been output.

But step 2 was a little more complicated than necessary because of its
legacy history. It was copying the string into a temporary buffer,
manually estimated how much of it would fit, and then passing on that
partial buffer to the TextBuffer::Write method.

In the new implementation, we just pass the entire string directly to
TextBuffer::WriteLine, and that handles the clipping itself. The
returned OutputCellIterator tells us how much of the string is left.
This approach fixes some issues with wide characters, and should also
cope better with future buffer enhancements.

Another improvement from the new implementation is that the Terminal now
handles delayed EOL wrap correctly. However, the downside of this is
that it introduced a cursor-dropping bug that previously only affected
conhost. I hadn't originally intended to fix that, but it became more of
an issue now.

The root cause was the fact that we called cursor.StartDeferDrawing()
before outputting the text, and this was something I had adopted in the
new implementation as well. But I've now removed that, and instead just
call cursor.SetIsOn(false). This seems to avoid the cursor droppings,
and hopefully still has similar performance benefits.

The other thing worth mentioning is that I've eliminated some special
casing handling for the ENABLE_VIRTUAL_TERMINAL_PROCESSING mode and
the WC_DELAY_EOL_WRAP flag in the WriteCharsLegacy function. They
were only used for VT output, so aren't needed here anymore.

Validation Steps Performed

I've just been testing manually, writing out sample text both in ASCII
and with wide Unicode chars. I've made sure it wraps correctly when
exceeding the available space, but doesn't wrap when stopped at the last
column, and with DECAWM disabled, it doesn't wrap at all.

I've also confirmed that the test case from issue #12739 is now working
correctly, and the cursor no longer disappears in Windows Terminal when
writing to the last column (i.e. the delayed EOL wrap is working).

Closes #780
Closes #6162
Closes #6555
Closes #12440
Closes #12739

[j4james]

I've just realised there's another cursor-dropping case that can occur, similar to #12739, but when you've got a wide character at the end of the line. I think it's probably the result of the cursor bounds checking issue that I mentioned in #13001 (comment).

Technically this has always been a problem (at least it's not new to this PR), but once Windows Terminal is handling the delayed EOL wrap correctly, it's going to be more likely to occur in everyday usage. So perhaps it's something that needs to be fixed first.

[DHowett]

(We may hold this until 1.18 -- how do you feel about that? Or, does it fix issues that we should get ahead of that are new regressions in 1.17 as well?)

[j4james]

(We may hold this until 1.18 -- how do you feel about that? Or, does it fix issues that we should get ahead of that are new regressions in 1.17 as well?)

Yeah, I'm happy to leave it until 1.18 - I don't think it's fixing anything major, and it's quite a big change, so there's always a risk it could be introducing new bugs.

[j4james]

One thing worth mentioning is that I had a PR ready to go for the IRM mode (#1947), which is built on top of this. But we've been without that for so long, I don't think it's the end of the world if it gets pushed back another release.

[zadjii-msft]

Oh man I really want to close out IRM and all these other threads, but yea we might want to hold this one. I think the 1.17 cutoff really snuck up on us after the holidays 😅

[j4james]

FYI, I found another two issues that are fixed by this PR: #6162 and #6555. It'll also fix at least some layout breaking cases in tmux (#6987). Essentially any time you use one of the characters from #6555 you would previously have ended up with a broken tmux layout, and now that doesn't happen.

I should add that this is probably largely thanks to the work Leonard has done in the TextBuffer code. But the WriteCharsLegacy implementation does its own width calculations, so it wasn't benefiting from those improvements, whereas the PrintString implementation in this PR relies on the buffer to determine the correct character widths.

[zadjii-msft]

Wait you're telling me there's a chance to fix #6987? @DHowett maybe we should pull this in to 1.17

[j4james]

It's definitely not going to fix all of the #6987 issues (for example, the U+FE0E test case I demonstrated is still broken), but it should at least fix some of them.

The problem is you've got hundreds of people reporting the problem, but nobody ever narrows it down to an easily reproducible test case. So it's hard to tell if they're all triggered by the same thing, or they're all wildly different.

[zadjii-msft]

Thank you for cleaning up one of my oldest messes. I can't tell you how happy I am that

This is not great but I need it demoable. Fix by making a buffer stream writer

will no longer be true. That was possibly one of my most egregious #fuckitshipit decisions in the Terminal codebase.

[zadjii-msft]

/note: where'd this go

[zadjii-msft]

oh duh

The root cause was the fact that we called cursor.StartDeferDrawing()
before outputting the text, and this was something I had adopted in the
new implementation as well. But I've now removed that, and instead just
call cursor.SetIsOn(false). This seems to avoid the cursor droppings,
and hopefully still has similar performance benefits.

[zadjii-msft]

One thing worth mentioning is that I had a PR ready to go for the IRM mode (#1947), which is built on top of this. But we've been without that for so long, I don't think it's the end of the world if it gets pushed back another release.

If you wanna fire that PR off we may as well pull that in for 1.17 too. This PR was a lot less scary than the label would imply, tbh. A fix for IRM might just be worth yolo'ing it.

[ghost]

Hello @DHowett!

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.

[DHowett]

Agreed. I'm happy with a YOLO approach to IRM. James, you're a g-d d-mn star!

[lhecker]

I've also been working on IRM and got a PR about 75% done (tests are missing). But of course I'd be more than happy to merge @j4james's PR instead. Although it might be helpful to compare our approaches in either case. 🙂

[DHowett]

I would love to see both! We could even look at it as "now and future" or fold them together magically

[j4james]

I've just finished work for the day, and I'm a bit exhausted, but I'll try and put my IRM PR up later tonight after I've had something to eat. I think it does include a couple of basic unit tests, so you're welcome to take those for Leonard's PR of you prefer his approach.

[zadjii-msft]

@j4james also don't stress about it - this ain't your day job, it's ours 😝 We can absolutely ship without it, so take your time. We're in no rush.

[j4james]

I've submitted PR #14700 has a draft for now if you want to check it out. I haven't filled in the PR notes, but there's really not much to it. It's very much a hack, though, so I'd be more than happy to have it replaced by something better. It was just intended to be a quick fix.

[ghost]

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

Handy links:

• Release Notes
• Store Download

[lhecker]

@j4james If you use WSL in OpenConsole, starting this PR, it won't scroll the viewport anymore when you type in your prompt, etc. I found that this branch is what made it work previously:

terminal/src/host/_stream.cpp

Lines 269 to 276 in c79298d

	// if at right or bottom edge of window, scroll right or down one char.
	if (cursorMovedPastViewport)
	{
	til::point WindowOrigin;
	WindowOrigin.x = 0;
	WindowOrigin.y = coordCursor.y - screenInfo.GetViewport().BottomInclusive();
	Status = screenInfo.SetViewportOrigin(false, WindowOrigin, true);
	}

Do you remember whether this was an intentional change? I'd be happy to write a fix if it wasn't. (If you have any preferences where you'd like such a fix to be added in AdeptDispatch/etc., let me know!)

Edit: Although, perhaps we just want to replicate Windows Terminal's Terminal::TrySnapOnInput? I'm not entirely sure what the best fix would look like.

[j4james]

It's not so much that I wanted to remove the "snap on input" behaviour, but I suspect I chose not to replicate that part of AdjustCursorPosition, because the way it was implemented was breaking VT applications.

The issue is that VT code works within a specific area of the scrollback buffer, which is typically the visible viewport. But since the user can manually scroll that viewport, we have to track the VT viewport independently via the _virtualBottom field. However, when you call SetViewportOrigin, that potentially changes the _virtualBottom, and then your VT viewport can end up in the wrong place.

So while I'm sure it's fixable, it's probably not just a matter of reinstating that original code. Also be aware that the _virtualBottom code is quite fragile, so it's easy to break something inadvertently. Every time I've tried to fix an issue related to _virtualBottom I've ended up introducing another bug elsewhere.

[j4james]

I should add that I would recommend not adjusting the horizontal viewport position for now, because in VT apps that's controlled by the horizontal cursor coupling mode (DECHCCM), which should be disabled by default.

There's actually also a vertical cursor coupling mode, but I'm not sure that's intended to apply to the scrollback situation we're dealing with here, and anyway that mode is supposed to be enabled by default.

[lhecker]

When you say you'd recommend against it, are you referring to snapping "on output" the way conhost did it before, or even snapping on input, the way Windows Terminal does it?

While the existence of snap-on-input or similar is not that big of a deal for most terminals including Windows Terminal, conhost has scroll-forward enabled by default and finding the prompt in the standard 9000-line scrollback is somewhat fiddly. Since the v1.18 OpenConsole will ship with the next Windows I'm worried people may complain about this change.

[DHowett]

Since the v1.18 OpenConsole will ship with the next Windows I'm worried people may complain about this change.

It's actually 1.19... and the gates for bug fixes are practically closed. Unless it's a bulletproof one-line fix, I don't think I could convince the triad to take it in 😄

[j4james]

If there is going to be any snapping, I think it should be on output, because that's what is expected from a VT standpoint, and is also what conhost always used to do.

I doubt anyone cares about horizontal snapping, because it's really annoying, and I doubt many people would be expecting that. But if you think it's a likely to generate complaints, I don't particular care what the default is. I'll add a mode for it one day anyway.

Edit: I realise now in my original reply I said "snap on input", when I was actually referring to what was essentially "snap on output" (the code you quoted).