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

Emmet - Wrap with abbreviation with live preview #45092

Merged
merged 17 commits into from
Mar 7, 2018
Merged

Emmet - Wrap with abbreviation with live preview #45092

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
39e6a2b
Revert "Fix for Emmet's wrap with abbreviation inserting extra spaces…
Mar 3, 2018
4a566ef
Adding ability for emmet to wrap with abbreviation in real time.
Feb 23, 2018
4947eaf
Fixes to wrap in real time:
Feb 28, 2018
3b9af18
Fixes to a few issues.
Mar 3, 2018
57082a4
Ammend for previous revert
Mar 5, 2018
b855519
Reapplying reverted commit, fixing the bug for this branch's version
Mar 5, 2018
b576c20
Adding multicursor support
Mar 5, 2018
2821c46
Renaming, refactoring and other stuff
Mar 5, 2018
22ca9c4
More refactorings
ramya-rao-a Mar 6, 2018
12674c6
More renaming and refactoring
Mar 6, 2018
0889378
Replacing placeholders when previewing, simplifying the extracting of…
Mar 6, 2018
106bc42
More refactoring
ramya-rao-a Mar 6, 2018
5ea306b
Adding a comment.
Mar 6, 2018
56d3f88
Readding test removed by mistake.
Mar 6, 2018
9fd6348
Refactoring
Mar 7, 2018
70c0a26
Carefully reverting changes in yarn.lock
Mar 7, 2018
206c968
carefully but right
Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 118 additions & 26 deletions extensions/emmet/src/abbreviationActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ interface ExpandAbbreviationInput {
filter?: string;
}

interface PreviewRangesWithContent {
previewRange: vscode.Range;
originalRange: vscode.Range;
originalContent: string;
}

