Omnisharp should tell editors when it has changed files

[jrieken]

The Renamer and code actions modify the current solution which means Roslyn already has a newer state in memory than editors assume. This conflict with incremental buffer updates. Take this example

class Foo {}

1. now Foo is being renamed to B so that
2. Roslyn already has an updated representation of the document in memory but only after that Omnisharp responds to the editor with
3. either a full buffer or text changes. In the latter case, the editor will send incremental buffer updates which are not aware of the fact that those changes already have been applied: :boom: (in the former case we are lucky)

class B{}

becomes (select 3 characters and replace with B, basically applying the same text change again)

class B

To get around this unhappy situation and keep the incremental buffer updates, Omnisharp should notify its editor about changes it made to the solution/projects/document. There is WorkspaceChanged event (https://github.com/dotnet/roslyn/blob/master/src/Workspaces/Core/Portable/Workspace/WorkspaceChangeEventArgs.cs) which we could (should?) use for this.

[nosami]

I'm struggling to understand this. Possibly because I'm not using the TextChanges APIs yet.

What I don't understand is why the text changes aren't applied to the editor after the rename response. After the response has been set, then both editor and server should have the same representation.

Or is the problem that applying text changes to the buffer is triggering new text changes to the server?

[jrieken]

Exactly - applying the changes on the buffer is triggering it again on the server. I really don't like that Roslyn/Omnisharp is making these changes to a file while the editor is having it open. My plan is to use the event channel from #106 to track versions of documents - tho it makes things complicated.

[jrieken]

On a second though it might be us not using Roslyn properly. IMO it should not change its internal state but only compute edit operations such that the editor can drive it. There is https://github.com/dotnet/roslyn/blob/master/src/Workspaces/Core/Portable/Rename/RenameOptions.cs#L14 and I wonder if we have tried that. There is something similar with the CodeActions (https://github.com/dotnet/roslyn/blob/03e30451ce7eb518e364b5806c524623424103e4/src/Workspaces/Core/Portable/CodeActions/CodeAction.cs#L69) which I couldn't get to work.

@Pilchie - Can you advise? Is preview operations et al the way to go or is there another way?

[jrieken]

I figured that https://github.com/OmniSharp/omnisharp-roslyn/blame/master/src/OmniSharp/Api/Refactoring/OmnisharpController.Rename.cs#L69 is the cause of evil.

@davidfowl - was there a specific reason for adding that? Iff not I would remove it and have at least rename be proper.

[davidfowl]

I guess there's no reason to apply changes if you're going to send it on the exe request

[davidfowl]

Next*

[jrieken]

Removing TryApplyChanges conflicts with the assumptions baked into RenameFacts.cs @filipw Is checking for the source text modifications on purpose or do you agree that only editors (master) should modify the Roslyn state (slave)?

[nosami]

You're the only person sending text changes back currently after the rename. Maybe make the TryApplyChanges dependent on WantsTextChanges being set?

[jrieken]

hm, could be a pragmatic fix for this. tho I hope eventually everyone will use text changes :pray:

[Pilchie]

Okay, sorry for taking so long to get back to this. The way this is supposed to work is that a host of Roslyn (like omnisharp) should have it's own implementation of Workspace, and that should override TryApplyChanges to apply the changes in it's own, host specific way.

In MSBuildWorkspace, this means writing the saved files to disk. In VS, this means writing closed files to disk, and editing the contents of open files in the editor's ITextBuffer.

NOTE: It doesn't mean that you take the new solution and just use it. Instead, you apply the changes to your host however you need to for your host (perhaps sending them back across to Sublime/etc and making changes there), and then use whatever normal mechanism you have that ends up calling Workspace.OnDocumentTextUpdated() to flow that back into Workspace.CurrentSolution in the same way that it does when a user types.

http://source.roslyn.io/Microsoft.CodeAnalysis.Workspaces/R/7f724e05433af793.html

Hope this helps.

[jrieken]

Thanks - so we do have our own version of a workspace - OmnisharpWorkspace. It could be in charge of this but it is more complex to send back the changes via http/stdio during that call. Also we lack the notion of open (in an editor) and not open files.

[davidfowl]

Yes so this actually doesn't apply to the http protocol. You have to rely on the buffer being sent back for omnisharp v1 clients. For one using the emitter, we can send the change back on workspace changed events.

[jrieken]

plus the open, closed notion. TypeScript server does something similar: https://github.com/Microsoft/TypeScript/blob/master/src/server/protocol.d.ts#L310

[jrieken]

will be continued in #225