Replace WinRT clipboard API with Win32 for copying #17006
Conversation
lhecker
added
Issue-Bug
| } | ||
|
|
||
| return wil::unique_close_clipboard_call{ success }; | ||
| } |
There was a problem hiding this comment.
This is a copy of the function from TerminalPage. My hope is that we resolve the duplicate at a future time and that the linker will fold it away until then.
There was a problem hiding this comment.
FWIW, clipboard handling was kept outside of Control so that it could be customized by the owning app (e.g. visual studio putting it into local paste history or what-have-you). I am not against having copy behavior reside in the control, or in the UI-agnostic inner layers (however: clipboard IO requires an HWND…), but it is a significant departure from that goal.
There was a problem hiding this comment.
(Alas, the linker will not fold the copies away because they reside in different DLLs!)
There was a problem hiding this comment.
If we already support such usage of the API I'd be super happy to figure something out to make it work. However, if it was only a plan so far, we may IMHO be putting the cart before the horse: We can easily add clipboard customization at any time if needed, but we can't take it away. We should IMHO be as conservative as possible with exposing functionality via APIs as they will constrain our ability to quickly iterate on our code base and collapse unused/cumbersome functionality.
In this case not having it customizable makes the code simpler, but also more robust - even at the standard 120x9001 buffer size the HTML and RTF contents can easily be >40MB if the extra HSTRING conversions are used (3x overhead).
| Clipboard::Flush(); | ||
| } | ||
| CATCH_LOG(); | ||
| } |
There was a problem hiding this comment.
The Win32 APIs require the RTF/HTML contents to be ASCII/UTF8 which the TextBuffer already creates, but WinRT doesn't support. I really didn't want to have to convert such arbitrarily large strings from UTF8 > UTF16 > UTF8 in order to transmit them between classes. The conversion itself is not too slow, but the memory consumption for all that seemed concerning.
So I just moved the entire clipboard handling over into ControlCore.
There was a problem hiding this comment.
WinRT doesn't support
FWIW, WinRT supports it just fine - using IBuffer and IBufferByteAccess
There was a problem hiding this comment.
Looks good to me! Curious about the resolution to the API change discussion but I'm sure you two will figure it out.
| Clipboard::Flush(); | ||
| } | ||
| CATCH_LOG(); | ||
| } |
There was a problem hiding this comment.
WinRT doesn't support
FWIW, WinRT supports it just fine - using IBuffer and IBufferByteAccess
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
In the spirit of #15360 this implements the copy part. The problem is that we have an issue accessing the clipboard while other applications continue to work just fine. The major difference between us and the others is that we use the WinRT clipboard APIs. So, the idea is that we just use the Win32 APIs instead. The feel-good side-effect is that this is (no joke) 200-1000x faster, but I suspect no one will notice the -3ms difference down to <0.01ms. The objective effect however is that it just works. This may resolve #16982. * Cycle through Text/HTML/RTF-only in the Interaction settings * Paste the contents into Word each time * Text is plain and HTML/RTF are colored ✅ (cherry picked from commit 5f3a857) Service-Card-Id: 92308708 Service-Version: 1.20
In the spirit of #15360 this implements the copy part.
The problem is that we have an issue accessing the clipboard while
other applications continue to work just fine. The major difference
between us and the others is that we use the WinRT clipboard APIs.
So, the idea is that we just use the Win32 APIs instead.
The feel-good side-effect is that this is (no joke) 200-1000x faster,
but I suspect no one will notice the -3ms difference down to <0.01ms.
The objective effect however is that it just works.
This may resolve #16982.
Validation Steps Performed