From 4eec1c211efbdfba1183b3e364b5991b5f65978b Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Fri, 28 Jun 2024 18:32:47 -0700 Subject: [PATCH] [snippets] Fix incorrect range traversal when resolving variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we expand a snippet, we know where the variables are within the snippet; they're described using `Point`s, but the origin of the coordinate system is the beginning of the snippet. To translate them into actual buffer `Point`s during snippet expansion, we “add” each point to the `Point` that marks the insertion point of the snippet. When doing so, we need to remember that, when a snippet contains a newline and content after that newline, that second line of content will be indented by the same amount as the initial line; we know how much leading whitespace (if any) there was before the snippet trigger text and apply it at the head of each line of the expansion. For this reason, we were adding the two points incorrectly. Since the column position doesn't reset to `0` each time a row is advanced, we should've been using `Point#translate`, not `Point#traverse`. I'd have caught this earlier if I had thought to test the combination of variable expansion and leading whitespace. I see other usages of `Point#traverse` in different contexts in this file, but I'll leave them be until they can be proven to be the source of a bug. --- packages/snippets/lib/snippet-expansion.js | 10 +++++--- packages/snippets/spec/.eslintrc | 9 +++++++ packages/snippets/spec/snippets-spec.js | 29 +++++++++++++++------- 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/packages/snippets/lib/snippet-expansion.js b/packages/snippets/lib/snippet-expansion.js index 859754f53c..0df215ec4e 100644 --- a/packages/snippets/lib/snippet-expansion.js +++ b/packages/snippets/lib/snippet-expansion.js @@ -215,10 +215,12 @@ module.exports = class SnippetExpansion { // markers are crucial for ensuring we adapt to those changes. for (const variable of this.snippet.variables) { const {point} = variable - const marker = this.getMarkerLayer(this.editor).markBufferRange([ - startPosition.traverse(point), - startPosition.traverse(point) - ], {exclusive: false}) + let markerRange = new Range( + startPosition.translate(point), + startPosition.translate(point) + ) + const marker = this.getMarkerLayer(this.editor).markBufferRange( + markerRange, {exclusive: false}) this.markersForVariables.set(variable, marker) } } diff --git a/packages/snippets/spec/.eslintrc b/packages/snippets/spec/.eslintrc index 65bf2aacac..85c7100909 100644 --- a/packages/snippets/spec/.eslintrc +++ b/packages/snippets/spec/.eslintrc @@ -1,5 +1,14 @@ { + "env": { "jasmine": true }, "rules": { + "node/no-unpublished-require": "off", + "node/no-extraneous-require": "off", + "no-unused-vars": "off", + "no-empty": "off", + "object-curly-spacing": ["error", "always"], "semi": ["error", "always"] + }, + "globals": { + "waitsForPromise": true } } diff --git a/packages/snippets/spec/snippets-spec.js b/packages/snippets/spec/snippets-spec.js index 637a55e446..09e30fed77 100644 --- a/packages/snippets/spec/snippets-spec.js +++ b/packages/snippets/spec/snippets-spec.js @@ -1,7 +1,7 @@ const path = require('path'); const temp = require('temp').track(); const Snippets = require('../lib/snippets'); -const {TextEditor} = require('atom'); +const { TextEditor } = require('atom'); const crypto = require('crypto'); const SUPPORTS_UUID = ('randomUUID' in crypto) && (typeof crypto.randomUUID === 'function'); @@ -14,8 +14,8 @@ describe("Snippets extension", () => { if (param == null) { param = {}; } - const {shift} = param; - const event = atom.keymaps.constructor.buildKeydownEvent('tab', {shift, target: editorElement}); + const { shift } = param; + const event = atom.keymaps.constructor.buildKeydownEvent('tab', { shift, target: editorElement }); atom.keymaps.handleKeyboardEvent(event); }; @@ -113,7 +113,7 @@ describe("Snippets extension", () => { invalidSnippets = 3; expect(snippets.getSnippets(editor)).toEqual({}); - invalidSnippets = {a: null}; + invalidSnippets = { a: null }; expect(snippets.getSnippets(editor)).toEqual({}); }); @@ -499,14 +499,14 @@ third tabstop $3\ expect(editor.getSelectedBufferRange()).toEqual([[2, 40], [2, 40]]); // tab backwards - simulateTabKeyEvent({shift: true}); + simulateTabKeyEvent({ shift: true }); expect(editor.getSelectedBufferRange()).toEqual([[2, 14], [2, 17]]); // should highlight text typed at tab stop - simulateTabKeyEvent({shift: true}); + simulateTabKeyEvent({ shift: true }); expect(editor.getSelectedBufferRange()).toEqual([[3, 15], [3, 15]]); // shift-tab on first tab-stop does nothing - simulateTabKeyEvent({shift: true}); + simulateTabKeyEvent({ shift: true }); expect(editor.getCursorScreenPosition()).toEqual([3, 15]); // tab through all tab stops, then tab on last stop to terminate snippet @@ -575,7 +575,7 @@ third tabstop $3\ simulateTabKeyEvent(); editor.moveRight(); - simulateTabKeyEvent({shift: true}); + simulateTabKeyEvent({ shift: true }); expect(editor.getCursorBufferPosition()).toEqual([4, 15]); }); }); @@ -672,7 +672,7 @@ third tabstop $3\ describe("when the snippet spans multiple lines", () => { beforeEach(async () => { - editor.update({autoIndent: true}); + editor.update({ autoIndent: true }); // editor.update() returns a Promise that never gets resolved, so we // need to return undefined to avoid a timeout in the spec. // TODO: Figure out why `editor.update({autoIndent: true})` never gets resolved. @@ -1055,6 +1055,17 @@ foo\ editor.insertText('TEST'); expect(editor.getText()).toBe("// TEST\n// ===="); }); + + describe("and the editor is indented", () => { + it("traverses newlines properly", () => { + editor.setText(' bannerGeneric'); + editor.setCursorScreenPosition([0, 15]); + simulateTabKeyEvent(); + expect(editor.getText()).toBe(" // \n // "); + editor.insertText('TEST'); + expect(editor.getText()).toBe(" // TEST\n // ===="); + }); + }); }); describe("and the document is HTML", () => {