Refactor copying query result to clipboard #25152
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@microsoft-github-policy-service agree |
Pull Request Test Coverage Report for Build 7342499286
💛 - Coveralls |
cheenamalhotra
approved these changes
Jan 2, 2024
Charles-Gagnon
approved these changes
Jan 2, 2024
| @@ -133,7 +133,7 @@ export async function copySelectionToClipboard(clipboardService: IClipboardServi | |||
| const getMessageText = (): string => { | |||
| return nls.localize('gridDataProvider.loadingRowsInProgress', "Loading the rows to be copied ({0}/{1})...", processedRows, rowCount); | |||
| }; | |||
| let resultString = ''; | |||
| let headerString = ''; | |||
There was a problem hiding this comment.
nit: Instead of making a whole separate var and copying that into the array you create below I'd suggest just creating the array here and push the header to it directly on line 145.
There was a problem hiding this comment.
Yeah, I guess I could do that but I wanted the logic of string-join to be separated from the logic of creating the data rows.
|
@serhat359 thanks for the PR, could you please update the PR description to include the perf improvements? e.g. rows, columns, time taken before, time taken after |
|
@alanrenmsft I added a table to the description that shows the performance improvements. |
|
@serhat359 thanks! |
alanrenmsft
approved these changes
Jan 8, 2024
|
When would this fix be merged to the main branch? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes the issue #24901
Makes copying table result to clipboard faster by using a StringBuilder (JS/TS implementation).
I've locally tested before and after versions of the function with different inputs to make sure the new function behaves the same as before.
You can see the performance results comparing before and after below. Each data row contains 4 columns ('foo', 'bar', 'baz', row index). SkipNewLine means the variable
shouldSkipNewLineAfterTrailingLineBreakconfig setting in the code.