on_text_changed() is not working well with "revert" (F5) command

[jfcherng]

Description

When a revert (F5) command is triggered, on_text_changed() thinks all text is deleted.

Steps to reproduce

import sublime
import sublime_plugin


class MyTextChangedListener(sublime_plugin.EventListener):
    def on_text_changed(self, view, changes):
        for change in changes:
            print(
                change.a.pt, change.a.row, change.a.col,
                change.b.pt, change.b.row, change.b.col,
                change.str,
            )

1. Save the above snippet and trigger revert (F5)
2. We immediately get the two following outputs from the ST console.

0 0 0 350 12 0      // empty the whole buffer
0 0 0 0 0 0         // hmm...

Expected behavior

The 0 0 0 0 0 0 part should add the whole file back. Or something equivalent to delete all buffer content and add all disk content back during the whole revert process.

Actual behavior

No text is added back.

Environment

• Build: 4074
• Operating system and version: Win7 x64

Related Issue

• ST's "Revert" (F5) command makes server malfunctional sublimelsp/LSP#1015 (Workaround by Workaround on_text_changed bug messing up view content synchronization sublimelsp/LSP#1095)

[rchl]

This will also trigger when stashing and popping changes from SublimeMerge, for example. ST then refreshes the view and that messes up the state in language servers, causing them to report issues that don't exist or even changing the code in wrong ways when formatting-on-save is enabled.

[rwols]

The lowest common denominator seems to be file system events not passing through on_text_changed.

[rchl]

For the purposes of LSP I think it would be enough if we would get an extra change with all content re-added (assuming that that would be easier to do than actually providing a real diff of changes).

[kud]

God the same issue, yes, happens when jsprettier formats the code.

[BenjaminSchaaf]

Would it be better to have a separate notification for reverts? From my reading of the lsp specification it seems like the best action would be to send a didClose and then didOpen when we revert, rather than a diff?

[rchl]

I believe that would be fine too. I'm not sure that specific case fits more of a modification or close/open case as from user's perspective the file is modified rather than closed and reopened but in any case, I think it would be OK to handle it that way.

(By "reverts" I hope you mean the generic case of when the file is touched externally and not just the revert command.)

[BenjaminSchaaf]

(By "reverts" I hope you mean the generic case of when the file is touched externally and not just the revert command.)

Except for callbacks both those cases have identical code paths :)

[BenjaminSchaaf]

I believe that would be fine too. I'm not sure that specific case fits more of a modification or close/open case as from user's perspective the file is modified rather than closed and reopened but in any case, I think it would be OK to handle it that way.

I don't think it really matters if what the user sees matches up directly with what events get fired for a plugin. The way reload works internally is very similar to a close/reopen: We clear the whole buffer, then load it again off disk.

[rwols]

But a textDocument/didClose followed by a textDocument/didOpen may cause the server to re-index a bunch of files.

You could opt for invoking on_close followed by on_load (+ on_load_async). That would auto-fix the problem with no changes required :)

I would still prefer to see the changes through on_text_changed though.

Note that there's also another open issue that asks for callbacks for all filesystem events that ST tracks: #2669 so avoid duplicating information through callbacks (i.e. if a file is already open, FS events seep through on_text_changed, and no FS event is fired from the linked feature request)

[rchl]

Invoking existing on_close/on_load could confuse existing plugins if those expect the file to be removed from the UI on on_close.

[FichteFoll]

Invoking existing on_close/on_load could confuse existing plugins if those expect the file to be removed from the UI on on_close.

But shouldn't they react on on_load again to re-adjust to the new state in that case? In a sense, ST is closing and reopening the entire file with the only difference being the tab doesn't disappear for the user.

If it were to behave like this, all view-specific settings should also be removed, FWIW.

[BenjaminSchaaf]

It definitely won't trigger on_close/on_load. My idea was to add something like on_text_reload or similar, which can then do didClose, didOpen.

[wbond]

We’ve already added on_reload() and on_revert() handlers. This issue is about on_text_changed() providing an invalid value. If LSP wants to hook into a different event handler and ignore the text change after a revert/reload, that is fine, however I think we need to deliver the text that changed during a revert/reload via this function. There is more out there than LSP, after all.

[BenjaminSchaaf]

@wbond In that case I think the best solution would be to just remove the on_test_changed callback causing 0 0 0 0 0 0. If a plugin wants to keep track of what's currently in the buffer they'll need to use on_text_changed, on_reload and on_revert and if they want the full buffer contents after a reload they can use substr(), but it means we're not making a full copy of the entire buffer every time it's reverted and sending that across IPC to python. This would happen for any file size, so reverting a 2GB file would cause a ~6GB allocation for the main buffer, the copy on the ST side and the copy on the plugin side.

[rwols]

If you're going the route of depending on on_revert and on_reload, it's probably best to also not send the first text change where it says to remove the whole buffer.

I'm not convinced it would cause a 6GB allocation for a 2GB file. In practice reverting or reloading files changes a few lines in the buffer. So, calculating that diff in ST core would result in 4GB + text diff sent over IPC.

[jfcherng]

I think my original issue has been fixed in ST 4075. Hence closing.

[jfcherng]

Sorry seems not.

[BenjaminSchaaf]

Yea, this one didn't make it in for 4075.

[BenjaminSchaaf]

Build 4080 has completely reworked how on_text_changed functions. A new listener class is now available for this purpose: TextChangeListener. A TextChangeListener where is_applicable is instantiated per buffer (not per view). It receives the mutually exclusive on_text_changed, on_revert and on_reload events, along with their async counterparts.

[wbond]

My hope is to have the docs for this and other 4081 changes done early next week.