Adding an overload to TextEditor.edit for insertion of a SnippetString by joelday · Pull Request #17628 · microsoft/vscode

joelday · 2016-12-20T22:58:01Z

Implementation of #15952. Let me know if there's anything I can improve/fix or if the desired API and behavior should change. Thanks!

More robust type validation on ext side of insertSnippet. Position/range check for snippet insertion was inverted. Adding insertSnippet to vscode.d.ts. (Should it be in vscode.proposed.d.ts?) Adding extension API tests for insertSnippet.

msftclas · 2016-12-20T22:58:04Z

Hi @joelday, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

joelday · 2016-12-20T23:11:39Z

@jrieken Looks like vscode-api-tests fails because it relies on the published vscode.d.ts. Was manually replacing that during development.

msftgits · 2016-12-20T23:30:15Z

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

msftclas · 2016-12-20T23:30:22Z

Hi @joelday, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

jrieken · 2017-01-09T14:17:37Z

@joelday Ping? Are you still looking into this? Anything thing I can help you with?

joelday · 2017-01-09T17:57:07Z

@jrieken should work as-is. Unless the implementation needs changes or the test needs to be more thorough (I just realized that it should probably be checking for snippet mode), I'd guess that what is left is to sign off on the API and documentation language.

joelday · 2017-01-09T18:01:42Z

@jrieken unsure of the right way to resolve the test failures for unpublished changes to vscode.d.ts.

jrieken · 2017-01-10T08:14:33Z

I had left some review comments that require some changes

joelday · 2017-01-10T22:54:19Z

@jrieken Weird, I can't find them/didn't get any notifications.

jrieken · 2016-12-21T08:17:27Z

src/vs/vscode.proposed.d.ts

+		 * @return A promise that resolves with a value indicating if the snippet could be inserted.
+		 */
+		insertSnippet(template: string, posOrRange: Position | Range): Thenable<boolean>;
+	}


this isn't needed as we make the change directly in vscode.d.ts

I didn't think so, but the extensions project pulls from the publicly published vscode package instead of the vscode.d.ts in the main project. That's why I got the first build failure in the PR.

jrieken · 2016-12-21T08:21:30Z

src/vs/vscode.d.ts

+		 * @param template The snippet template to insert
+		 * @param posOrRange The position or replacement range representing the location of the insertion.
+		 * @return A promise that resolves with a value indicating if the snippet could be inserted.
+		 */


template should be of type SnippetString

Unsure about posOrRange - we should consider using the current editor selection/selections. That would allow to insert snippets in multiple locations at the same time. Also inserting snippets is highly interactive, so it's fair to use the current selection for that (or change the selection first).

We should consider making this an overload of the edit method

Agreed that you should be able to call this without being concerned with location. Sharing an argument for both position or range seems a little awkward as well. So long as I can alter the selection before starting. How should this work for multiple selections?

In the case of overloading TextEditor.edit, if that involves changes to TextEditorEdit, how does entering snippet mode fit in with subsequent insertions, replacements, etc.? It seems like this might have more in common with CompletionItem than editing.

Another thought: Should we supply a callback to know when snippet mode was exited?

How should this work for multiple selections?
As with a single selection, the editor API allows to set one or many selections. The snippet logic tho isn't yet smart enough to always honour multiple cursors when snippets are inserted. We have a issue for this and it will be implemented in the near future

how does entering snippet mode fit in with subsequent insertions, replacements, etc.? It seems like this might have more in common with CompletionItem than editing.

Sure, I was just wondering if reusing the name edit makes sense because that is sort of what we do. It doesn't have to fit in the builder but could just reuse the name, like so:

edit(snippet: SnippetString, options?:{/*undo stops control*/}): void; edit(callback: (editBuilder: TextEditorEdit) => void, options?: { undoStopBefore: boolean; undoStopAfter: boolean; }): Thenable<boolean>;

Another thought: Should we supply a callback to know when snippet mode was exited?

Not a fan because it really depends on the user tabbing through things to exit snippet mode.

jrieken · 2016-12-21T08:23:13Z

src/vs/editor/contrib/snippet/common/snippetController.ts

 		this.run(snippet, overwriteBefore, overwriteAfter);
 	}

+	public insertSnippetWithReplaceRange(template: string, replaceRange: Range): void {


We should try to get away with just using insertSnippet. The controller is already complex

Agreed that this could be consolidated.

jrieken · 2017-01-11T08:32:06Z

Oh, maybe I didn't press some buttons here... Should be in now

joelday · 2017-01-13T17:56:47Z

@jrieken Sorry for the late responses.

joelday · 2017-01-17T00:27:52Z

@jrieken I'll experiment with this tonight based on your feedback. Thanks!

…ng when already in snippet mode.

jrieken

Really good. Left some nit comments. I will also think about if edit should return a promise or not. Initially I was against it but maybe it should resolve as soon the snippet is inserted, e.g. the document is updated... Unsure yet

jrieken · 2017-01-18T10:32:49Z

src/vs/workbench/api/node/mainThreadEditorsTracker.ts

 }

