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

Addresses single line if/for/while/etc indentation #115454

Closed

Conversation

WatDuhHekBro
Copy link

@WatDuhHekBro WatDuhHekBro commented Jan 30, 2021

Resolves #43244

This PR adds/changes indentation rules for languages with C-like single line control statements. While the majority of these changes can be implemented through the existing language-configuration.json options, there are some extra changes to the codebase that needs to be done.

So far, this PR implements the majority of these changes for C/C++, C#, Java, TS/JS/TSX/JSX, Objective C/C++, and PHP. JavaScript is shown below for example.

vsc-main

However, there was a notable problem with the new indentation rules in that the following would break. (^ represents the cursor)

Before:

if(foo)
{^}

After:

if(foo)
	{^}

To fix that, I added a new indentation rule to capture the previous line as well (so it can be taken into context).

vsc-nope

Using this new rule restricts the unindent to more specific cases.

If decreaseIndentPattern was used instead:

if(foo) {
	bar();
	
	^
}
if(foo) {
	bar();
	
{^}
}

Although it works, it's not as elegant as before. This is what the end result should look like.

notepad

Update: This part is working now!

update

To-Do

  • Add unindent for break statements for languages other than TS/JS/TSX/JSX (via onEnterRule)
  • Find a way to make an indent change preserve an auto-closing pair
  • Add tests
    • (All languages: C/C++, C#, Java, TS/JS/TSX/JSX, Objective C/C++, and PHP)
    • decreaseIndentWithPreviousLinePattern itself works
    • Single line...
      • if
      • else if
      • else
      • for
      • while
    • Bracket on the same line as control statement works
    • Bracket on the same line as control statement works after auto-closing and going to the next line
    • Bracket on the line after a control statement works
    • Bracket on the line after a control statement's auto-closing works
    • Bracket after case/default works
      • auto-closing
      • normal
    • case/default doesn't unindent after a normal statement
    • case/default does unindent after a break statement
    • decreaseIndentWithPreviousLinePattern doesn't affect a control statement where the previous line is just whitespace

@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@Devyre
Copy link

Devyre commented Feb 20, 2021

Not sure if this is known and covered by the TODO, but I noticed some weird behavior after the first single line indent (and seems to also happen with multiline).

Watch this:
https://streamable.com/4cr41o

@WatDuhHekBro
Copy link
Author

@Devyre Good catch, I didn't notice it before. Let me know if it works this time.

@WatDuhHekBro
Copy link
Author

As far as I can tell, for the purposes of this PR, src/vs/editor/contrib/linesOperations/test/moveLinesCommand.test.ts:272 doesn't need to be taken into account. indentRules is only used to construct mode which is just used for the language identifier.

That leaves src/vs/editor/test/browser/controller/cursor.test.ts:3199 to test decreaseIndentWithPreviousLinePattern and then the rest of the tests will be inside each language-specific extension because the actual indentation is based on the regex patterns defined there. Basically, the language-specific extension tests are to make sure that any changes to the regex doesn't break the new indentation.

@WatDuhHekBro
Copy link
Author

WatDuhHekBro commented Feb 23, 2021

From what I can tell, any modifications to src/ don't affect the imported vscode module in extensions/. So I believe the tests should run fine once these changes are taken into account, the major one being the combined auto-completion and indent action. Until then, the CI checks will fail.

As for implementing indentation tests for other base language extensions, I'd probably have to modify the package.json of all of them or make a separate directory <language>-language-features to uphold the conventions in place, and then maybe add a launch configuration. On the other hand... I could just cross my fingers and hope for the best. After all, there wasn't any language configuration testing for those other language extensions (or any testing for that matter).

Besides that though, I think it's somewhat ready for review now.

@WatDuhHekBro WatDuhHekBro marked this pull request as ready for review February 23, 2021 06:03
@WatDuhHekBro
Copy link
Author

One more thing I should point out is that decreaseIndentWithPreviousLinePattern is probably not the best solution. I noticed that the four existing indentation rules are kinda inflexible on their own. For example, increaseIndentPattern tests the current line and indents when you hit enter while decreaseIndentPattern tests the current line and immediately unindents when the pattern matches, and there is no unindent equivalent to indentNextLinePattern.

One way to solve this might be to unify indentationRules and onEnterRules. For some patterns like increaseIndentPattern, either rule set could work, while other rules like decreaseIndentPattern have no onEnterRules equivalent.

@rebornix
Copy link
Member

I'm not sure if we want to invest more in textmate indentation rules at this moment. I'm leaning towards no so closing this for now. Thank you for your contribution! If we think it's wroth exploring, we can still take this in in the future, but I'll leave this decision to @aiday-mar.

@rebornix rebornix closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor-autoindent Editor auto indentation issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect indentation for single line if/for/while/etc, multiline chaining statements etc
5 participants