-
Notifications
You must be signed in to change notification settings - Fork 8.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for paste filtering and bracketed paste mode #9034
Add support for paste filtering and bracketed paste mode #9034
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is why we need better AI 😅 |
I love this. Thank you 😀 |
|
||
_terminal->ClearSelection(); | ||
_terminal->TrySnapOnInput(); | ||
_terminal->WritePastedText(wstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very satisfying 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad! 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to keep the other terminal operations in this method to make WritePastedText more lightweight and decoupling from non-paste-related things. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure! I was wondering if the other two operations are something the Core should handle too.
@zadjii-msft if we want Core to be reusable, should it be given more responsibilities for figuring out when to clear the selection and snap on input? I feel like it should -- so maybe we should move the two lines under WritePastedText
into WritePastedText
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I agree, that should probably move down below too. That makes sense. Doesn't make sense to block this PR over it though, IMO. We could probably just get away with
_terminal->WritePastedText(wstr); | |
// TODO:projects/5 The calls to ClearSelection and TrySnapOnInput below could probably get moved inside Terminal::WritePastedText. | |
_terminal->WritePastedText(wstr); |
and have me come back to it 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh the bot automerged it. Whoops. I'm sure this whole area's about to be torn up anyways
We should only do this if more than one person asks for it. Jantari is the first; will there be a second?
Nope. Not unless somebody complains.
I'd like to handle this myself as part of the input stack change. Is that okay? 😄
Not a problem! I feel like you don't have too much work to do, given the above notes. Thanks again for doing this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks excellent to me. Thank you.
|
||
_terminal->ClearSelection(); | ||
_terminal->TrySnapOnInput(); | ||
_terminal->WritePastedText(wstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure! I was wondering if the other two operations are something the Core should handle too.
@zadjii-msft if we want Core to be reusable, should it be given more responsibilities for figuring out when to clear the selection and snap on input? I feel like it should -- so maybe we should move the two lines under WritePastedText
into WritePastedText
. What do you think?
filtered.append(L"\x1b[201~"); | ||
} | ||
|
||
if (_pfnWriteInput) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we don't need to do any of this work if there is no _pfnWriteInput
! 😁
::Microsoft::Console::Utils::FilterOption::ControlCodes; | ||
|
||
std::wstring filtered = ::Microsoft::Console::Utils::FilterStringForPaste(stringView, option); | ||
if (IsXtermBracketedPasteModeEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's an idea. Maybe to handle Jantari's case (since they want to paste literal control sequences), we can strip control codes only when bracketed paste mode is enabled.
@j4james, do you think that's a reasonable compromise? It solves the case where a clipboard payload can break out of bracketed paste and doesn't make unguarded paste any worse.
At the same time, I still can't see the value in pasting raw control codes here today in Common Era 2021, so...
The only time I ever did it was when I was actively trying to blow up bracketed paste in xterm/rxvt.
Otherwise, that's what ^V
is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, my vote is let's strip them always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to just strip them always, too. Also I don't like the idea of coupling "control codes filtering" and "bracketed paste mode". This is what @j4james told me specifically not to do in #7508 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I'm in agreement with you guys. I think it's preferable that they're always filtered.
@msftbot make sure @zadjii-msft signs off |
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems good to me. Not sure how many times we've reviewed it now haha. I'm not gonna block over a nit comment
|
||
_terminal->ClearSelection(); | ||
_terminal->TrySnapOnInput(); | ||
_terminal->WritePastedText(wstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I agree, that should probably move down below too. That makes sense. Doesn't make sense to block this PR over it though, IMO. We could probably just get away with
_terminal->WritePastedText(wstr); | |
// TODO:projects/5 The calls to ClearSelection and TrySnapOnInput below could probably get moved inside Terminal::WritePastedText. | |
_terminal->WritePastedText(wstr); |
and have me come back to it 😛
@DHowett Reading your comments regarding my list of future work just make me realize I really do not know ConPTY & conhost.exe related infrastructure that well 😅 . But I can sleep at night knowing it's in good hands of you & @zadjii-msft . |
🎉 Handy links: |
I was going to ask how we can disable the warning now that we have bracketed paste mode; good thing I searched first 😆 Answer is here for anyone else wondering. Documented here. |
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'` and confirmed 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
This adds "paste filtering" & "bracketed paste mode" to the Windows
Terminal.
I've moved the paste handling code in
TerminalControl
toMicrosoft::Console::Util
to be able to easily test it, and the pastetransformer from
TerminalControl
toTerminalCore
.Supersedes #7508
References #395 (overall bracketed paste support request)
Tests added. Manually tested.