From 072362a6c91e78b155a7cff78e9156fd2d37d790 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Tue, 9 Feb 2021 14:00:56 -0800 Subject: [PATCH] Fix behavior when inserting content between two adjacent tab stops. --- lib/insertion.js | 3 +- lib/snippet-expansion.js | 211 ++++++++++++++++++++++++++++++++------- lib/snippet.js | 17 +++- lib/tab-stop.js | 4 +- spec/snippets-spec.js | 56 +++++++++++ 5 files changed, 249 insertions(+), 42 deletions(-) diff --git a/lib/insertion.js b/lib/insertion.js index 96065d1e..cc6d232f 100644 --- a/lib/insertion.js +++ b/lib/insertion.js @@ -45,9 +45,10 @@ function transformText (str, flags) { } class Insertion { - constructor ({ range, substitution }) { + constructor ({ range, substitution, references }) { this.range = range this.substitution = substitution + this.references = references if (substitution) { if (substitution.replace === undefined) { substitution.replace = '' diff --git a/lib/snippet-expansion.js b/lib/snippet-expansion.js index 54a525bc..aa36b519 100644 --- a/lib/snippet-expansion.js +++ b/lib/snippet-expansion.js @@ -10,9 +10,29 @@ module.exports = class SnippetExpansion { this.cursor = cursor this.snippets = snippets this.subscriptions = new CompositeDisposable - this.tabStopMarkers = [] this.selections = [this.cursor.selection] + // Holds the `Insertion` instance corresponding to each tab stop marker. We + // don't use the tab stop's own numbering here; we renumber them + // consecutively starting at 0 in the order in which they should be + // visited. So `$1` (if present) will always be at index `0`, and `$0` (if + // present) will always be the last index. + this.insertionsByIndex = [] + + // Each insertion has a corresponding marker. We keep them in a map so we + // can easily reassociate an insertion with its new marker when we destroy + // its old one. + this.markersForInsertions = new Map() + + // The index of the active tab stop. + this.tabStopIndex = null + + // If, say, tab stop 4's placeholder references tab stop 2, then tab stop + // 4's insertion goes into this map as a "related" insertion to tab stop 2. + // We need to keep track of this because tab stop 4's marker will need to + // be replaced while 2 is the active index. + this.relatedInsertionsByIndex = new Map() + const startPosition = this.cursor.selection.getBufferRange().start let {body, tabStopList} = this.snippet let tabStops = tabStopList.toArray() @@ -28,8 +48,11 @@ module.exports = class SnippetExpansion { this.editor.transact(() => { this.ignoringBufferChanges(() => { this.editor.transact(() => { + // Insert the snippet body at the cursor. const newRange = this.cursor.selection.insertText(body, {autoIndent: false}) if (this.snippet.tabStopList.length > 0) { + // Listen for cursor changes so we can decide whether to keep the + // snippet active or terminate it. this.subscriptions.add(this.cursor.onDidChangePosition(event => this.cursorMoved(event))) this.subscriptions.add(this.cursor.onDidDestroy(() => this.cursorDestroyed())) this.placeTabStopMarkers(startPosition, tabStops) @@ -49,9 +72,12 @@ module.exports = class SnippetExpansion { cursorMoved ({oldBufferPosition, newBufferPosition, textChanged}) { if (this.settingTabStop || textChanged) { return } - const itemWithCursor = this.tabStopMarkers[this.tabStopIndex].find(item => item.marker.getBufferRange().containsPoint(newBufferPosition)) + const insertionAtCursor = this.insertionsByIndex[this.tabStopIndex].find(insertion => { + let marker = this.markersForInsertions.get(insertion) + return marker.getBufferRange().containsPoint(newBufferPosition) + }) - if (itemWithCursor && !itemWithCursor.insertion.isTransformation()) { return } + if (insertionAtCursor && !insertionAtCursor.isTransformation()) { return } this.destroy() } @@ -80,30 +106,35 @@ module.exports = class SnippetExpansion { applyAllTransformations () { this.editor.transact(() => { - this.tabStopMarkers.forEach((item, index) => - this.applyTransformations(index, true)) + this.insertionsByIndex.forEach((insertion, index) => + this.applyTransformations(index)) }) } - applyTransformations (tabStop, initial = false) { - const items = [...this.tabStopMarkers[tabStop]] - if (items.length === 0) { return } + applyTransformations (tabStopIndex) { + const insertions = [...this.insertionsByIndex[tabStopIndex]] + if (insertions.length === 0) { return } - const primary = items.shift() - const primaryRange = primary.marker.getBufferRange() + const primaryInsertion = insertions.shift() + const primaryRange = this.markersForInsertions.get(primaryInsertion).getBufferRange() const inputText = this.editor.getTextInBufferRange(primaryRange) this.ignoringBufferChanges(() => { - for (const item of items) { - const {marker, insertion} = item - var range = marker.getBufferRange() - + for (const [index, insertion] of insertions.entries()) { // Don't transform mirrored tab stops. They have their own cursors, so // mirroring happens automatically. if (!insertion.isTransformation()) { continue } + var marker = this.markersForInsertions.get(insertion) + var range = marker.getBufferRange() + var outputText = insertion.transform(inputText) this.editor.transact(() => this.editor.setTextInBufferRange(range, outputText)) + + // Manually adjust the marker's range rather than rely on its internal + // heuristics. (We don't have to worry about whether it's been + // invalidated because setting its buffer range implicitly marks it as + // valid again.) const newRange = new Range( range.start, range.start.traverse(new Point(0, outputText.length)) @@ -114,36 +145,115 @@ module.exports = class SnippetExpansion { } placeTabStopMarkers (startPosition, tabStops) { - for (const tabStop of tabStops) { + // Tab stops within a snippet refer to one another by their external index + // (1 for $1, 3 for $3, etc.). We respect the order of these tab stops, but + // we renumber them starting at 0 and using consecutive numbers. + // + // Luckily, we don't need to convert between the two numbering systems very + // often. But we do have to build a map from external index to our internal + // index. We do this in a separate loop so that the table is complete + // before we need to consult it in the following loop. + const indexTable = {} + for (let [index, tabStop] of tabStops.entries()) { + indexTable[tabStop.index] = index + } + + for (let [index, tabStop] of tabStops.entries()) { const {insertions} = tabStop - const markers = [] if (!tabStop.isValid()) { continue } for (const insertion of insertions) { const {range} = insertion const {start, end} = range + let references = null + if (insertion.references) { + references = insertion.references.map(external => indexTable[external]) + } + // Since this method is called only once at the beginning of a snippet expansion, we know that 0 is about to be the active tab stop. + const shouldBeInclusive = (index === 0) || (references && references.includes(0)) const marker = this.getMarkerLayer(this.editor).markBufferRange([ startPosition.traverse(start), startPosition.traverse(end) - ]) - markers.push({ - index: markers.length, - marker, - insertion - }) + ], { exclusive: !shouldBeInclusive }) + // Now that we've created these markers, we need to store them in a + // data structure because they'll need to be deleted and re-created + // when their exclusivity changes. + this.markersForInsertions.set(insertion, marker) + + if (references) { + const relatedInsertions = this.relatedInsertionsByIndex.get(index) || [] + relatedInsertions.push(insertion) + this.relatedInsertionsByIndex.set(index, relatedInsertions) + } } - - this.tabStopMarkers.push(markers) + this.insertionsByIndex[index] = insertions } this.setTabStopIndex(0) this.applyAllTransformations() } + // When two insertion markers are directly adjacent to one another, and the + // cursor is placed right at the border between them, the marker that should + // "claim" the newly typed content will vary based on context. + // + // All else being equal, that content should get added to the marker (if any) + // whose tab stop is active, or else the marker whose tab stop's placeholder + // references an active tab stop. The `exclusive` setting on a marker + // controls whether that marker grows to include content added at its edge. + // + // So we need to revisit the markers whenever the active tab stop changes, + // figure out which ones need to be touched, and replace them with markers + // that have the settings we need. + adjustTabStopMarkers (oldIndex, newIndex) { + // Take all the insertions whose markers were made inclusive when they + // became active and restore their original marker settings. + const insertionsForOldIndex = [ + ...this.insertionsByIndex[oldIndex], + ...(this.relatedInsertionsByIndex.get(oldIndex) || []) + ] + + for (let insertion of insertionsForOldIndex) { + this.replaceMarkerForInsertion(insertion, {exclusive: true}) + } + + // Take all the insertions belonging to the newly active tab stop (and all + // insertions whose placeholders reference the newly active tab stop) and + // change their markers to be inclusive. + const insertionsForNewIndex = [ + ...this.insertionsByIndex[newIndex], + ...(this.relatedInsertionsByIndex.get(newIndex) || []) + ] + + for (let insertion of insertionsForNewIndex) { + this.replaceMarkerForInsertion(insertion, {exclusive: false}) + } + } + + replaceMarkerForInsertion (insertion, settings) { + const marker = this.markersForInsertions.get(insertion) + + // If the marker is invalid or destroyed, return it as-is. Other methods + // need to know if a marker has been invalidated or destroyed, and we have + // no need to change the settings on such markers anyway. + if (!marker.isValid() || marker.isDestroyed()) { + return marker + } + + // Otherwise, create a new marker with an identical range and the specified + // settings. + const range = marker.getBufferRange() + const replacement = this.getMarkerLayer(this.editor).markBufferRange(range, settings) + + marker.destroy() + this.markersForInsertions.set(insertion, replacement) + return replacement + } + goToNextTabStop () { const nextIndex = this.tabStopIndex + 1 - if (nextIndex < this.tabStopMarkers.length) { + if (nextIndex < this.insertionsByIndex.length) { if (this.setTabStopIndex(nextIndex)) { return true } else { @@ -167,21 +277,30 @@ module.exports = class SnippetExpansion { if (this.tabStopIndex > 0) { this.setTabStopIndex(this.tabStopIndex - 1) } } - setTabStopIndex (tabStopIndex) { - this.tabStopIndex = tabStopIndex + setTabStopIndex (newIndex) { + const oldIndex = this.tabStopIndex + this.tabStopIndex = newIndex + // Set a flag before moving any selections so that our change handlers know + // that the movements were initiated by us. this.settingTabStop = true + // Keep track of whether we placed any selections or cursors. let markerSelected = false - const items = this.tabStopMarkers[this.tabStopIndex] - if (items.length === 0) { return false } + const insertions = this.insertionsByIndex[this.tabStopIndex] + if (insertions.length === 0) { return false } const ranges = [] this.hasTransforms = false - for (const item of items) { - const {marker, insertion} = item + + // Go through the active tab stop's markers to figure out where to place + // cursors and/or selections. + for (const insertion of insertions) { + const marker = this.markersForInsertions.get(insertion) if (marker.isDestroyed()) { continue } if (!marker.isValid()) { continue } if (insertion.isTransformation()) { + // Set a flag for later, but skip transformation insertions because + // they don't get their own cursors. this.hasTransforms = true continue } @@ -189,6 +308,8 @@ module.exports = class SnippetExpansion { } if (ranges.length > 0) { + // We have new selections to apply. Reuse existing selections if + // possible, destroying the unused ones if we already have too many. for (const selection of this.selections.slice(ranges.length)) { selection.destroy() } this.selections = this.selections.slice(0, ranges.length) for (let i = 0; i < ranges.length; i++) { @@ -202,26 +323,38 @@ module.exports = class SnippetExpansion { this.selections.push(newSelection) } } + // We placed at least one selection, so this tab stop was successfully + // set. markerSelected = true } this.settingTabStop = false // If this snippet has at least one transform, we need to observe changes // made to the editor so that we can update the transformed tab stops. - if (this.hasTransforms) { this.snippets.observeEditor(this.editor) } + if (this.hasTransforms) { + this.snippets.observeEditor(this.editor) + } else { + this.snippets.stopObservingEditor(this.editor) + } + + if (oldIndex !== null) { + this.adjustTabStopMarkers(oldIndex, newIndex) + } return markerSelected } goToEndOfLastTabStop () { - if (this.tabStopMarkers.length === 0) { return } - const items = this.tabStopMarkers[this.tabStopMarkers.length - 1] - if (items.length === 0) { return } - const {marker: lastMarker} = items[items.length - 1] + const size = this.insertionsByIndex.length + if (size === 0) { return } + const insertions = this.insertionsByIndex[size - 1] + if (insertions.length === 0) { return } + const lastMarker = this.markersForInsertions.get(insertions[insertions.length - 1]) + if (lastMarker.isDestroyed()) { return false } else { - this.editor.setCursorBufferPosition(lastMarker.getEndBufferPosition()) + this.seditor.setCursorBufferPosition(lastMarker.getEndBufferPosition()) return true } } @@ -229,7 +362,9 @@ module.exports = class SnippetExpansion { destroy () { this.subscriptions.dispose() this.getMarkerLayer(this.editor).clear() - this.tabStopMarkers = [] + this.insertionsByIndex = [] + this.relatedInsertionsByIndex = new Map() + this.markersForInsertions = new Map(); this.snippets.stopObservingEditor(this.editor) this.snippets.clearExpansions(this.editor) } diff --git a/lib/snippet.js b/lib/snippet.js index fcdfed90..d3b1767d 100644 --- a/lib/snippet.js +++ b/lib/snippet.js @@ -1,6 +1,16 @@ const {Range} = require('atom') const TabStopList = require('./tab-stop-list') +function tabStopsReferencedWithinTabStopContent (segment) { + const results = [] + for (const item of segment) { + if (item.index) { + results.push(item.index, ...tabStopsReferencedWithinTabStopContent(item.content)) + } + } + return new Set(results) +} + module.exports = class Snippet { constructor({name, prefix, bodyText, description, descriptionMoreURL, rightLabelHTML, leftLabel, leftLabelHTML, bodyTree}) { this.name = name @@ -28,12 +38,17 @@ module.exports = class Snippet { if (index === 0) { index = Infinity; } const start = [row, column] extractTabStops(content) + const referencedTabStops = tabStopsReferencedWithinTabStopContent(content) const range = new Range(start, [row, column]) const tabStop = this.tabStopList.findOrCreate({ index, snippet: this }) - tabStop.addInsertion({ range, substitution }) + tabStop.addInsertion({ + range, + substitution, + references: Array.from(referencedTabStops) + }) } else if (typeof segment === 'string') { bodyText.push(segment) var segmentLines = segment.split('\n') diff --git a/lib/tab-stop.js b/lib/tab-stop.js index 61a423e4..322f1ccf 100644 --- a/lib/tab-stop.js +++ b/lib/tab-stop.js @@ -20,8 +20,8 @@ class TabStop { return !all } - addInsertion ({ range, substitution }) { - let insertion = new Insertion({ range, substitution }) + addInsertion ({ range, substitution, references }) { + let insertion = new Insertion({ range, substitution, references }) let insertions = this.insertions insertions.push(insertion) insertions = insertions.sort((i1, i2) => { diff --git a/spec/snippets-spec.js b/spec/snippets-spec.js index 68994478..ba9f8d32 100644 --- a/spec/snippets-spec.js +++ b/spec/snippets-spec.js @@ -284,6 +284,14 @@ third tabstop $3\ "has a transformed tab stop such that it is possible to move the cursor between the ordinary tab stop and its transformed version without an intermediate step": { prefix: 't18', body: '// $1\n// ${1/./=/}' + }, + "has two tab stops adjacent to one another": { + prefix: 't19', + body: '${2:bar}${3:baz}' + }, + "has several adjacent tab stops, one of which has a placeholder with reference to another tab stop at its edge": { + prefix: 't20', + body: '${1:foo}${2:bar}${3:baz $1}$4' } } }); @@ -927,6 +935,54 @@ foo\ }); }); + describe("when the snippet has two adjacent tab stops", () => { + it("ensures insertions are treated as part of the active tab stop", () => { + editor.setText('t19'); + editor.setCursorScreenPosition([0, 3]); + simulateTabKeyEvent(); + expect(editor.getText()).toBe('barbaz'); + expect( + editor.getSelectedBufferRange() + ).toEqual([ + [0, 0], + [0, 3] + ]); + editor.insertText('w'); + expect(editor.getText()).toBe('wbaz'); + editor.insertText('at'); + expect(editor.getText()).toBe('watbaz'); + simulateTabKeyEvent(); + expect( + editor.getSelectedBufferRange() + ).toEqual([ + [0, 3], + [0, 6] + ]); + editor.insertText('foo'); + expect(editor.getText()).toBe('watfoo'); + }); + }); + + describe("when the snippet has a placeholder with a tabstop mirror at its edge", () => { + it("allows the associated marker to include the inserted text", () => { + editor.setText('t20'); + editor.setCursorScreenPosition([0, 3]); + simulateTabKeyEvent(); + expect(editor.getText()).toBe('foobarbaz '); + expect(editor.getCursors().length).toBe(2); + let selections = editor.getSelections(); + expect(selections[0].getBufferRange()).toEqual([[0, 0], [0, 3]]); + expect(selections[1].getBufferRange()).toEqual([[0, 10], [0, 10]]); + editor.insertText('nah'); + expect(editor.getText()).toBe('nahbarbaz nah'); + simulateTabKeyEvent(); + editor.insertText('meh'); + simulateTabKeyEvent(); + editor.insertText('yea'); + expect(editor.getText()).toBe('nahmehyea'); + }); + }); + describe("when the snippet contains tab stops with an index >= 10", () => { it("parses and orders the indices correctly", () => { editor.setText('t10');