Copy clipboard command in ADS (html/plain text supported) #13527

gupta1 · 2020-11-24T01:19:09Z

This PR fixes #
Adds a keyboard command in ADS to copy query and results in plain text as well as in html formatting. It will paste it to target location in the format supported i.e. word supports html, notepad supports plain text only.

UI for discoverability will be added later once we decide where in ADS we should add it.

Tested it for SQL and Kusto query editor. Screenshots of the feature and results are below:

Command:

Copied text in outlook:

Copied text in notepad:

coveralls · 2020-11-24T01:42:21Z

Pull Request Test Coverage Report for Build 384365367

0 of 0 changed or added relevant lines in 0 files are covered.
617 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.01%) to 41.252%

Files with Coverage Reduction	New Missed Lines	%
extensions/notebook/src/jupyter/serverInstance.ts	4	73.81%
src/sql/workbench/browser/modelComponents/modelStore.ts	12	48.08%
src/sql/workbench/contrib/notebook/browser/htmlMarkdownConverter.ts	12	78.93%
src/sql/workbench/browser/modelComponents/viewBase.ts	27	1.57%
extensions/resource-deployment/src/ui/deployClusterWizard/pages/serviceSettingsPage.ts	30	5.43%
extensions/azurecore/src/azureResource/commands.ts	48	0%
extensions/azurecore/src/azureResource/tree/flatAccountTreeNode.ts	70	0%
src/sql/workbench/api/common/extHostModelView.ts	118	35.61%
extensions/resource-deployment/src/ui/modelViewUtils.ts	296	4.2%

Totals
Change from base Build 380002068:	0.01%
Covered Lines:	21854
Relevant Lines:	48612

💛 - Coveralls

Charles-Gagnon · 2020-11-24T02:50:43Z

src/vs/platform/clipboard/browser/clipboardService.ts

