Skip to content

Fix #49777 - Emmet balance In after balance out should go back to initial selection and not first child#49996

Merged
ramya-rao-a merged 3 commits into
microsoft:masterfrom
Heldenkrieger01:EmmetBalance
May 30, 2018
Merged

Fix #49777 - Emmet balance In after balance out should go back to initial selection and not first child#49996
ramya-rao-a merged 3 commits into
microsoft:masterfrom
Heldenkrieger01:EmmetBalance

Conversation

@Heldenkrieger01

Copy link
Copy Markdown
Contributor

This should fix #49777 according to the instructions of @ramya-rao-a.

This is my first contribution, I hope everything is alright 😃.

@msftclas

msftclas commented May 16, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@ramya-rao-a ramya-rao-a left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @Heldenkrieger01,

Thanks for the PR and your first contribution! Its a good start. I have left 2 comments for cases when the fix will not work as the stack being maintained gets stale.

My proposal:

  • Have a variable that will contain the selections that are applied at the end of every balance out command
  • When a new balance in/out command is run, compare current selections with the above. If they are the same, then they all belong to the same stack, else clear the stack.
  • When a balance in command is run and the stack is empty, then clear the above variable and use the getRangeToBalanceIn method to determine the selections

Thoughts?

return selection;
}

function getRangeToBalanceIn(document: vscode.TextDocument, selection: vscode.Selection, rootNode: HtmlNode): vscode.Selection {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function getRangeToBalanceIn is needed.
Take the case when the user selects an html node and starts balancing inwards. Then the beforeEmmetSelection array will be empty and the command would fail. In this case, we should use getRangeToBalanceIn

Comment thread extensions/emmet/src/balance.ts Outdated
import { HtmlNode } from 'EmmetNode';
import { getNode, parseDocument, validate } from './util';

let balanceStack: Array<vscode.Selection[]> = [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the current code, the balanceStack is never reset. Take the case, where the user runs a series of balance out operations. Then they continue with some other work. After some time, they select some text and try to balance in. The current code will pop the stack and give wrong selections

Or take the case of a series of balance out, escape, then another series of balance out in a separate part of the file. Now when you balance in, the stack has selections from the first set which would be wrong

@ramya-rao-a

Copy link
Copy Markdown
Contributor

@Heldenkrieger01 I have made the necessary changes

@ramya-rao-a ramya-rao-a merged commit 30bcdff into microsoft:master May 30, 2018
@Heldenkrieger01 Heldenkrieger01 deleted the EmmetBalance branch June 7, 2018 06:57
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Emmet balance In after balance out should go back to initial selection and not first child

3 participants