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.

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

Move indent right one when the highlighted line is a scope start/end #77762

Merged
merged 9 commits into from Feb 6, 2020

Conversation

@LadyCailin
Copy link
Member

LadyCailin commented Jul 22, 2019

Previously, the indent was always to the left of the current line.
While this logic was easier to implement, it is inconsistent with the
behavior expected. This commit changes it to select the scope (or a rough
proxy of that), rather than the line's indent. This is not a complete fix for #49342, as this
does not add support to semantic detection, however, this will work in
probably 90% of the cases, and is a relatively straightforward fix.

In general, this works:

image
image
image

However, there are still edge cases where this does not work correctly, since it's not really semantic, it's just based on the indentation of the above and below lines.

image
image

For what it's worth, this does work in Notepad++ at least:
image

This is a relative edge case, and this should solve the problem in most codebases, but not all. The correct solution of course is to find the indents based on semantics, which is what #49342 is about, so I don't think this actually closes the bug totally, and so suggest it remain open. However, solving that problem is a bigger task, and requires a large amount of changes in the model, so in the meantime I think this is a good stopgap, and anyways is an appropriate fallback for plain text and unknown syntax.

Where begin/end scope is ambiguous, for instance in yml, where there aren't really "end scopes" per se, it prioritizes it as a begin scope:

image

There is strange behavior when the configured indents don't line up with the indents in the source. These are hidden by the current behavior, but now my PR exposes them in a weird way.

Current version:

image

My version:
image
image

This makes the odd behavior a bit more obvious, but I think the underlying issue is that the indent detection isn't semantic anyways, it's entirely based on the number of spaces compared to the configured indent size. The issue here is that a, b, c, d, and f (but not e and second a) are in the 0th scope, and e and second a are in the 1st scope, because tab settings are set to 4 spaces. I'm not sure what to do about this.

I should also mention that this will not do the appropriate indentation in Allman's style if the line above the { is selected, as mentioned in #53737, but I think that's probably ok, as correct behavior will be seen if the bracket is selected.

Previously, the indent was always to the left of the current line.
While this logic was easier to implement, it is inconsistent with the
behavior expected. This commit changes it to select the scope, rather
than the line's indent. This is not a complete fix for #49342, as this
does not add support to semantic detection, however, this will work in
probably 90% of the cases, and is a relatively straightforward fix.
@rebornix rebornix self-requested a review Jul 22, 2019
@rebornix rebornix self-assigned this Jul 22, 2019
@wmertens

This comment has been minimized.

Copy link

wmertens commented Sep 30, 2019

LGTM! Small change, big improvement, even if it's not a 100% fix.

@alexdima alexdima assigned alexdima and unassigned rebornix Feb 4, 2020
alexdima added 7 commits Feb 6, 2020
@alexdima alexdima removed the request for review from rebornix Feb 6, 2020
@alexdima alexdima added this to the February 2020 milestone Feb 6, 2020
@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Feb 6, 2020

Thank you!

@alexdima alexdima merged commit 519e5bb into microsoft:master Feb 6, 2020
4 checks passed
4 checks passed
linux
Details
windows
Details
darwin
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.