Copy formatted code to clipboard #20242
Conversation
|
@rebornix, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @jrieken to be potential reviewers. |
|
@rebornix As for your point about large files, if this feature is put behind a different command (ie. not clubbed together with the default copy-paste behaviour) then I don't see any reason for it to be an issue. |
|
@hashhar yes that's what I'm working on right now. I just updated the pr and this time we already implemented option 1. What I'm going to do next is finding a magic number of content size. For any content larger than that, you need to run another command from command palette or context menu to copy rich text while if the content size is smaller than the number, then we have Copy Rich Text turned on. The idea about finding the magic number is making sure copying such content with styles won't lead to any noticeable latency (< 100ms ) on a normal machine (especially not my rMBP). |
rebornix
changed the title
|
It's ready for @alexandrudima 's review. The thing that I don't feel right is the Copy with Syntax Highlighting command, which forces the rich text rendering. As we bind handlers to browser's copy/paste event, there is no obvious way for a command to talk to the handler. What I did here is leveraging localstorage but I have a bad feeling about this. |
| let containsRTL = options.containsRTL; | ||
|
|
||
| let result = `<div>`; | ||
| const characterMapping = new CharacterMapping(text.length + 1, viewLineTokens.length); |
There was a problem hiding this comment.
No need to build up character mapping here
| } | ||
|
|
||
| const tokenType = token.type; | ||
| let partContentCnt = 0; |
There was a problem hiding this comment.
since there's no need to build up character mapping, there's also no need to keep track of partContentCnt
|
|
||
| let charIndex = options.startOffset; | ||
| let tabsCharDelta = 0; | ||
| let charOffsetInPart = 0; |
There was a problem hiding this comment.
since there's no need to build up character mapping, there's also no need to keep track of charOffsetInPart
| } | ||
|
|
||
| characterMapping.setPartLength(tokenIndex, partContentCnt); | ||
| let style = tokenType.split(' ').map(type => rules[type]).join(''); |
There was a problem hiding this comment.
please leave a todo here, we should clean this up once I merge the minimap branch where a view line token holds on to the metadata, it should be straight-forward with those changes to read here the font style and the foreground color directly
| characterMapping.setPartLength(tokenIndex, partContentCnt); | ||
| let style = tokenType.split(' ').map(type => rules[type]).join(''); | ||
| if (containsRTL) { | ||
| result += `<span dir="ltr" style="${style}">${partContent}</span>`; |
There was a problem hiding this comment.
perhaps this is overkill. I have a feeling the software where we paste should figure out by itself what to do with RTL characters, so I suggest to ignore RTL here for now.
| } | ||
|
|
||
| public getHTMLToCopy(ranges: Range[], enableEmptySelectionClipboard: boolean): string { | ||
| let rules: { [key: string]: string } = {}; |
There was a problem hiding this comment.
please leave a TODO here so we clean this up once the minimap branch gets merged in (same as above)
| return; | ||
| } | ||
|
|
||
| window.localStorage.setItem('forceCopyWithSyntaxHighlighting', 'true'); |
There was a problem hiding this comment.
very very nasty, especially for standalone editor embedders (other HTML pages outside vscode).
In such cases you can use a static, you can put it somewhere in editor/config or something...
| @@ -29,9 +29,12 @@ class ClipboardEventWrapper implements IClipboardEvent { | |||
| return false; | |||
| } | |||
|
|
|||
| public setTextData(text: string): void { | |||
| public setTextData(text: string, richText?: string): void { | |||
There was a problem hiding this comment.
I would use null for richText as a signal that it should not be set (instead of undefined + optional).
IMHO null is a lot more clear and prevents bugs where we forget by accident to add a second argument....
#3518.
Users are asking for this feature for a while and now it's the time to make it happen as @alexandrudima 's tokenization and syntax highlighting improvement https://code.visualstudio.com/blogs/2017/02/08/syntax-highlighting-optimizations makes everything easy.
Before his smart refactor, whenever we open an ordinary source code file, we have tens of CSS Class for each token and view line and we put like around hundreds of CSS entries in the output HTML. In this situation, if we want to send the formatted code or HTML snippet to the clipboard and makes it look reasonable, we have to do one of the followings
token meta function js definition parameters punctuationtocolor: #ffffff. This one looks more reasonable but it requires a good engine to take care of the CSS rules.At this moment, Alex already did all the complex CSS calculation for us in the improvement I mentioned above, the drawbacks of #1 and #2 are already gone now. For #1, we reduced the amount of CSS rules to 10 or 20 in most cases so the payload is not that giant. For #2, since the whole theme/tokenization engine is based on colors directly instead of CSS Classes, we just need to read the color from memory.
#2 is a better approach but as a POC, I still pick #2 then I can keep the code change minimal.
Performance
We are not relying on the browser(Chrome) to handle the copy/paste as we can't guarantee that content users select in the editor exist in the DOM tree. This is because we only render content that are visible to users for the sake of performance. If users select content out of the viewport then we have to pay the price of rendering the invisible part to get the DOM elements with styles.
You can imagine the worst case is that ppl open a large file and copy the whole content. As most of the lines are invisible and we have to render all of them when copying, it's very likely to see lagging. So I tested against the three files Alex played with in https://code.visualstudio.com/blogs/2017/02/08/syntax-highlighting-optimizations#_tokenization-times on my powerfull MacBook Pro (Retina, 15-inch, Mid 2015, 2.2 GHz Intel Core i7)
bootstrap.min.cssis good,checker.ts's lag is noticeable andsqlite3.cis bad. Butsqlite3.cis unusual and I bet you don't want to copy the content from Code and paste it to other applications. On my lovely machine, pasting the 6.73 MB content to Word takes 9 minutes.checker.tsis not an usual case I suppose as it has 22000+ lines and copying the whole content and pasting doesn't always happen. But I still put this feature behind a boolean option and set its default value as false to avoid annoying most users who don't necessarily need it.TODO
<style>tag in HTML entry in the clipboard. Word loves it but Evernote just ignores this element. Besides, browsers will transform all the classes to inline styles when you copy the content on a webpage, so maybe we should do the same thing. It requires some tweaks onrenderViewLine.TextandURLto the clipboard,HTMLis not supported according to https://msdn.microsoft.com/en-us/library/ms536744(v=vs.85).aspx