implement browser clipboard service by sbatten · Pull Request #75293 · microsoft/vscode

sbatten · 2019-06-11T18:59:55Z

No description provided.

bpasero

@sbatten can you please revisit where you changed methods to return a Promise and forgot to add an await, I found at least https://github.com/microsoft/vscode/pull/75293/files#diff-058cba421d6dc4a91f615f8a9e35adb8R553

Also, are we certain navigator.clipboard works as advertised? I think it can be undefined in certain cases so we might need to probe for that and install a workaround (see https://stackoverflow.com/questions/51805395/navigator-clipboard-is-undefined).

https://caniuse.com/#search=clipboard seems to indicate that the clipboard API is only available on user interactions (e.g. click), though not sure that still holds true.

Maybe @alexandrudima and @rebornix could help out here, given we seem to have some way of clipboard support (via https://github.com/Microsoft/vscode/blob/39947c72507deb8a44de785825243b9da30a7a19/src/vs/editor/contrib/clipboard/clipboard.ts#L22) in the form of document.execCommand).

bpasero · 2019-06-12T05:34:56Z

+	async provideTextContent(resource: URI): Promise<ITextModel> {
+		const model = this.modelService.createModel(await this.clipboardService.readText(), this.modeService.createByFilepathOrFirstLine(resource), resource);

 		return Promise.resolve(model);


Suggested change

return Promise.resolve(model);

return model;

bpasero · 2019-06-12T05:35:34Z

+
+export class ClipboardService implements IClipboardService {
+
+	_serviceBrand: any;


Suggested change

_serviceBrand: any;

_serviceBrand: ServiceIdentifier<IClipboardService>;

bpasero · 2019-06-12T05:37:48Z

+	async resolve(variable: Variable): Promise<string | undefined> {
 		for (const delegate of this._delegates) {
 			let value = delegate.resolve(variable);
 			if (value !== undefined) {


@sbatten this is broken now, you have to await the value, no?

rebornix · 2019-06-12T06:08:37Z

The last time when I modified Monaco’s clipboard service related code, we can access the clipboard only in events handler of cut, copy and paste but not navigator.clipboard . @sbatten we can take a look together.

rebornix · 2019-06-12T17:18:27Z

Re https://developer.mozilla.org/en-US/docs/Web/API/Clipboard

Calls to the methods of the Clipboard object will not succeed if the user hasn't granted the needed permissions using the Permissions API and the "clipboard-read" or "clipboard-write" permission as appropriate.

I'm wondering if we should use document.execCommand by default if we can do that (say there is a copy event handler registered for the focused element, or it's an input/textarea/etc), and then fall back to navigator.clipboard. We don't want to bother with permission access when it's not necessary.

roblourens · 2019-06-14T00:14:02Z

-				this.clipboardService.writeText(result.body.result);
-			}, err => this.clipboardService.writeText(this.value.value));
+				return this.clipboardService.writeText(result.body.result);
+			}, err => { return this.clipboardService.writeText(this.value.value); });


The err change is redundant, does the same as the original.

roblourens · 2019-06-14T00:14:50Z

 		actions.push(new Action('workbench.debug.action.copyAll', nls.localize('copyAll', "Copy All"), undefined, true, () => {
-			this.clipboardService.writeText(this.getVisibleContent());
-			return Promise.resolve(undefined);
+			return Promise.resolve(this.clipboardService.writeText(this.getVisibleContent()));


Seems like we should use async vs promises consistently in the same file at least, the change above this uses async

roblourens · 2019-06-14T00:15:26Z

 			enabled: true,
 			id: KEYBINDINGS_EDITOR_COMMAND_COPY,
-			run: () => this.copyKeybinding(keybindingItem)
+			run: () => { return this.copyKeybinding(keybindingItem); }


Same as above, () => foo returns foo

sbatten · 2019-06-14T18:43:18Z

@bpasero updated with feedback. The link for caniuse is referring to the older synchronous clipboard API. The new API is async but not well supported across browsers. It does what we need which is a method to read and write arbitrary text to the clipboard. After discussing with @rebornix, this is what the service is responsible for while general copy, cut, paste are handled separately using the synchronous API. For me, this means our service does the best it can with the async API and throws when the browser does not support this. We cannot fallback to the synchronous API since there is no meaning to copy text that isn't selected when an extension asks to write text to the clipboard.

bpasero

@sbatten I mentioned this before, you changed methods to return a Promise but you are not awaiting them properly. E.g. Variable.resolve():

resolveVariables(resolver: VariableResolver): this {
	this.walk(candidate => {
		if (candidate instanceof Variable) {
			if (candidate.resolve(resolver)) {
				this._placeholders = undefined;
			}
		}
		return true;
	});
	return this;
}

sbatten · 2019-06-18T01:01:09Z

@bpasero yea i thought i got all these with 37e5c97 but as I continue, this is exploding more and more. The assumption of a synchronous clipboard is very pervasive. If I continue this way, I am going to have to write some tooling to do the conversion as it is becoming very difficult to keep track of what's changed and what needs to change. It does seem that the explosion roots from the clipboard based snippets however, so maybe there is a way to reduce the impact by modifying that a bit.

bpasero · 2019-06-18T05:44:02Z

I am not sure I can follow. There shouldn't be a ton of users of clipboard service, or? And I would assume that in many cases the usage of the clipboard service is one of the last things in the code (e.g. as a result of an action), so doing an await... should be no problem?

sbatten · 2019-06-18T08:00:38Z

@bpasero yes clipboard service usage is low but once that new location's function becomes async, this permeates, and where it is changed, the value is actually needed from readText. I'm looking into splitting some function calls to async and sync versions to limit the impact to the primary code path that is the problem (clipboard based snippets)

bpasero · 2019-06-18T09:26:25Z

@sbatten oh now I see what you mean: if someone calls writeText and someone readText without waiting, you might end up reading stale values?

I think the only solution here is to add a Barrier that synchronizes read/write access. We have utility classes for this in async.ts

sbatten · 2019-06-18T17:56:39Z

@bpasero while that may also be an issue, it wasn't exactly to what I am referring.

I'm referring to the following:

function A() {
 const val = B();
 return val !== undefined;
}

function B() {
 return true;
}

In the above example, imagine B becomes async. The same would need to happen to A because it needs to await the value of B. B may only have one reference in the whole code base. But A has N, C_1 ... C_N. If we iterate on this process, it explodes quickly as C_x can also have many references. Further, even with a linting rule applied, this case is not detected because we store the promise of B so we don't know it is floating. So, finding all references and updating has been the approach so far but like I said, one changes brings 10 others. Eventually this will end, but its a dangerous change. The result is me looking into a smaller more local refactor of snippets or tooling to make sure the larger change is safe.

bpasero · 2019-06-18T18:27:57Z

@sbatten yes both things are valid:

we need to sync on reading and writing
we need to change all methods that read/write clipboard to become async at least if the caller relies on the result of the call. I think we can assume that this is only the case for reading but not writing

sbatten · 2019-06-19T00:26:41Z

@bpasero

we need to change all methods that read/write clipboard to become async at least if the caller relies on the result of the call. I think we can assume that this is only the case for reading but not writing

agree and since this is becoming a dangerously large and unvalidate-able change, I want to minimize it or use some smarter tooling to detect issues. for context I have another incomplete local commit with another 10-20 files that have to change on top of what's here.

bpasero · 2019-06-19T05:31:43Z

Ok, just add it and I can review again.

roblourens · 2019-06-20T01:18:27Z

 	id: 'acceptSelectedSuggestion',
 	precondition: SuggestContext.Visible,
-	handler: x => x.acceptSelectedSuggestion(true),
+	handler: async (x) => { await x.acceptSelectedSuggestion(true); },


Functions like this don't need to use async/await, the fat arrow function is already returning the promise

But it's also fine to write it this way

@roblourens Yea you're right. Originally, I was planning to make them all with async/await for clarity reasons, but I'm not sure I was consistent in that approach.

sbatten · 2019-06-20T01:43:16Z

@bpasero finished all updates... I hope. Ran unit tests to ensure no changes, caught one missed await in the test itself.

bpasero · 2019-06-24T10:32:23Z

Adding @jrieken for changes in snippets land and @Tyriar for terminal.

jrieken

Sorry, this needs a different approach for snippets et al

jrieken · 2019-06-24T10:39:14Z

 	}

-	insert(
+	async insert(


🛑 this cannot be async, never. Subsequently all following async changes cannot happen.

Making snippets insert async, means that insert suggestion become async and that means you can accept a suggestion, type, and get the suggestion insert after that. This needs a different approach, e.g the snippet completion provider has the change to read the clipboard at the time of computing snippets (async is OK there)

bpasero

@sbatten a couple of feedback (outside of snippets/terminal):

you do not need to ever use Promise.resolve() if the thing is a promise you return, I noticed you are using it in a couple of places in your change
startupProfiler: suggest to convert this to use async and not a mix of async+then

Tyriar

Terminal 👌

sbatten · 2019-07-10T22:38:00Z

@jrieken updated with your changes to remove nearly all changes to snippets code. please verify

@bpasero updated to address your feedback

implement browser clipboard service

09179c2

sbatten requested a review from bpasero June 11, 2019 18:59

sbatten self-assigned this Jun 11, 2019

sbatten added 2 commits June 11, 2019 19:50

Merge remote-tracking branch 'microsoft/master' into clipboardService

4fc0511

actually register the clipboard service

7dd5e0f

bpasero requested changes Jun 12, 2019

View reviewed changes

sbatten added 3 commits June 13, 2019 15:47

Merge branch 'master' into clipboardService

b986be8

update all refs to newly async funcs

37e5c97

addressing comments

a0905da

roblourens reviewed Jun 14, 2019

View reviewed changes

small fixes

a950a63

bpasero requested changes Jun 15, 2019

View reviewed changes

Merge branch 'master' into clipboardService

842547e

bpasero added the web Issues related to running VSCode in the web label Jun 18, 2019

sbatten added 3 commits June 19, 2019 19:39

Merge branch 'master' into clipboardService

675bd33

all asyncs as a result of readText

de93eeb

fix unit test

4179152

roblourens reviewed Jun 20, 2019

View reviewed changes

cleanup for writeText refs

41eb54f

bpasero requested review from bpasero and jrieken June 24, 2019 06:38

bpasero requested review from Tyriar and roblourens June 24, 2019 10:32

jrieken requested changes Jun 24, 2019

View reviewed changes

bpasero approved these changes Jun 24, 2019

View reviewed changes

Tyriar approved these changes Jun 24, 2019

View reviewed changes

Merge remote-tracking branch 'microsoft/master' into clipboardService

557bd91

jrieken mentioned this pull request Jul 8, 2019

Support async clipboard reading in snippets world #76847

Closed

sbatten added 4 commits July 10, 2019 22:11

merge conflicts

75dc05d

cleanup and reduce diff noise

e88fada

qfix

8d825f6

address feedback

4ce26f2

sbatten requested review from bpasero and jrieken July 10, 2019 22:38

jrieken approved these changes Jul 11, 2019

View reviewed changes

bpasero merged commit 305d5be into microsoft:master Jul 11, 2019

bpasero added this to the July 2019 milestone Jul 11, 2019

github-actions bot locked and limited conversation to collaborators Mar 28, 2020


		export class ClipboardService implements IClipboardService {

		_serviceBrand: any;

	_serviceBrand: any;
	_serviceBrand: ServiceIdentifier<IClipboardService>;

Conversation

sbatten commented Jun 11, 2019

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebornix commented Jun 12, 2019

Uh oh!

rebornix commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbatten commented Jun 14, 2019

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

sbatten commented Jun 18, 2019

Uh oh!

bpasero commented Jun 18, 2019

Uh oh!

sbatten commented Jun 18, 2019

Uh oh!

bpasero commented Jun 18, 2019

Uh oh!

sbatten commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero commented Jun 18, 2019

Uh oh!

sbatten commented Jun 19, 2019

Uh oh!

bpasero commented Jun 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbatten commented Jun 20, 2019

Uh oh!

bpasero commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrieken left a comment

Choose a reason for hiding this comment

Uh oh!

jrieken Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

sbatten commented Jul 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sbatten commented Jun 18, 2019 •

edited

Loading

bpasero commented Jun 24, 2019 •

edited

Loading

jrieken Jun 24, 2019 •

edited

Loading

bpasero left a comment •

edited

Loading