improvement: Don't sent text on didSave #7015
Conversation
Previously, we would have clients send us full text on didSave, which was never used. Now, we removed that option. I also removed reading the file from disk to buffers since that file should already be up to date form didChanged
| @@ -884,9 +884,6 @@ abstract class MetalsLspService( | |||
| ): CompletableFuture[Unit] = { | |||
| val path = params.getTextDocument.getUri.toAbsolutePath | |||
| savedFiles.add(path) | |||
| // read file from disk, we only remove files from buffers on didClose. | |||
| val text = path.toInput.text | |||
There was a problem hiding this comment.
This doesn't seem necessary and for large files might actually be problematic.
tgodzik
force-pushed
the
did-save
branch
3 times, most recently
from
82ff17b to
9fd01d9
Compare
| _ <- server.didChange(file)(_ => | ||
| code.replaceAll(allMarkersRegex, "") | ||
| ) | ||
| _ <- server.didSave(file)(identity) |
There was a problem hiding this comment.
Maybe we should do either of two:
- remove change function from
server.didChange(it will always de identity) - have
server.didSavewith the function send firstdidChangeand only laterdidSave.
| _.replace("n + 1", "n + 2") | ||
| ) | ||
| _ <- server.didSave("a/src/main/scala/a/Util.scala")(identity) | ||
| _ <- server.didFocus("a/src/main/scala/a/Main.worksheet.sc") |
There was a problem hiding this comment.
I don't think we should ever need didFocus, since it may not be supported, right?
There was a problem hiding this comment.
It's just focus switched to Util.scala and the worksheet doesn't evaluate. This will work in VS Code, thoguh wonder what happens if did focus is not supported
| @@ -785,6 +785,7 @@ final case class TestingServer( | |||
| val abspath = toPath(filename) | |||
| val oldText = abspath.toInputFromBuffers(buffers).text | |||
| val newText = fn(oldText) | |||
| buffers.put(abspath, newText) | |||
| if (!buffers.contains(path)) { | ||
| buffers.put(path, path.toInput.text) | ||
| } |
There was a problem hiding this comment.
| if (!buffers.contains(path)) { | |
| buffers.put(path, path.toInput.text) | |
| } | |
| buffers.putIfAbsent(path, path.toInput.text) |
I think that buffers is based on a concurrent map (TrieMap), so maybe it would be safer to perform an atomic operation here, in case multiple requests are processed in parallel. 🙂
Also, a question to didSave and didChange: it looks like currently they always overwrite the in-memory buffers content for the given file, but in case of concurrent requests we could apply the older data (race condition)? Seems that the DidCloseTextDocumentParams should contain document version, could we use that to always safely keep only the newest one? Sorry if I have misunderstood how this all works. 🙂
There was a problem hiding this comment.
You are very right. We want and we should use text document version simply nobody found the time to do it. I thought there was an issue for that but I could not find it now, here is a connected issue: #1665.
Previously, we would have clients send us full text on didSave, which was never used.
Now, we removed that option.
I also removed reading the file from disk to buffers since that file should already be up to date form didChanged