LSP icon indicating copy to clipboard operation
LSP copied to clipboard

Applying large text edits results in the viewport shifting to the right

Open uhthomas opened this issue 4 years ago • 12 comments

System information

macOS Catalina 10.15.5 (19F101) Sublime Text Dev Channel, Build 4074

Install

Package Control

Reproduction

Assuming the LSP configuration

{
    "lsp_format_on_save": true,
    "lsp_code_actions_on_save": {
        "source.fixAll": true,
        "source.organizeImports": true
    },
    "show_symbol_action_links": true,
    "only_show_lsp_completions": true,
    "clients": {
        "gopls": {
            "enabled": true,
            "settings": {
                "gopls.usePlaceholders": true,
                "gopls.analyses": {
                  "fillreturns": true,
                  "nonewvars": true,
                  "undeclaredname": true,
                  "unusedparams": true
                },
                "gopls.completeUnimported": true,
                "gopls.staticcheck": true
            }
        }
    }
}

Then, given the snippet

package main

import "fmt"

func main() {
	fmt.Println("Hello, world!"	) // <--- place your cursor here
}

Place your cursor in front of the arrow and then save. The cursor will jump to column 1.

uhthomas avatar Jun 19 '20 14:06 uhthomas

The server sends two changes to fix formatting. The first that removes the whole 6th line and the second that adds back formatted one.

Server response
[
  {
    "newText": "",
    "range": {
      "end": {
        "character": 0,
        "line": 6
      },
      "start": {
        "character": 0,
        "line": 5
      }
    }
  },
  {
    "newText": "\tfmt.Println(\"Hello, world!\") // <--- place your cursor here\n",
    "range": {
      "end": {
        "character": 0,
        "line": 6
      },
      "start": {
        "character": 0,
        "line": 6
      }
    }
  }
]

I think that fixing this would be quite tricky depending on how correct we want to be.

This case might seem simple on the surface but many won't be and the algorithm to anchor the cursor to the original place would likely be quite complex.

Ideally, we would reuse something that ST uses internally but I don't think there is any API exposed for that.

Maybe we could apply edits in a different way to fix it but again, I doubt it.

rchl avatar Jun 19 '20 19:06 rchl

Hah, interesting. It's definitely more complex than I first imagined. I wonder how editors like vscode, atom, Goland, etc. handle this case? sublimelsp is the only project I've noticed this sort of behaviour.

uhthomas avatar Jun 19 '20 20:06 uhthomas

We could try adding caret stability functionality but something general for all language servers is going to be complex. The first thing that comes to mind is to check whether there are edits on the caret's line, and if so, re-position the caret manually somehow.

rwols avatar Jun 19 '20 20:06 rwols

It might be a good idea to iterate on this process? IMO anchoring the caret at a fixed point is almost certainly better than sending it to column 1. It might be worth taking a look at vscode and seeing how they handle it, as the project is open source and also has a general purpose LSP.

uhthomas avatar Jun 19 '20 20:06 uhthomas

It's most likely handled at the core level of the editor. Maybe VSCode handles that case better. We are just using ST's APIs to remove and add regions as instructed by the server.

rchl avatar Jun 19 '20 21:06 rchl

I was thinking that we could process the payload ourselves to create a minimal change. We could do that by applying changes in memory and then creating minimal change from the diff. That would help this case and also the ridiculous case of pyls where it always returns back the whole document.

I believe this might be an implementation of such algorithm: https://github.com/microsoft/vscode-eslint/blob/master/server/src/diff.ts (in JS).

rchl avatar Jun 20 '20 19:06 rchl

That sounds like a great idea! Is there another open issue for pyls?

uhthomas avatar Jun 21 '20 13:06 uhthomas

There's also https://docs.python.org/3/library/difflib.html#sequencematcher-objects

rwols avatar Jun 22 '20 14:06 rwols

It should also be a server setting because for example clangd already has minimal text changes. It would be a waste of CPU cycles to do it again ourselves.

rwols avatar Jun 22 '20 14:06 rwols

Yes, eslint also sends minimal changes.

BTW, eslint implementation should give LSP compatible change chunks so it has advantage over more generic diff libraries. Just needs to be rewritten in python.

rchl avatar Jun 22 '20 14:06 rchl

FWIW I'm also seeing this while using rust-analyzer for LSP. Formatting is janking the viewport's scroll around.

acheronfail avatar Jul 12 '20 22:07 acheronfail

See: https://github.com/sublimehq/sublime_text/issues/4144

rwols avatar May 12 '21 23:05 rwols