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 line autoindent#14390 #23248

Closed

Conversation

ice-blaze
Copy link

Main issue #14390
Another Issue in which it could be related #22093.

Questions, is there a way to test performances ?
The algorithm work like this:

  • When moving lines, it will inherit the whitespaces of the next line.
  • In some cases depending on the start or end of the line, if it's one of thoses chars []{}(): it will indent correctly the line

I created some tests, but I need to have some other feedbacks:

  • Does the tests cover all the cases ?
  • Could be possible performence issue ?
  • Anything else that could be bad ?

For the issue #22093, they are talking aboit a different approache, after moving the lines, we use an auto indent functionality. Why not I would say but it seems for now the current autoindent feature isn't working well for selected code.

@mention-bot
Copy link

@ice-blaze, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @egamma to be potential reviewers.

@rebornix rebornix self-assigned this Mar 27, 2017
@rebornix
Copy link
Member

Thanks @ice-blaze for your contribution :)

The reason that moving lines needs to honor indentation rules is making sure the indentation adjustment is following rules that Language can control. And we should make it as generic as possible because different languages understand indentation differently, making one case happy doesn't solve the problem.

When moving lines, it will inherit the whitespaces of the next line.

Inherit is reasonable, I may implement it in this way as well. But a failing case is Comments. If next line happens to be a single line comment, I don't think inheriting from it is always a correct decision.

In some cases depending on the start or end of the line, if it's one of thoses chars []{}(): it will indent correctly the line

Well []{}() works well for JavaScript/TypeScript or similar languages, but there are two major problems with it

  1. We should check https://github.com/Microsoft/vscode/blob/master/extensions/typescript/language-configuration.json#L6 brackets configuration of a language, instead of hard code.
  2. def ... end block in Ruby doesn't work, which is a common case.

A simple solution in my mind right now is trying to get a good indentation guess for the first line of selected range, calculate the indentation difference, apply it the selected range and keep all others untouched. We are not Reindent the entire selected range so the change is minimal.

@ice-blaze
Copy link
Author

@rebornix Can you write an example where it won't work with the comments ?

Well in fact it's true that I don't cover all languages case with open blocks, but at least I cover major big languages (C/C++, Java/C#, Python, Php, Javascript). And at the end, for every languages I use the inherite of the next whitespace line. (except for some cases with comments if I understand right)

So maybe would it be better if I put only the inherit whitespace code in the vscode and for everything related on open blocks I create a plugin for it ?

For the Ruby, case I can add it.

Well I didn't know this file https://github.com/Microsoft/vscode/blob/master/extensions/typescript/language-configuration.json#L6 you are completly right, I should use it. Is there something equivalent for open blocks ? I guess not or you would already say it.

@ice-blaze
Copy link
Author

ice-blaze commented Mar 27, 2017

@rebornix Well I guess this case

line to move
// if (true) {
another line
// if (true) {
    line to move
another line

@jsonMartin
Copy link

Sweet, looking forward to this auto-indentation! It's the only remaining feature from Jetbrains IDEs that I'm missing in VS Code.

@ice-blaze
Copy link
Author

ice-blaze commented Apr 14, 2017

@jsonMartin Well I guess we could use the format feature, applied to the moving block or line.

I tried to implement it but the VSCode structure is too complex for me so I give up. But I guess if someone have ease to use the VSCode API, it could be quite easy to just make a call "reformat" after the moving line command.

@microsoft microsoft deleted a comment from msftclas Sep 27, 2017
@microsoft microsoft deleted a comment from msftclas Sep 27, 2017
@rebornix
Copy link
Member

Considering the complexity of indentation rules management, I'd like to close this PR for now and once we decide to put some time on it, we can reopen this PR and see how we can improve the experience for moving lines with auto indent turned on. Thanks for your help!

@rebornix rebornix closed this Oct 24, 2019
@ice-blaze
Copy link
Author

It's better to close it, I have no intent to change it the future.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 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.

6 participants