8000 Emmet - Wrap with abbreviation with live preview by gushuro · Pull Request #45092 · microsoft/vscode · GitHub

More Web Proxy on the site http://driver.im/

Emmet - Wrap with abbreviation with live preview #45092

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

Merged

ramya-rao-a merged 17 commits into microsoft:master from gushuro:wraprealtime

Mar 7, 2018

Contributor

gushuro commented

Adding ability to have a preview of the resulting text when calling the Wrap with Abbreviation command, as the user types in the input box.
Feature request here #23169

Gustavo Martin Hurovich added 7 commits

March 2, 2018 18:02


          Revert "Fix for Emmet's wrap with abbreviation inserting extra spaces (…

39e6a2b

…microsoft#43345)"

This reverts commit 7420a04.


          Adding ability for emmet to wrap with abbreviation in real time.

4a566ef

Currently it's only working for single cursor.


          Fixes to wrap in real time:

4947eaf

- Removed flickering when typing abbreviation
- Removed tabstops
- Fixed bug when wrapping multiline text


          Fixes to a few issues.

3b9af18

- Added checks for not reverting previews more times than needed, that was causing extra text to be deleted.
- Fixed issue when wrapping nodes with multiple level of indentation.
- Removed all the undo commands. Now all the logic of going back to the original state is handled by revertPreview.


          Ammend for previous revert

57082a4


          Reapplying reverted commit, fixing the bug for this branch's version

b855519

Refactoring some of the code, now a single object contains the current and original ranges, as well as the original content to wrap


          Adding multicursor support

b576c20

gushuro requested a review from ramya-rao-a

March 5, 2018 22:37

ramya-rao-a reviewed

View reviewed changes

Contributor

ramya-rao-a left a comment

Initial comments

extensions/emmet/src/abbreviationActions.ts Outdated

@@ @@ -24,6 +24,12 @@ interface ExpandAbbreviationInput { @@
               	filter?: string;
               }
+              interface RangesAndContent {
+              	currentRange: vscode.Range;

Contributor

ramya-rao-a

The currentRange here will only have the preview ranges correct? If so, let's rename it to previewRange to make it more readable.
Also rename content to originalContent to match with the originalRange and RangesAndContent to PreviewRangesWithContent

Contributor Author

gushuro

Sure

extensions/emmet/src/abbreviationActions.ts Outdated

+              	}
+              	// On inputBox closing
+              	return abbreviationPromise.then(inputAbbreviation => {

Contributor

ramya-rao-a

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)); ?

extensions/emmet/src/abbreviationActions.ts Outdated

-              		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)) {
+              			let returnPromise: Thenable<any> = Promise.resolve();

Contributor

ramya-rao-a

You can condense these 5 lines to return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();

extensions/emmet/src/abbreviationActions.ts Outdated

8000

+              				returnPromise = revertPreview(editor, rangesToReplace).then(() => { return false; });
+              			}
+              			return returnPromise;
+              		}
               		let extractedResults = helper.extractAbbreviationFromText(inputAbbreviation);

Contributor

ramya-rao-a

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();
}

extensions/emmet/src/abbreviationActions.ts Outdated

		}
		rangesToReplace.forEach(rangesAndContent => {

Contributor

ramya-rao-a

The currentRange of each rangesToReplace array gets updated inside revertPreview. Since the originalRange doesnt change, it is safe to loop through and use it here before revertPromise is completed. But for the sake of readability, it is better to do this after `revertPromise is completed.

extensions/emmet/src/abbreviationActions.ts Outdated

+              			rangesToReplace.forEach(rangesAndContent => {
+              				let rangeToReplace = rangesAndContent.originalRange;
+              				let textToWrap = rangeToReplace.isSingleLine ? ['$TM_SELECTED_TEXT'] : ['\n\t$TM_SELECTED_TEXT\n'];
+              				expandAbbrList.push({ syntax: syntax || '', abbreviation, rangeToReplace, textToWrap, filter });

Contributor

ramya-rao-a

why would syntax be null/undefined here?

extensions/emmet/src/abbreviationActions.ts Outdated

-              			let textToWrap = rangeToReplace.isSingleLine ? ['$TM_SELECTED_TEXT'] : ['\n\t$TM_SELECTED_TEXT\n'];
-              			expandAbbrList.push({ syntax, abbreviation, rangeToReplace, textToWrap, filter });
+              		return Promise.resolve().then(() => {
+              			return applyPreview(editor, expandAbbrList, rangesToReplace).then(ranges => {

Contributor

ramya-rao-a

Why return the ranges here from applyPreview and the update rangesToReplace?
rangesToReplace is accessible from inside applyPreview and can be updated from there as well.
Just like you did in revertPreview

Gustavo Martin Hurovich and others added 2 commits

March 5, 2018 15:56


          Renaming, refactoring and other stuff

2821c46


          More refactorings

22ca9c4

gushuro commented

View reviewed changes

extensions/emmet/src/abbreviationActions.ts Outdated

@@ @@ -90,45 +89,34 @@ export function wrapWithAbbreviation(args: any) { @@
               	function makeChanges(inputAbbreviation: string | undefined, previewMade?: boolean, definitive?: boolean): Thenable<any> {
               		if (!inputAbbreviation || !inputAbbreviation.trim() || !helper.isAbbreviationValid(syntax, inputAbbreviation)) {
-              			return previewMade ? revertPreview(editor, rangesToReplace).then(() => { return false; }) : Promise.resolve();
+              			return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();

Contributor Author

gushuro

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?


          More renaming and refactoring

12674c6

ramya-rao-a reviewed

View reviewed changes

extensions/emmet/src/abbreviationActions.ts Outdated

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

Contributor

ramya-rao-a

What if inputChanged gets called before the previous makeChanges ED48 completes?

extensions/emmet/src/abbreviationActions.ts Outdated

+              				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]/)) {

Contributor

ramya-rao-a

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?

extensions/emmet/src/abbreviationActions.ts Outdated

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

Contributor

ramya-rao-a

This will not replace placeholders. Try wrapping with !

extensions/emmet/src/abbreviationActions.ts Outdated

+              					indentPrefix = (preceedingText.match(/(^[\s]*)/) || ['', ''])[1];
+              				}
+              				let newText = expandedText || '';

Contributor

ramya-rao-a

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

kieferrm assigned ramya-rao-a

Gustavo Martin Hurovich and others added 7 commits

March 6, 2018 11:13


          Replacing placeholders when previewing, simplifying the extracting of…

… preceeding whitespace, added a check for validity of expandedtext on each selection.


          More refactoring

106bc42


          Adding a comment.

5ea306b


          Readding test removed by mistake.

56d3f88


          Refactoring

9fd6348


          Carefully reverting changes in yarn.lock

70c0a26


          carefully but right

206c968

ramya-rao-a approved these changes

View reviewed changes

ramya-rao-a merged commit 2e746d8 into microsoft:master

gushuro mentioned this pull request

Adding Live Preview for Wrapping Individual Lines with Abbreviation #45453

Merged

Yukaii mentioned this pull request

Live Preview Yukaii/map-replace.js#1

Closed

github-actions bot locked and limited conversation to collaborators

github-actions bot unassigned ramya-rao-a

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

0