Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better accomodate the auto whitespace removal when inserting or merging snippets #146179

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Better accomodate the auto whitespace removal when inserting or merging snippets #146179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 34 additions & 12 deletions src/vs/editor/contrib/snippet/browser/snippetSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { EditOperation, ISingleEditOperation } from 'vs/editor/common/core/editO
import { IPosition } from 'vs/editor/common/core/position';
import { Range } from 'vs/editor/common/core/range';
import { Selection } from 'vs/editor/common/core/selection';
import { TextChange } from 'vs/editor/common/core/textChange';
import { IIdentifiedSingleEditOperation, ITextModel, TrackedRangeStickiness } from 'vs/editor/common/model';
import { ModelDecorationOptions } from 'vs/editor/common/model/textModel';
import { OvertypingCapturer } from 'vs/editor/contrib/suggest/browser/suggestOvertypingCapturer';
Expand All @@ -27,6 +28,7 @@ export class OneSnippet {

private _placeholderDecorations?: Map<Placeholder, string>;
private _placeholderGroups: Placeholder[][];
private _offset: number = -1;
_placeholderGroupsIdx: number;
_nestingLevel: number = 1;

Expand All @@ -38,13 +40,18 @@ export class OneSnippet {
};

constructor(
private readonly _editor: IActiveCodeEditor, private readonly _snippet: TextmateSnippet,
private readonly _offset: number, private readonly _snippetLineLeadingWhitespace: string
private readonly _editor: IActiveCodeEditor,
private readonly _snippet: TextmateSnippet,
private readonly _snippetLineLeadingWhitespace: string
) {
this._placeholderGroups = groupBy(_snippet.placeholders, Placeholder.compareByIndex);
this._placeholderGroupsIdx = -1;
}

public initialize(textChange: TextChange): void {
this._offset = textChange.newPosition;
}

dispose(): void {
if (this._placeholderDecorations) {
this._editor.deltaDecorations([...this._placeholderDecorations.values()], []);
Expand All @@ -54,6 +61,10 @@ export class OneSnippet {

private _initDecorations(): void {

if (this._offset === -1) {
throw new Error(`Snippet not initialized!`);
}

if (this._placeholderDecorations) {
// already initialized
return;
Expand Down Expand Up @@ -245,6 +256,7 @@ export class OneSnippet {
// elements from the beginning of the array
for (const placeholder of this._placeholderGroups[this._placeholderGroupsIdx]) {
const nested = others.shift()!;
console.assert(nested._offset !== -1);
console.assert(!nested._placeholderDecorations);

// Massage placeholder-indicies of the nested snippet to be
Expand Down Expand Up @@ -405,8 +417,6 @@ export class SnippetSession {
const modelBasedVariableResolver = editor.invokeWithinContext(accessor => new ModelBasedVariableResolver(accessor.get(ILabelService), model));
const readClipboardText = () => clipboardText;

let delta = 0;

// know what text the overwrite[Before|After] extensions
// of the primary curser have selected because only when
// secondary selections extend to the same text we can grow them
Expand Down Expand Up @@ -466,15 +476,13 @@ export class SnippetSession {
new RandomBasedVariableResolver,
]));

const offset = model.getOffsetAt(start) + delta;
delta += snippet.toString().length - model.getValueLengthInRange(snippetSelection);

// store snippets with the index of their originating selection.
// that ensures the primiary cursor stays primary despite not being
// the one with lowest start position
edits[idx] = EditOperation.replace(snippetSelection, snippet.toString());
edits[idx].identifier = { major: idx, minor: 0 }; // mark the edit so only our undo edits will be used to generate end cursors
snippets[idx] = new OneSnippet(editor, snippet, offset, snippetLineLeadingWhitespace);
edits[idx]._isTracked = true;
snippets[idx] = new OneSnippet(editor, snippet, snippetLineLeadingWhitespace);
}

return { edits, snippets };
Expand Down Expand Up @@ -509,12 +517,19 @@ export class SnippetSession {
const { edits, snippets } = SnippetSession.createEditsAndSnippets(this._editor, this._template, this._options.overwriteBefore, this._options.overwriteAfter, false, this._options.adjustWhitespace, this._options.clipboardText, this._options.overtypingCapturer);
this._snippets = snippets;

this._editor.executeEdits('snippet', edits, undoEdits => {
this._editor.executeEdits('snippet', edits, _undoEdits => {
// Sometimes, the text buffer will remove automatic whitespace when doing any edits,
// so we need to look only at the undo edits relevant for us.
// Our edits have an identifier set so that's how we can distinguish them
const undoEdits = _undoEdits.filter(edit => !!edit.identifier);
for (let idx = 0; idx < snippets.length; idx++) {
snippets[idx].initialize(undoEdits[idx].textChange);
}

if (this._snippets[0].hasPlaceholder) {
return this._move(true);
} else {
return undoEdits
.filter(edit => !!edit.identifier) // only use our undo edits
.map(edit => Selection.fromPositions(edit.range.getEndPosition()));
}
});
Expand All @@ -528,7 +543,15 @@ export class SnippetSession {
this._templateMerges.push([this._snippets[0]._nestingLevel, this._snippets[0]._placeholderGroupsIdx, template]);
const { edits, snippets } = SnippetSession.createEditsAndSnippets(this._editor, template, options.overwriteBefore, options.overwriteAfter, true, options.adjustWhitespace, options.clipboardText, options.overtypingCapturer);

this._editor.executeEdits('snippet', edits, undoEdits => {
this._editor.executeEdits('snippet', edits, _undoEdits => {
// Sometimes, the text buffer will remove automatic whitespace when doing any edits,
// so we need to look only at the undo edits relevant for us.
// Our edits have an identifier set so that's how we can distinguish them
const undoEdits = _undoEdits.filter(edit => !!edit.identifier);
for (let idx = 0; idx < snippets.length; idx++) {
snippets[idx].initialize(undoEdits[idx].textChange);
}

for (const snippet of this._snippets) {
snippet.merge(snippets);
}
Expand All @@ -539,7 +562,6 @@ export class SnippetSession {
} else {
return (
undoEdits
.filter(edit => !!edit.identifier) // only use our undo edits
.map(edit => Selection.fromPositions(edit.range.getEndPosition()))
);
}
Expand Down
19 changes: 19 additions & 0 deletions src/vs/editor/contrib/snippet/test/browser/snippetController2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,25 @@ suite('SnippetController2', function () {
assertSelections(editor, new Selection(2, 8, 2, 8));
});

test('issue #145727: insertSnippet can put snippet selections in wrong positions (1 of 2)', function () {
const ctrl = instaService.createInstance(SnippetController2, editor);
model.setValue('');
CoreEditingCommands.Tab.runEditorCommand(null, editor, null);

ctrl.insert('\naProperty: aClass<${2:boolean}> = new aClass<${2:boolean}>();\n', { adjustWhitespace: false });
assertSelections(editor, new Selection(2, 19, 2, 26), new Selection(2, 41, 2, 48));
});

test('issue #145727: insertSnippet can put snippet selections in wrong positions (2 of 2)', function () {
const ctrl = instaService.createInstance(SnippetController2, editor);
model.setValue('');
CoreEditingCommands.Tab.runEditorCommand(null, editor, null);

ctrl.insert('\naProperty: aClass<${2:boolean}> = new aClass<${2:boolean}>();\n');
// This will insert \n aProperty....
assertSelections(editor, new Selection(2, 23, 2, 30), new Selection(2, 45, 2, 52));
});

test('leading TAB by snippets won\'t replace by spaces #101870', function () {
const ctrl = instaService.createInstance(SnippetController2, editor);
model.setValue('');
Expand Down