Intend on paste

[Giggiux]

This is a WIP, in particular for where the file where the code is situated, and because it's not properly tested.

The changes will work better if integrated with the changes in PR scalameta/metals-vscode#572.

This PR, merged with the previously linked one to metals-vscode should close scalameta/metals-feature-requests#169, by adding automatic on paste indentation and automatic auto increase and decrease of indentation for optional braces.

The indent on paste works on "rangeFormatting", it assumes that the first line pasted is correctly indented, and then checks and computes, based on that line indentation, the correct indentation of the following pasted lines.

The main issue I find in the current solution is that we don't have any indentation configuration, so we don't know if the user is using tabs or spaces, and how many for each indent. What we do, is try to check in a simple way the current file indentation, otherwise will go to a default "two spaces" indentation. Anyway, this solution should not break the user's code even if the indentation is not exactly the same in the whole file. And Scalafmt can always be run to fix any wrong indentation.
Also, the code is currently situated in the MultilineStringFormattingProvider, but that was just for simplicity of implementation, I do not expect to leave it there.

[tgodzik]

We would welcome all comments/suggestions, so that we can be sure that the suggested solution is widely accepted. This heavily influences the UX and the overall experience of writing in Scala so it is important to get it right.

We are using here OnTypeFormatting again, but I see no better solution for this in Metals. Python Vs Code extension does it via separate paste command, but that I think would not be that visible to the users.

[ckipp01]

The indent on paste works on "rangeFormatting", it assumes that the first line pasted is correctly indented, and then checks and computes, based on that line indentation, the correct indentation of the following pasted lines.

I barely have any experience with indentation based syntax, but I'll really interested to see how this works. From playing around a bit with it, this sounds easy, but is actually a bit tricky unless the lines you are copying from has the exact indentation you already want, the first line won't be right. I don't know how to get around this.

The main issue I find in the current solution is that we don't have any indentation configuration, so we don't know if the user is using tabs or spaces, and how many for each indent.

Actually this information should be contained in the FormattingOptions. It's required to be sent it and has both tabSize and insertSpaces. https://microsoft.github.io/language-server-protocol/specification#textDocument_formatting

Also, the code is currently situated in the MultilineStringFormattingProvider, but that was just for simplicity of implementation, I do not expect to leave it there.

👍🏼 big thumbs up on this. Actually this may be a great time to get a better pattern in place to make it easier to add other features via onTypeFormatting. Right now it's pretty tightly coupled with multi-line strings, but there are other tickets that would be pretty easy to tackle if it was more decoupled, like scalameta/metals-feature-requests#99

[Giggiux]

is actually a bit tricky unless the lines you are copying from has the exact indentation you already want, the first line won't be right

Essentially that's why it assumes that the first line pasted is correctly indented and it will work best (in vscode) with the changes in PR scalameta/metals-vscode#572: it has support for indentation in scala3.

Actually this information should be contained in the FormattingOptions.

The only issue I can find is that the FormattingOptions are sent for each textDocument/formatting request, so in order to use it with rangeFormatting, assuming that those options are unique in the whole project, we would have to save them from a formatting request. To do so, we could force/fake a formatting request on startup to save the FormattingOptions somewhere to reuse them, but this would be very unresponsive, and also just a hackish workaround.
If you have any different approaches in mind, let me know!

In the meantime the current approach is to search of the correct indentation in a very simple way: checking in the current file where there should be already an indent and using what's found (if tabs or spaces, and if spaces, how many).

[ckipp01]

The only issue I can find is that the FormattingOptions are sent for each textDocument/formatting request, so in order to use it with rangeFormatting, assuming that those options are unique in the whole project, we would have to save them from a formatting request.

Ahh, I was assuming that it'd be coming in during the onTypeFormatting request, which wouldn't require a save.

[Giggiux]

Maybe you're right: not on onTypeFormatting but on rangeFormatting we also have the FormattingOptions: the DocumentRangeFormattingParams for each request also have options: FormattingOptions.
So it may actually work fine!

I'll try and write a solution using these options!

[Giggiux]

@ckipp01 with the latest commit I added a very basic abstraction of the onTypeFormatting and rangeFormatting provider, not having everything anymore in multilineStringFormattingProvider, that essentially doesn't exist anymore as a provider but has been repurposed as an OnTypeRangeFormatter.
I tried to use a similar structure to the one we use in CodeActionProvider, but maybe there is a better solution to which I'm open to implementing!

I still need to add tests, will do it in the next commits!

[laughedelic]

but is actually a bit tricky unless the lines you are copying from has the exact indentation you already want, the first line won't be right.

I agree with this. as a vim(-mode) user, I often copy a set of lines (in visual-line mode) and when they are pasted, it doesn't matter where the cursor stands, insert will start on the next line with whatever indentation it originally had. I haven't tried this yet, but based on the description, I think to be able to use this functionality, I would need to switch to insert mode and place the cursor in the right position before pasting. It might sound vim-specific, but I'm sure other editors have similar behavior when copying several lines as a whole (e.g. by selecting via gutter).

I've written Haskell before and some editor plugin had this functionality (long time ago, nothing fancy). As far as I remember, the logic there was similar, but based indentation on the previous line, so if you have an open indentation block, it would paste inside that block (i.e. increase indentation), if the previous line doesn't open a block, it would paste on the same indentation level. Just an idea to consider.

[Giggiux]

I should've added support for the vim(-mode) users: now the indentation happens also if the first line is not correctly indented.
Changing this also added some weird behavior that I had to fix, but now it should work.

Anyone that can help me find unsupported edge cases that I did not got right, is welcome!
An example of such edge case is the when parsing after a multi-line string:

object Main:
  val myMultilineString: String = """| My
                                     | string""".stripMargin
  //@@ pasting here
end Main

the correct indentation found to indent the first line should not be considered the one of the piped line ( | string) but the one of the actual string declaration (val myMultilineString ...)

[tgodzik]

Thanks for the great work! I have some preliminary comments, but haven't dug into IndentOnPaste too much.

[tgodzik]

The indentation should not have changed here I think

[Giggiux]

This case is not handled by the MultilineStringFormatter, or at least it seems it's not, so the rangeFormatter falls back on the IndentOnPaste formatter that tries to indent it as normal code, and not multiline strings.

[tgodzik]

Hmm... but if the code is already indented in the paste snippet, shouldn't we keep the indentation? The same as with any block that has parts of it indented.

[Giggiux]

I haven't actually checked the MultilineString's code but if the MultilineString formatter doesn't return an empty list, but a None, the formatting "passes" to the IndentOnPaste that doesn't consider "correct" the multiline-string formatting, or well, in this particular cases behave like this:

1. finds the correct indentation for the first line
2. finds the correct indentation for the second line based on the first one
3. computes all other lines according to the difference between the pasted indentation and the second line

It works like this because, as already discussed, the first line can either be pasted from the beginning of the line having references the copied indentation, or be pasted already in the correct indentation level not having those references. So the computation for the difference in indentation between the pasted lines and the current file is done comparing the second line to the others.

[tgodzik]

However, in this case val is indented. We could tweak the algorithm in this case to adjust the intentation on the next lines starting with the second:

newIndentation = secondLineIndentation - firstLineIndentation + caluclatedIndetationForFirstline

Otherwise, if the second line is indented, we will always get it wrong.

Or, we could special case | and other operators to be always indented, which would work in most cases I think

[tgodzik]

Do we need to pass splitLines? That's an implementation detail of a particular OnTypeRangeFormatter

[Giggiux]

Since splitLines are used both in the MultilineString and in the IndentOnPaste formatters, I thought it would be more "efficient" to avoid having to repeat the same splitting operation of the entire sourceCode for both formatters.

[tgodzik]

ok sure, might be needed then. Alternatively we can make it a lazy val in a case class that would contain more of the parameters, this way it will only be used if needed.

[Giggiux]

Which parameters do you think are the best to keep in the formatting API, + the new case class?

[tgodzik]

If we need everything then it can probably be all the existing parameters.

[Giggiux]

I tried to implement some sort of abstraction, it may need another iteration or two to be good enough, tho.

[tgodzik]

Why check for trimmed.startsWith("|") ? We should probably tokenize the line and see if it's something that can start stat.

We could modify https://github.com/scalameta/scalameta/blob/625b18b28860d91a9f0cbaf423c1ba8dc344ecb4/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScalametaParser.scala#L1074

[Giggiux]

Essentially it's to support this case in the simplest way possible:

Anyone that can help me find unsupported edge cases that I did not got right, is welcome!
An example of such edge case is the when parsing after a multi-line string:

object Main:
  val myMultilineString: String = """| My
                                     | string""".stripMargin
  //@@ pasting here
end Main

the correct indentation found to indent the first line should not be considered the one of the piped line ( | string) but the one of the actual string declaration (val myMultilineString ...)

[tgodzik]

Ach yeah! After thinking a bit I realized the reason for it. I wonder though if we would be able to cover all possible cases. Since you can also have for example (I think that is only in Scala 3):

  val intNum = 123 
    + 123

Not sure what other corner cases we can have.

[tgodzik]

I would just keep whatever indentation we have in paste adjsuted by the first lines indentation, so if:
newfirstLineIndent = 2, secondLineIndent = 4 => newSecondLineIndent = 2
newfirstLineIndent = 10, secondLineIndent = 4 => newSecondLineIndent = 12

[Giggiux]

This part of the code checks if the first pasted line needs to have increased indentation compared to the non-pasted code line above.
Suppose you're pasting right after an if:

def function():
  if (1 < 2):
    @@ pasting here

Since we're not assuming anymore that we have the first line indented correctly due to @laughedelic suggestion, we need to compute the first line correct indentation based on the previous one.
The rest of the code, essentially uses the idea you propose.

Also related to this, @dos65 tested that vim-mode for VSCode doesn't actually trigger rangeFormatting when pasting, but maybe using coc-metals it does?

[dos65]

@Giggiux I checked what is wrong with windows tests - that was the line separator.
Suggested changes should fix them.

[tgodzik]

I went ahead and simplified the code a bit, I didn't change any implementation details, but removed some classes and methods that were adding to the complexity.

I also moved all the new classes to the formatting package. I will follow up later and add the remaining things connected to formatting there.

[tgodzik]

Instead of using contribute method I just inlined everything and removed the parent class. This was just adding to the cognitive overload.

[tgodzik]

Just tested it out after the changes and everything seems to work ok. We can still improve it a bit, but we can do it in the successive PRs