Snippets: Bug fix - placeholder ordering (#1722)

* Fix placeholder ordering

* Get tests green

* Remove unnecessary image

* Fix lint issues

browser/src/Services/Snippets/OniSnippet.ts

@@ -23,6 +23,8 @@ export interface OniSnippetPlaceholder {
     character: number
 
     value: string
+
+    isFinalTabstop: boolean
 }
 
 export const getLineCharacterFromOffset = (
@@ -74,6 +76,7 @@ export class OniSnippet {
                 ...position,
                 index: p.index,
                 value: p.toString(),
+                isFinalTabstop: p.isFinalTabstop,
             }
         })
 

browser/src/Services/Snippets/SnippetSession.ts

@@ -24,15 +24,15 @@ export const splitLineAtPosition = (line: string, position: number): [string, st
     return [prefix, post]
 }
 
-export const getSmallestPlaceholder = (
+export const getFirstPlaceholder = (
     placeholders: OniSnippetPlaceholder[],
 ): OniSnippetPlaceholder => {
     return placeholders.reduce((prev: OniSnippetPlaceholder, curr: OniSnippetPlaceholder) => {
-        if (!prev) {
+        if (!prev || prev.isFinalTabstop) {
             return curr
         }
 
-        if (curr.index < prev.index) {
+        if (curr.index < prev.index && !curr.isFinalTabstop) {
             return curr
         }
         return prev
@@ -52,6 +52,18 @@ export const getPlaceholderByIndex = (
     return matchingPlaceholders[0]
 }
 
+export const getFinalPlaceholder = (
+    placeholders: OniSnippetPlaceholder[],
+): OniSnippetPlaceholder | null => {
+    const matchingPlaceholders = placeholders.filter(p => p.isFinalTabstop)
+
+    if (matchingPlaceholders.length === 0) {
+        return null
+    }
+
+    return matchingPlaceholders[0]
+}
+
 export interface IMirrorCursorUpdateEvent {
     mode: Oni.Vim.Mode
     cursors: types.Range[]
@@ -146,14 +158,19 @@ export class SnippetSession {
         const placeholders = this._snippet.getPlaceholders()
 
         if (!this._currentPlaceholder) {
-            const newPlaceholder = getSmallestPlaceholder(placeholders)
+            const newPlaceholder = getFirstPlaceholder(placeholders)
             this._currentPlaceholder = newPlaceholder
         } else {
+            if (this._currentPlaceholder.isFinalTabstop) {
+                this._finish()
+                return
+            }
+
             const nextPlaceholder = getPlaceholderByIndex(
                 placeholders,
                 this._currentPlaceholder.index + 1,
             )
-            this._currentPlaceholder = nextPlaceholder || getSmallestPlaceholder(placeholders)
+            this._currentPlaceholder = nextPlaceholder || getFinalPlaceholder(placeholders)
         }
 
         await this._highlightPlaceholder(this._currentPlaceholder)
@@ -166,7 +183,7 @@ export class SnippetSession {
             placeholders,
             this._currentPlaceholder.index - 1,
         )
-        this._currentPlaceholder = nextPlaceholder || getSmallestPlaceholder(placeholders)
+        this._currentPlaceholder = nextPlaceholder || getFirstPlaceholder(placeholders)
 
         await this._highlightPlaceholder(this._currentPlaceholder)
     }

browser/test/Mocks/index.ts

@@ -163,6 +163,10 @@ export class MockEditor extends Editor {
         return this._currentSelection
     }
 
+    public async clearSelection(): Promise<void> {
+        // tslint:disable-line
+    }
+
     public setActiveBufferLine(line: number, lineContents: string): void {
         this._activeBuffer.setLineSync(line, lineContents)
 

browser/test/Services/Snippets/OniSnippetTests.ts

@@ -50,6 +50,7 @@ describe("OniSnippet", () => {
                 line: 0,
                 character: 3,
                 value: "index",
+                isFinalTabstop: false,
             })
         })
 
@@ -63,6 +64,7 @@ describe("OniSnippet", () => {
                 line: 0,
                 character: 3,
                 value: "a",
+                isFinalTabstop: false,
             })
 
             assert.deepEqual(placeholders[1], {
@@ -70,6 +72,7 @@ describe("OniSnippet", () => {
                 line: 1,
                 character: 3,
                 value: "b",
+                isFinalTabstop: false,
             })
         })
 
@@ -82,6 +85,7 @@ describe("OniSnippet", () => {
                 line: 1,
                 character: 3,
                 value: "",
+                isFinalTabstop: true,
             })
         })
     })

browser/test/Services/Snippets/SnippetSessionTests.ts

@@ -113,7 +113,7 @@ describe("SnippetSession", () => {
 
     describe("next placeholder", () => {
         it("highlights correct placeholder after calling nextPlaceholder", async () => {
-            snippetSession = new SnippetSession(mockEditor as any, "${0:test} ${1:test2}")
+            snippetSession = new SnippetSession(mockEditor as any, "${1:test} ${0:test2}")
 
             await snippetSession.start()
 
@@ -128,7 +128,7 @@ describe("SnippetSession", () => {
         })
 
         it("traverses order correctly, when placeholders are reversed", async () => {
-            snippetSession = new SnippetSession(mockEditor as any, "${1:test} ${0:test2}")
+            snippetSession = new SnippetSession(mockEditor as any, "${0:test} ${1:test2}")
 
             await snippetSession.start()
 
@@ -154,25 +154,30 @@ describe("SnippetSession", () => {
             snippetSession = new SnippetSession(mockEditor as any, "${1:test} ${0:test2} ${1}")
 
             const placeholder0Range = types.Range.create(0, 5, 0, 9)
-            const placeholder1Range = types.Range.create(0, 0, 0, 3)
 
-            await snippetSession.start() // Placeholder 0
-            await snippetSession.nextPlaceholder() // Placeholder 1
+            await snippetSession.start() // Placeholder 1
+            await snippetSession.nextPlaceholder() // Placeholder 0
 
-            let selection = await mockEditor.getSelection()
+            const selection = await mockEditor.getSelection()
 
-            assert.strictEqual(selection.start.line, placeholder1Range.start.line)
-            assert.strictEqual(selection.start.character, placeholder1Range.start.character)
-            assert.strictEqual(selection.end.character, placeholder1Range.end.character)
-
-            await snippetSession.nextPlaceholder() // Wrap back to placeholder 0
-            selection = await mockEditor.getSelection()
-
-            // Validate we are highlighting the _second_ item now
             assert.strictEqual(selection.start.line, placeholder0Range.start.line)
             assert.strictEqual(selection.start.character, placeholder0Range.start.character)
             assert.strictEqual(selection.end.character, placeholder0Range.end.character)
         })
+
+        it("triggers finish event after last placeholder", async () => {
+            snippetSession = new SnippetSession(mockEditor as any, "${0:test}")
+
+            let cancelHit = 0
+            snippetSession.onCancel.subscribe(() => {
+                cancelHit++
+            })
+
+            await snippetSession.start()
+            await snippetSession.nextPlaceholder()
+
+            assert.strictEqual(cancelHit, 1, "Verify onCancel event was fired")
+        })
     })
 
     describe("synchronizeUpdatedPlaceholders", () => {

extensions/typescript/snippets/typescript.json

@@ -21,7 +21,7 @@
     },
     "Import external module.": {
         "prefix": "import statement",
-        "body": ["import { $1 } from \"${0:module}\";"],
+        "body": ["import { $0 } from \"${1:module}\";"],
         "description": "Import external module."
     },
     "Property getter": {