@@ -13,6 +13,10 @@ export class BrowserClipboardService implements IClipboardService {

 	private readonly mapTextToType = new Map<string, string>(); // unsupported in web (only in-memory)

+	async write(data: any, type?: string): Promise<void> {


I don't see why this is necessary - but regardless make sure you add {{ SQL CARBON EDIT }} to all lines you modify in the vs dir

I have added write method to IClipboardService which supports copying plain text as well as html formatted text https://www.electronjs.org/docs/api/clipboard#clipboardwritedata-type
Currently BrowserClipboardService and NativeClipboardService both implements IClipboardService. For NativeClipboardService, I have implemented the above method whereas for BrowserClipboardService , it throws "Not Implemented" error. Another option is to only add write method in NativeClipboardService but not sure how it will affect ADS web for this command scenario. Let me know what you think would be the best way.

Gotcha - so basically for things that support HTML the clipboard will paste the HTML text otherwise it'll fall back to plain text? What's the behavior for things like Excel, Notepad, Outlook, etc?

Charles-Gagnon · 2020-11-24T02:50:47Z

src/sql/platform/clipboard/browser/clipboardService.ts

@@ -25,6 +25,10 @@ export class BrowserClipboardService implements IClipboardService {
 		this._notificationService.info(localize('imageCopyingNotSupported', "Copying images is not supported"));
 	}

+	write(data: string): Promise<void> {


Why are you adding this? Just use writeText

This should have the same type as the vs implementation (which should be the new type you add per my suggestion below)

And why isn't it calling vsClipboardService.write?

Charles-Gagnon · 2020-11-24T02:56:50Z

src/sql/workbench/contrib/query/browser/query.contribution.ts

+		CopyQueryWithResultsKeyboardAction,
+		CopyQueryWithResultsKeyboardAction.ID,
+		CopyQueryWithResultsKeyboardAction.LABEL,
+		{ primary: KeyMod.CtrlCmd | KeyCode.KEY_M }


This key binding is already used for RunCurrentQueryWithActualPlan https://github.com/microsoft/azuredatastudio/blob/main/src%2Fsql%2Fworkbench%2Fcontrib%2Fquery%2Fbrowser%2Fquery.contribution.ts#L132

please work with PM to define the shortcut key @gupta1

Sure, changing it to Ctrl+Alt+C for now since Ctrl+Shift+C which is used in Kusto Explorer is already used for something else in ADS. I can change it later if needed once @MsSQLGirl and Raden are back from vacation next week.

Charles-Gagnon

Make sure you add the SQL CARBON EDIT too

Charles-Gagnon · 2020-11-24T22:00:52Z

src/vs/platform/clipboard/browser/clipboardService.ts

@@ -13,6 +13,10 @@ export class BrowserClipboardService implements IClipboardService {

 	private readonly mapTextToType = new Map<string, string>(); // unsupported in web (only in-memory)

+	async write(data: any, type?: string): Promise<void> {


Gotcha - so basically for things that support HTML the clipboard will paste the HTML text otherwise it'll fall back to plain text? What's the behavior for things like Excel, Notepad, Outlook, etc?

Charles-Gagnon · 2020-11-24T22:02:19Z

src/sql/workbench/contrib/query/browser/keyboardQueryActions.ts

+
+			this._clipboardService.write(data);
+		}
+		return Promise.resolve(null);


You shouldn't be returning null - we use undefined in nearly all cases

The return type is void so you shouldn't be returning anything at all

Since this is an async function you can just remove this line and it'll work as expected

For the above comment related to Outlook, notepad behavior:
Yes correct. Excel, Outlook supports HTML text, it shows up with all the formatting whereas Notepad doesnt, so its just plain text.

Charles-Gagnon · 2020-11-24T22:06:15Z

src/vs/platform/clipboard/browser/clipboardService.ts

@@ -13,6 +13,10 @@ export class BrowserClipboardService implements IClipboardService {

 	private readonly mapTextToType = new Map<string, string>(); // unsupported in web (only in-memory)

+	async write(data: any, type?: string): Promise<void> {


This shouldn't be any type though - you should create a new type

export type ClipboardData = { { text?: string; html?: string; rtf?: string; bookmark?: string; }

and use that instead

Sure, makes sense. I will add it to clipboardService.ts

Charles-Gagnon · 2020-11-24T22:06:51Z

src/sql/workbench/contrib/query/browser/keyboardQueryActions.ts

+				html: `${escape(queryText).replace(/\r\n|\n|\r/gm, '<br />')}${allResults.html}`
+			};
+
+			this._clipboardService.write(data);


Charles-Gagnon · 2020-11-24T22:07:20Z

src/sql/workbench/contrib/query/browser/keyboardQueryActions.ts

+		@IQueryModelService protected readonly queryModelService: IQueryModelService,
+	) {
+		super(id, label);
+		this.enabled = true;


I wouldn't expect this to be necessary - what happens if you remove it?

I removed it initially but all other methods had it so wasn't sure if its required for some other settings I am not aware of. I can remove it.

Charles-Gagnon · 2020-11-24T22:10:09Z

src/sql/platform/clipboard/browser/clipboardService.ts

@@ -25,6 +25,10 @@ export class BrowserClipboardService implements IClipboardService {
 		this._notificationService.info(localize('imageCopyingNotSupported', "Copying images is not supported"));
 	}

+	write(data: string): Promise<void> {


This should have the same type as the vs implementation (which should be the new type you add per my suggestion below)

Charles-Gagnon · 2020-11-24T22:10:50Z

src/sql/platform/clipboard/browser/clipboardService.ts

@@ -25,6 +25,10 @@ export class BrowserClipboardService implements IClipboardService {
 		this._notificationService.info(localize('imageCopyingNotSupported', "Copying images is not supported"));
 	}

+	write(data: string): Promise<void> {


And why isn't it calling vsClipboardService.write?

Charles-Gagnon

Some cleanup but functionally looks fine

src/sql/workbench/contrib/query/browser/keyboardQueryActions.ts

src/vs/platform/clipboard/browser/clipboardService.ts

src/vs/platform/clipboard/common/clipboardService.ts

src/vs/platform/electron/common/electron.ts

src/vs/platform/electron/electron-main/electronMainService.ts

src/vs/workbench/services/clipboard/electron-sandbox/clipboardService.ts

src/vs/workbench/test/electron-browser/workbenchTestServices.ts

* draft commit * few changes * Changes to copy query with results in plain and html formatting * undo changes * undo unintended change * remove comments * Addressed comments * Some clean up Co-authored-by: Monica Gupta <mogupt@microsoft.com>

Monica Gupta added 8 commits October 30, 2020 10:10

draft commit

71ca37a

few changes

bf69c1b

Merge branch 'main' into copy_clipboard_kusto

021b593

Changes to copy query with results in plain and html formatting

e605508

Merge branch 'main' into copy_clipboard_kusto

57a7fad

undo changes

7be8c70

undo unintended change

bb176a1

remove comments

7ef2528

gupta1 requested a review from alanrenmsft November 24, 2020 01:26

Charles-Gagnon reviewed Nov 24, 2020

View reviewed changes

Charles-Gagnon requested changes Nov 24, 2020

View reviewed changes

Addressed comments

5540557

gupta1 requested a review from Charles-Gagnon November 24, 2020 23:16

gupta1 marked this pull request as ready for review November 24, 2020 23:20

gupta1 changed the title ~~Copy clipboard kusto~~ Copy clipboard command in ADS (html/plain text supported) Nov 24, 2020

alanrenmsft approved these changes Nov 24, 2020

View reviewed changes

Charles-Gagnon approved these changes Nov 25, 2020

View reviewed changes

Some clean up

feb51a8

gupta1 merged commit 397354e into main Nov 26, 2020

gupta1 deleted the copy_clipboard_kusto branch November 26, 2020 05:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy clipboard command in ADS (html/plain text supported) #13527

Copy clipboard command in ADS (html/plain text supported) #13527

gupta1 commented Nov 24, 2020 •

edited

Loading

coveralls commented Nov 24, 2020 •

edited

Loading

Charles-Gagnon Nov 24, 2020

gupta1 Nov 24, 2020 •

edited

Loading

Charles-Gagnon Nov 24, 2020 •

edited by gupta1

Loading

Charles-Gagnon Nov 24, 2020

Charles-Gagnon Nov 24, 2020

Charles-Gagnon Nov 24, 2020

Charles-Gagnon Nov 24, 2020

alanrenmsft Nov 24, 2020

gupta1 Nov 24, 2020

Charles-Gagnon left a comment

Charles-Gagnon Nov 24, 2020 •

edited by gupta1

Loading

Charles-Gagnon Nov 24, 2020

gupta1 Nov 24, 2020

Charles-Gagnon Nov 24, 2020 •

edited

Loading

gupta1 Nov 24, 2020

Charles-Gagnon Nov 24, 2020

Charles-Gagnon Nov 24, 2020

gupta1 Nov 24, 2020

Charles-Gagnon Nov 24, 2020

Charles-Gagnon Nov 24, 2020

Charles-Gagnon left a comment

		@@ -13,6 +13,10 @@ export class BrowserClipboardService implements IClipboardService {

		private readonly mapTextToType = new Map<string, string>(); // unsupported in web (only in-memory)

		async write(data: any, type?: string): Promise<void> {

Copy clipboard command in ADS (html/plain text supported) #13527

Copy clipboard command in ADS (html/plain text supported) #13527

Conversation

gupta1 commented Nov 24, 2020 • edited Loading

coveralls commented Nov 24, 2020 • edited Loading

Pull Request Test Coverage Report for Build 384365367

💛 - Coveralls

Choose a reason for hiding this comment

gupta1 Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Charles-Gagnon Nov 24, 2020 • edited by gupta1 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charles-Gagnon left a comment

Choose a reason for hiding this comment

Charles-Gagnon Nov 24, 2020 • edited by gupta1 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charles-Gagnon Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charles-Gagnon left a comment

Choose a reason for hiding this comment

gupta1 commented Nov 24, 2020 •

edited

Loading

coveralls commented Nov 24, 2020 •

edited

Loading

gupta1 Nov 24, 2020 •

edited

Loading

Charles-Gagnon Nov 24, 2020 •

edited by gupta1

Loading

Charles-Gagnon Nov 24, 2020 •

edited by gupta1

Loading

Charles-Gagnon Nov 24, 2020 •

edited

Loading