export function wrapWithAbbreviation(args: any) {
if (!validate(false) || !vscode.window.activeTextEditor) {
return;
Expand All @@ -32,48 +38,134 @@ export function wrapWithAbbreviation(args: any) {
const editor = vscode.window.activeTextEditor;
let rootNode = parseDocument(editor.document, false);

const syntax = getSyntaxFromArgs({ language: editor.document.languageId });
const syntax = getSyntaxFromArgs({ language: editor.document.languageId }) || '';
if (!syntax) {
return;
}

const abbreviationPromise = (args && args['abbreviation']) ? Promise.resolve(args['abbreviation']) : vscode.window.showInputBox({ prompt: 'Enter Abbreviation' });
let previewMade = false;

// Fetch general information for the succesive expansions. i.e. the ranges to replace and its contents
let rangesToReplace: PreviewRangesWithContent[] = [];

editor.selections.sort((a: vscode.Selection, b: vscode.Selection) => { return a.start.line - b.start.line; }).forEach(selection => {
let rangeToReplace: vscode.Range = selection.isReversed ? new vscode.Range(selection.active, selection.anchor) : selection;
if (rangeToReplace.isEmpty) {
let { active } = selection;
let currentNode = getNode(rootNode, active, true);
if (currentNode && (currentNode.start.line === active.line || currentNode.end.line === active.line)) {
rangeToReplace = new vscode.Range(currentNode.start, currentNode.end);
} else {
rangeToReplace = new vscode.Range(rangeToReplace.start.line, 0, rangeToReplace.start.line, editor.document.lineAt(rangeToReplace.start.line).text.length);
}
}

const firstLineOfSelection = editor.document.lineAt(rangeToReplace.start).text.substr(rangeToReplace.start.character);
const matches = firstLineOfSelection.match(/^(\s*)/);
const preceedingWhiteSpace = matches ? matches[1].length : 0;

rangeToReplace = new vscode.Range(rangeToReplace.start.line, rangeToReplace.start.character + preceedingWhiteSpace, rangeToReplace.end.line, rangeToReplace.end.character);
let textToReplace = editor.document.getText(rangeToReplace);
rangesToReplace.push({ previewRange: rangeToReplace, originalRange: rangeToReplace, originalContent: textToReplace });
});

let abbreviationPromise;
let currentValue = '';

function inputChanged(value: string): string {
if (value !== currentValue) {
currentValue = value;
makeChanges(value, previewMade, false).then((out) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if inputChanged gets called before the previous makeChanges completes?

if (typeof out === 'boolean') {
previewMade = out;
}
});
}
return '';
}

abbreviationPromise = (args && args['abbreviation']) ? Promise.resolve(args['abbreviation']) : vscode.window.showInputBox({ prompt: 'Enter Abbreviation', validateInput: inputChanged });
const helper = getEmmetHelper();

return abbreviationPromise.then(inputAbbreviation => {
if (!inputAbbreviation || !inputAbbreviation.trim() || !helper.isAbbreviationValid(syntax, inputAbbreviation)) { return false; }
function makeChanges(inputAbbreviation: string | undefined, previewMade?: boolean, definitive?: boolean): Thenable<any> {
if (!inputAbbreviation || !inputAbbreviation.trim() || !helper.isAbbreviationValid(syntax, inputAbbreviation)) {
return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning false allows us to not revert the preview several times. That would not a problem per se, since revertPreview only replaces the current range with the original content, but why make extra unnecessary edits?

}

let extractedResults = helper.extractAbbreviationFromText(inputAbbreviation);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be an exisitng bug... but shouldnt we check for the validity of the abbreviation after it is extracted instead of before?
Also, if extractedResults is empty then shouldnt we revert the previous preview?

I would suggest to move the extraction to be the first thing that we do in makeChanges(). Then,

if (!extractedResults || !helper.isAbbreviationValid(syntax, extractedResults.abbreviation)) {
return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();
}

if (!extractedResults) {
return false;
return Promise.resolve(previewMade);
}

let { abbreviation, filter } = extractedResults;
if (definitive) {
const revertPromise = previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();
return revertPromise.then(() => {
const expandAbbrList: ExpandAbbreviationInput[] = rangesToReplace.map(rangesAndContent => {
let rangeToReplace = rangesAndContent.originalRange;
let textToWrap = rangeToReplace.isSingleLine ? ['$TM_SELECTED_TEXT'] : ['\n\t$TM_SELECTED_TEXT\n'];
return { syntax, abbreviation, rangeToReplace, textToWrap, filter };
});
return expandAbbreviationInRange(editor, expandAbbrList, true).then(() => { return Promise.resolve(); });
});
}

let expandAbbrList: ExpandAbbreviationInput[] = [];
const expandAbbrList: ExpandAbbreviationInput[] = rangesToReplace.map(rangesAndContent => {
let match = rangesAndContent.originalContent.match(/\n[\s]*/g);
let textToWrap = match ? ['\n\t' + rangesAndContent.originalContent.split(match[match.length - 1]).join('\n\t') + '\n'] : [rangesAndContent.originalContent];
return { syntax, abbreviation, rangeToReplace: rangesAndContent.originalRange, textToWrap, filter };
});

editor.selections.forEach(selection => {
let rangeToReplace: vscode.Range = selection.isReversed ? new vscode.Range(selection.active, selection.anchor) : selection;
if (rangeToReplace.isEmpty) {
let { active } = selection;
let currentNode = getNode(rootNode, active, true);
if (currentNode && (currentNode.start.line === active.line || currentNode.end.line === active.line)) {
rangeToReplace = new vscode.Range(currentNode.start, currentNode.end);
} else {
rangeToReplace = new vscode.Range(rangeToReplace.start.line, 0, rangeToReplace.start.line, editor.document.lineAt(rangeToReplace.start.line).text.length);
}
}
return applyPreview(editor, expandAbbrList, rangesToReplace);
}

const firstLineOfSelection = editor.document.lineAt(rangeToReplace.start).text.substr(rangeToReplace.start.character);
const matches = firstLineOfSelection.match(/^(\s*)/);
const preceedingWhiteSpace = matches ? matches[1].length : 0;
// On inputBox closing
return abbreviationPromise.then(inputAbbreviation => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are currently failing as the promise returned by makeChanges is not being returned by wrapWithAbbreviation

Since makeChanges does a revertPreview when given abbreivation is null/undefined/invalid, can we not simplify this to return abbreviationPromise.then(inputAbbreviation => makeChanges(inputAbbreviation, previewMade, true)); ?

return makeChanges(inputAbbreviation, previewMade, true);
});
}

rangeToReplace = new vscode.Range(rangeToReplace.start.line, rangeToReplace.start.character + preceedingWhiteSpace, rangeToReplace.end.line, rangeToReplace.end.character);
let textToWrap = rangeToReplace.isSingleLine ? ['$TM_SELECTED_TEXT'] : ['\n\t$TM_SELECTED_TEXT\n'];
expandAbbrList.push({ syntax, abbreviation, rangeToReplace, textToWrap, filter });
});
function revertPreview(editor: vscode.TextEditor, rangesToReplace: PreviewRangesWithContent[]): Thenable<any> {
return editor.edit(builder => {
for (let i = 0; i < rangesToReplace.length; i++) {
builder.replace(rangesToReplace[i].previewRange, rangesToReplace[i].originalContent);
rangesToReplace[i].previewRange = rangesToReplace[i].originalRange;
}
}, { undoStopBefore: false, undoStopAfter: false });
}

return expandAbbreviationInRange(editor, expandAbbrList, true);
});
function applyPreview(editor: vscode.TextEditor, expandAbbrList: ExpandAbbreviationInput[], rangesToReplace: PreviewRangesWithContent[]): Thenable<any> {
const anyExpandAbbrInput = expandAbbrList[0];
let expandedText = expandAbbr(anyExpandAbbrInput);
let linesInserted = 0;

if (expandedText) {
return editor.edit(builder => {
for (let i = 0; i < rangesToReplace.length; i++) {
if (i) {
expandedText = expandAbbr(expandAbbrList[i]);
}
let thisRange = rangesToReplace[i].previewRange;
let indentPrefix = '';
let preceedingText = editor.document.getText(new vscode.Range(new vscode.Position(thisRange.start.line, 0), thisRange.start));
// If there is only whitespace before the text to wrap, take that as prefix. If not, take as much whitespace as there is before text appears.
if (!preceedingText.match(/[^\s]/)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this if/else, why not follow the same logic as preceedingWhiteSpace in https://github.com/Microsoft/vscode/blob/master/extensions/emmet/src/abbreviationActions.ts#L66-L68?

indentPrefix = preceedingText;
} else {
indentPrefix = (preceedingText.match(/(^[\s]*)/) || ['', ''])[1];
}

let newText = expandedText || '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we continue if expandedText is null/undefined/empty?

newText = newText.replace(/\n/g, '\n' + indentPrefix).replace(/\$\{[\d]*\}/g, '|');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not replace placeholders. Try wrapping with !

let newTextLines = (newText.match(/\n/g) || []).length + 1;
let currentTextLines = thisRange.end.line - thisRange.start.line + 1;
builder.replace(thisRange, newText);
rangesToReplace[i].previewRange = new vscode.Range(thisRange.start.line + linesInserted, thisRange.start.character, thisRange.start.line + linesInserted + newTextLines - 1, 1000); // TODO: fix 1000! Count characters in the resulting text?
linesInserted += newTextLines - currentTextLines;
}
}, { undoStopBefore: false, undoStopAfter: false });
}
return Promise.resolve([]);
}