+export interface IInsertSnippetOptions extends IUndoStopOptions {
+


jrieken · 2017-01-18T10:33:55Z

src/vs/vscode.proposed.d.ts

+		 */
+		edit(snippet: SnippetString, options?: { undoStopBefore: boolean; undoStopAfter: boolean; }): void;
+
+	}


It is enough to have it vscode.d.ts only. Duplication not needed

Should editor-api-tests be changed to point to the local vscode.d.ts instead of pulling it from https://raw.githubusercontent.com/Microsoft/vscode/master/src/vs/vscode.d.ts at build time?

joelday · 2017-01-18T16:10:53Z

@jrieken, I can have it return a promise again, just let me know.

- Basing snippet insertion failure on a new `_codeEditor` null-check. - Now returns `Thenable<boolean>`. - Removed vscode.proposed.d.ts copy of the `TextEditor` change. - Removing empty options interface.

joelday · 2017-01-18T18:17:39Z

@jrieken Made a few changes. Thank you!

joelday · 2017-01-18T18:22:48Z

src/vs/workbench/api/node/mainThreadEditorsTracker.ts

 	}

-	insertSnippet(template: string, opts: IInsertSnippetOptions) {
+	insertSnippet(template: string, opts: IUndoStopOptions) {


@jrieken Should console.warn('applyEdits on invisible editor'); be added to insertSnippet?

jrieken · 2017-01-20T09:52:30Z

I have fixed the compile issue with vscode-api-tests and this is now ready to be merged. Thanks for your contribution and for the patience!

joelday · 2017-01-20T17:22:52Z

@jrieken I'm really excited about having made this kind of contribution! The surface of external APIs is a pretty big commitment. This is a huge win for joelday.docthis and extensions like it. Thank you for the critical eye.

joelday and others added 4 commits December 16, 2016 00:50

Adding insertSnippet to TextEditor extension API.

38a3a1a

Fix hygiene error.

75840d1

TextEditor.insertSnippet extension API.

40522e2

More robust type validation on ext side of insertSnippet. Position/range check for snippet insertion was inverted. Adding insertSnippet to vscode.d.ts. (Should it be in vscode.proposed.d.ts?) Adding extension API tests for insertSnippet.

Merge branch 'master' of https://github.com/Microsoft/vscode

9c42a68

msftclas added the cla-required label Dec 20, 2016

msftgits closed this Dec 20, 2016

msftgits reopened this Dec 20, 2016

msftclas added the cla-signed label Dec 20, 2016

Temporarily referencing vscode.proposed.d.ts for API tests.

a41b017

bpasero assigned jrieken Dec 21, 2016

jrieken added this to the January 2017 milestone Dec 21, 2016

jrieken reviewed Jan 11, 2017

View reviewed changes

icfjoeld and others added 6 commits January 17, 2017 13:18

Merge master.

8a30392

Merge remote-tracking branch 'upstream/master' into insert-snippet-api

f7587b3

Insert snippet API changes.

3c817c7

Merge remote-tracking branch 'upstream/master' into insert-snippet-api

c27b386

Fix test selection range.

713aaff

TextEditor.edit snippet overload now returns void. No longer preventi…

95fc032

…ng when already in snippet mode.

jrieken reviewed Jan 18, 2017

View reviewed changes

icfjoeld added 2 commits January 18, 2017 10:13

Snippet insertion extension API changes

c21734f

- Basing snippet insertion failure on a new `_codeEditor` null-check. - Now returns `Thenable<boolean>`. - Removed vscode.proposed.d.ts copy of the `TextEditor` change. - Removing empty options interface.

Reverting launch.json change.

ff2ea5d

joelday commented Jan 18, 2017

View reviewed changes

joelday changed the title ~~Adding an insertSnippet method to the TextEditor extension API.~~ Adding an overload to TextEditor.edit for insertion of a SnippetString Jan 18, 2017

joelday changed the title ~~Adding an overload to TextEditor.edit for insertion of a SnippetString~~ Adding an overload to TextEditor.edit for insertion of a SnippetString Jan 18, 2017

jrieken merged commit b4817e5 into microsoft:master Jan 20, 2017

jchannon mentioned this pull request Jan 23, 2017

text editor insert snippet API #15952

Closed

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

		}

		export interface IInsertSnippetOptions extends IUndoStopOptions {

Conversation

joelday commented Dec 20, 2016

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

joelday commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftgits commented Dec 20, 2016

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

jrieken commented Jan 9, 2017

Uh oh!

joelday commented Jan 9, 2017

Uh oh!

joelday commented Jan 9, 2017

Uh oh!

jrieken commented Jan 10, 2017

Uh oh!

joelday commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelday Jan 13, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Jan 11, 2017

Uh oh!

joelday commented Jan 13, 2017

Uh oh!

joelday commented Jan 17, 2017

Uh oh!

jrieken 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!

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!

joelday commented Jan 18, 2017

Uh oh!

joelday commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Jan 20, 2017

Uh oh!

joelday commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joelday commented Dec 20, 2016 •

edited

Loading

joelday Jan 13, 2017 •

edited

Loading

jrieken left a comment •

edited

Loading

joelday commented Jan 20, 2017 •

edited

Loading