[1.19] Shell integration marks are totally in the wrong place

[zadjii-msft]

This was on
Windows Terminal Canary
Version: 1.19.2643.0

Looks like the marks are like... off by one?

Possible culprits:

• Improve performance of scrollbar marks #16006
• ConPTY: Fix missing flush on console mode changes #15991 - 41f7ed7 - Nope, just reverting that didn't work.
• ConPTY: Avoid WINDOW_BUFFER_SIZE_EVENT when the viewport moves #15935 - 741633e - Nope, not this one either
• Reimplement TextBuffer::Reflow #15701 - 7474839 not this one

[zadjii-msft]

Looks like this is also busted in v1.19.2652.0

[zadjii-msft]

oh no, scrolling is really badly messed up

[zadjii-msft]

Bisecting. Starting by looking for a good commit around

❔ ✅ ❌

• ❌ 198c11f main
• ...
• ❔ 2e91e9c Display connection state in tab and add context menu entry for restarting connection ( Display connection state in tab and add context menu entry for restarting connection #15760)
• ...
• ✅ c5e0908 Update cmd.exe FAQ (Update cmd.exe FAQ #15918)

[zadjii-msft]

4ddfc3e is the first bad commit
commit 4ddfc3e

COOKED_READ_DATA: Fix scrolling under ConPTY (#15930)

[zadjii-msft]

Okay this is 100% due to:

terminal/src/host/_stream.cpp

Lines 79 to 93 in cc2ba53

	// ConPTY translates scrolling into newlines. We don't want that during cooked reads (= "cmd.exe prompts")
	// because the entire prompt is supposed to fit into the VT viewport, with no scrollback. If we didn't do that,
	// any prompt larger than the viewport will cause >1 lines to be added to scrollback, even if typing backspaces.
	// You can test this by removing this branch, launch Windows Terminal, and fill the entire viewport with text in cmd.exe.
	if (needsConPTYWorkaround)
	{
	buffer.SetAsActiveBuffer(false);
	buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes());
	buffer.SetAsActiveBuffer(isActiveBuffer);
	if (isActiveBuffer && renderer)
	{
	renderer->TriggerRedrawAll();
	}
	}

@lhecker how load bearing is that? I know that comment sounds super scary above it, but manually setting needsConPTYWorkaround to false definitely fixes the scrolling and marks issue.

[lhecker]

I'll fix #16044 and this issue in a single PR.
For #16044 I'll make it so that appending text only reprints the minimum amount of text that's necessary and for this issue I'll revert the above code entirely.

After all that, I'll reopen #15899 and leave it unfixed for the foreseeable future, because as much of a significant bug it is, I feel like fixing it is too difficult for the impact it has. To properly fix it, it always either required us to sacrifice on our i18n or to implement a pager, both of which are a pain to do. We have other issues that are much easier to fix and are hit much more often, right?
And if we don't reprint the entire prompt when appending text, I don't think it'll be hit very often. After all, cmd.exe always reprinted the entire prompt when inserting in the middle...

[zadjii-msft]

Totally cool with that. Tossing this one over to you then ☺️