Adding an overload to TextEditor.edit for insertion of a SnippetString #17628

Merged
merged 13 commits into from Jan 20, 2017

Projects

None yet

4 participants

@joelday
Contributor
joelday commented Dec 20, 2016

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

joelday added some commits Dec 16, 2016
@joelday joelday Adding insertSnippet to TextEditor extension API. 38a3a1a
@joelday joelday Fix hygiene error. 75840d1
@joelday joelday TextEditor.insertSnippet extension API.
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.
40522e2
@joelday joelday Merge branch 'master' of https://github.com/Microsoft/vscode
9c42a68
@msftclas

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
Contributor
joelday commented Dec 20, 2016 edited

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

@msftgits

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

@msftgits msftgits closed this Dec 20, 2016
@msftgits msftgits reopened this Dec 20, 2016
@msftclas

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;

@msftclas msftclas added the cla-signed label Dec 20, 2016
@joelday joelday Temporarily referencing vscode.proposed.d.ts for API tests.
a41b017
@jrieken jrieken was assigned by bpasero Dec 21, 2016
@jrieken jrieken added this to the January 2017 milestone Dec 21, 2016
@jrieken
Member
jrieken commented Jan 9, 2017

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

@joelday
Contributor
joelday commented Jan 9, 2017

@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
Contributor
joelday commented Jan 9, 2017

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

@jrieken
Member
jrieken commented Jan 10, 2017

I had left some review comments that require some changes

@joelday
Contributor
joelday commented Jan 10, 2017

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

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>;
+ }
@jrieken
jrieken Jan 11, 2017 Member

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

@joelday
joelday Jan 13, 2017 edited Contributor

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.

src/vs/vscode.d.ts
+ * @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.
+ */
+ insertSnippet(template: string, posOrRange: Position | Range): Thenable<boolean>;
@jrieken
jrieken Jan 11, 2017 Member
  • 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
@joelday
joelday Jan 13, 2017 Contributor

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?

@jrieken
jrieken Jan 14, 2017 Member

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.

public insertSnippet(template: string, overwriteBefore: number, overwriteAfter: number): void {
const snippet = CodeSnippet.fromTextmate(template, this._variableResolver);
this.run(snippet, overwriteBefore, overwriteAfter);
}
+ public insertSnippetWithReplaceRange(template: string, replaceRange: Range): void {
@jrieken
jrieken Jan 11, 2017 Member

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

@joelday
joelday Jan 13, 2017 Contributor

Agreed that this could be consolidated.

@jrieken
Member
jrieken commented Jan 11, 2017

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

@joelday
Contributor
joelday commented Jan 13, 2017

@jrieken Sorry for the late responses.

@joelday
Contributor
joelday commented Jan 17, 2017

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

joelday added some commits Jan 17, 2017
@joelday joelday Merge master.
8a30392
@joelday joelday Merge remote-tracking branch 'upstream/master' into insert-snippet-api f7587b3
@joelday joelday Insert snippet API changes. 3c817c7
@joelday joelday Merge remote-tracking branch 'upstream/master' into insert-snippet-api c27b386
@joelday joelday Fix test selection range.
713aaff
@joelday joelday TextEditor.edit snippet overload now returns void. No longer preventi…
…ng when already in snippet mode.
95fc032
@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

setEndOfLine: EndOfLine;
}
+export interface IInsertSnippetOptions extends IUndoStopOptions {
+
src/vs/vscode.proposed.d.ts
+ */
+ edit(snippet: SnippetString, options?: { undoStopBefore: boolean; undoStopAfter: boolean; }): void;
+
+ }
@jrieken
jrieken Jan 18, 2017 Member

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

@joelday
joelday Jan 18, 2017 Contributor

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
Contributor
joelday commented Jan 18, 2017

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

@joelday

@jrieken I'm thinking I should be null-checking _codeEditor here and returning false. Maybe I don't even need to focus it, but OTOH, it still needs to exist, right?

That is actually true. The _codeEditor reference can be null. This is because in the API an editor is a editor widget plus a document. Internally, they are both separate and the editor tracker basically tracks all documents and only when it is attached to an editor creates an 'api editor'. That why there also a check in the edit implementation

Owner
joelday replied Jan 18, 2017 edited

@jrieken How do you feel about switching back to Thenable<boolean> and basing false on that?

joelday added some commits Jan 18, 2017
@joelday joelday Snippet insertion extension API changes
- 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.
c21734f
@joelday joelday Reverting launch.json change.
ff2ea5d
@joelday
Contributor
joelday commented Jan 18, 2017

@jrieken Made a few changes. Thank you!

@@ -392,9 +388,13 @@ export class MainThreadTextEditor {
return false;
}
- insertSnippet(template: string, opts: IInsertSnippetOptions) {
+ insertSnippet(template: string, opts: IUndoStopOptions) {
@joelday
joelday Jan 18, 2017 Contributor

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

@joelday joelday changed the title from Adding an insertSnippet method to the TextEditor extension API. to Adding an overload to `TextEditor.edit` for insertion of a `SnippetString` Jan 18, 2017
@joelday joelday changed the title from Adding an overload to `TextEditor.edit` for insertion of a `SnippetString` to Adding an overload to TextEditor.edit for insertion of a SnippetString Jan 18, 2017
@jrieken
Member
jrieken commented Jan 20, 2017

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!

@jrieken jrieken merged commit b4817e5 into Microsoft:master Jan 20, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@joelday
Contributor
joelday commented Jan 20, 2017 edited

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment