Cursor location after formatting request

[olafurpg]

While trying out the new "metals.scalafmt.onSave": true I observed that the cursor moves to the bottom of the file after format. The server currently returns a single TextEdit covering the full range of the file

https://github.com/scalameta/metals/blob/a670ba9530a569b19b1b3859d11f0871a11d303f/metals/src/main/scala/scala/meta/metals/providers/DocumentFormattingProvider.scala#L66-L69

Maybe we can provide more fine grained edits so that the cursor is kept at the same token. Not sure how to accomplish this since LSP textDocument/formatting doesn't provide the current position of the cursor. Even if we knew where the cursor was, we'd have to extend scalafmt to return the offset of the cursor in the new formatted buffer (whch should be an easy fix).

[gabro]

I suspect this is due to Position(Int.MaxValue, Int.MaxValue). vscode-prettier calculates the end range precisely, and it seems to work fine.

[olafurpg]

Ah, that trick wasn't so amazing after all then 😅 Maybe we don't need any fancy tricks then

[gabro]

Ah, I've only now understood why I couldn't reproduce it.

I use the editor.formatOnSave: true option in VSCode, which LSP adheres to.

With that option, the cursor location is preserved. Have you tried this with other editors like Atom? /cc @laughedelic

Related: I propose we remove this option from the VSCode extension since it's completely against the guidelines.

[olafurpg]

I use editor.formatOnSave: false as well and never had problems with the cursor jumping. I suspect vscode preserves the line/column position.

Related: I propose we remove this option from the VSCode extension since it's completely against the guidelines.

I agree, there should not be two ways to accomplish the same thing, editor.formatOnSave: false is the canonical way to do this in vscode.

[gabro]

Here: https://github.com/scalameta/metals/pull/235

[laughedelic]

With that option, the cursor location is preserved. Have you tried this with other editors like Atom?

I didn't see VSCode implementation of this feature, but it's definitely something VSCode-specific. In Atom IDE there is also such general format-on-save, which is implemented differently from the formatting on willSaveWaitUntil request (for historical reasons), but it doesn't preserve the cursor position either.

I think there are two parts of this issue:

• Response to the format request should be more precise. As mentioned by @olafurpg:

  scalafmt FormatWriter can give precise diff per token if we want

  This could be also better for reformatting big files, because it's faster for the editor to apply severa small diffs than to replace the whole buffer (I'm just speculating though).

• The way clients handle TextEdits in general and whole file formatting in particular should be improved in the clients. We cannot do much about it here since it's out of the LSP scope, it's just a general UX problem.

Related: I propose we remove this option from the VSCode extension since it's completely against the guidelines.

I agree, there should not be two ways to accomplish the same thing, editor.formatOnSave: false is the canonical way to do this in vscode.

See https://github.com/scalameta/metals/pull/235#issuecomment-378652734. The nuance is that these two things are not exactly the same (metals.scalafmt.onSave is only about Metals).

[gabro]

This could be also better for reformatting big files, because it's faster for the editor to apply severa small diffs than to replace the whole buffer (I'm just speculating though).

About this, I've quickly checked what other plugins do and they all seem to replace the whole buffer: I've checked vscode-prettier, vscode-go, vscode-beautify.

[yyadavalli]

FYI, using end = Position(Int.MaxValue, Int.MaxValue) is causing formatting to fail when using the server with emacs lsp-mode.

https://github.com/emacs-lsp/lsp-mode/issues/386

[gabro]

Ah, thank you for reporting this @yyadavalli. I've opened an issue to track this, it's an easy fix in case you want to send a PR :)

https://github.com/scalameta/metals/issues/315

[olafurpg]

LSP doesn't expose an option for the server to communicate where the new cursor position should be so I propose to close this as a won't fix. At least we no longer return Int.MaxValue, which is an improvement