export function wrapIndividualLinesWithAbbreviation(args: any) {
Expand Down
25 changes: 0 additions & 25 deletions extensions/emmet/src/test/wrapWithAbbreviation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,31 +197,6 @@ suite('Tests for Wrap with Abbreviations', () => {
});
});

test('Wrap with multiline abbreviation doesnt add extra spaces', () => {
// Issue #29898
const contents = `
hello
`;
const expectedContents = `
<ul>
<li><a href="">hello</a></li>
</ul>
`;

return withRandomFileEditor(contents, 'html', (editor, doc) => {
editor.selections = [new Selection(1, 2, 1, 2)];
const promise = wrapWithAbbreviation({ abbreviation: 'ul>li>a' });
if (!promise) {
assert.equal(1, 2, 'Wrap returned undefined instead of promise.');
return Promise.resolve();
}
return promise.then(() => {
assert.equal(editor.document.getText(), expectedContents);
return Promise.resolve();
});
});
});

test('Wrap individual lines with abbreviation', () => {
const contents = `
<ul class="nav main">
Expand Down
6 changes: 1 addition & 5 deletions extensions/emmet/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2058,16 +2058,12 @@ vscode-emmet-helper@^1.2.0:
dependencies:
"@emmetio/extract-abbreviation" "0.1.6"
jsonc-parser "^1.0.0"
vscode-languageserver-types "^3.6.0-next.1"
vscode-languageserver-types "^3.5.0"

vscode-languageserver-types@^3.5.0:
version "3.5.0"
resolved "https://registry.yarnpkg.com/vscode-languageserver-types/-/vscode-languageserver-types-3.5.0.tgz#e48d79962f0b8e02de955e3f524908e2b19c0374"

vscode-languageserver-types@^3.6.0-next.1:
version "3.6.0-next.1"
resolved "https://registry.yarnpkg.com/vscode-languageserver-types/-/vscode-languageserver-types-3.6.0-next.1.tgz#98e488d3f87b666b4ee1a3d89f0023e246d358f3"

[email protected]:
version "3.2.1"
resolved "https://registry.yarnpkg.com/vscode-nls/-/vscode-nls-3.2.1.tgz#b1f3e04e8a94a715d5a7bcbc8339c51e6d74ca51"
Expand Down