Add support for bracketed paste mode in ConHost #15155
Conversation
microsoft-github-policy-service
bot
added
Issue-Feature
| CPINFO CPInfo; | ||
| CPINFO OutputCPInfo; | ||
| CPINFO CPInfo = {}; | ||
| CPINFO OutputCPInfo = {}; |
| { | ||
| THROW_HR_IF_NULL(E_INVALIDARG, pData); | ||
|
|
||
| std::deque<std::unique_ptr<IInputEvent>> keyEvents; | ||
| const auto pushControlSequence = [&](const std::wstring_view sequence) { | ||
| std::for_each(sequence.begin(), sequence.end(), [&](const auto wch) { |
There was a problem hiding this comment.
A range-for should be simpler I think.
There was a problem hiding this comment.
The main reason I used std::for_each was so I could avoid having to put the { on a new line, which I really loathe. I know that's petty. But I did also check out the generated assembly, and std::for_each actually seemed to be a little more efficient - at least fewer instructions - so I figured that might count as a more rational justification.
That said, it's not a big deal to change if you really don't like it this way.
There was a problem hiding this comment.
The main reason I used
std::for_eachwas so I could avoid having to put the{on a new line, which I really loathe. I know that's petty.
If you knew. 🥲 K&R gang 4 life.
But I did also check out the generated assembly, and
std::for_eachactually seemed to be a little more efficient - at least fewer instructions - so I figured that might count as a more rational justification.
Oh interesting. I was always under the impression that compilers produce better assembly for range-for loops if they can infer the bounds easily (like for span/string_view). At least in my little test it produces the same assembly though: https://godbolt.org/z/crj8aM1qr (it's the function next to the pushControlSequence$ = 96). BTW you can also spot the 4 SSE2 movaps/movdqa there, which exist due to taking the string-view by-copy. It's quite sad to see that the latest MSVC actually still hasn't fixed this issue... The movaps/movdqa pairs are entirely redundant, because all it does is copy from one spot on the stack to another. Just read the damn original value! 😄 The movaps/movdqa sequence has a latency of like 10-15 cycles.
That said, it's not a big deal to change if you really don't like it this way.
No, please feel free to keep it (unless anyone else insists on it of course). Although you might be interested in using std::ranges::for_each(sequence, ...) (produces the same assembly, but it's more template instantiations unfortunately).
There was a problem hiding this comment.
K&R gang 4 life.
Absolutely!
At least in my little test it produces the same assembly though: https://godbolt.org/z/crj8aM1qr
That is interesting. That looks nothing like the assembly I'm seeing generated in Visual Studio. I don't think my version of VS is that out of date (the C/C++ Compiler is 19.34.31937), but maybe it's just the other code in the method that alters the way those lambdas are optimized.
Not that I'm seeing a huge difference between the two anyway. I just found it interesting that the std::for_each seemed a little better. I was concerned it would be worse.
Although you might be interested in using
std::ranges::for_each(sequence, ...)
Yes, I was hoping there was something like that, so I'm glad you brought that up. Unfortunately it also introduces additional overhead in the generated assembly. For the most part it looks almost identical to the std::for_each version, but for some reason it adds a security_cookie, and an associated security_check_cookie call on exit. That's really annoying.
There was a problem hiding this comment.
On looking at your godbolt link again, I realised now why it didn't look anything like what I was expecting. The function immediately below pushControlSequence$ = 96 (which is identical for both versions), is just the code that calls the pushControlSequence lambda. I think the lambda itself is further below that (following sequence$ = 88 in the one source, and sequence$ = 104 in the other).
In godbolt those too lambdas are massively different, but that's mostly because the one seems to have partially inlined the push_back calls. I don't get that inlining in VS for either of them, but the general structure is similar to what I'm seeing. The differences I see in VS are things like the one having more registers saved on the stack, and extra register loads in the body of the loop.
| COOKED_READ_DATA* _cookedReadData; // non-ownership pointer | ||
| SCREEN_INFORMATION* pCurrentScreenBuffer = nullptr; | ||
| COOKED_READ_DATA* _cookedReadData = nullptr; // non-ownership pointer | ||
| bool _bracketedPasteMode = false; |
There was a problem hiding this comment.
I am so glad you made this change. Thanks. I mean, all of the initializers.
This adds support for XTerm's "bracketed paste" mode in ConHost. When
enabled, any pasted text is bracketed with a pair of escape sequences,
which lets the receiving application know that the content was pasted
rather than typed.
References and Relevant Issues
Bracketed paste mode was added to Windows Terminal in PR #9034.
Adding it to ConHost ticks one more item off the list in #13408.
Detailed Description of the Pull Request / Additional comments
This only applies when VT input mode is enabled, since that is the way
Windows Terminal currently works.
When it comes to filtering, though, the only change I've made is to
filter out the escape character, and only when bracketed mode is
enabled. That's necessary to prevent any attempts to bypass the
bracketing, but I didn't want to mess with the expected behavior for
legacy apps if bracketed mode is disabled.
Validation Steps Performed
Manually tested in bash with
bind 'set enable-bracketed-paste on'andconfirmed that pasted content is now buffered, instead of being executed
immediately.
Also tested in VIM, and confirmed that you can now paste preformatted
code without the autoindent breaking the formatting.
Closes #395