-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge changes from a single DidChange notification #74268
Changes from 2 commits
f489ce0
1c13d92
8db030b
7d98064
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,11 @@ | |
|
||
using System; | ||
using System.Composition; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.Host.Mef; | ||
using Microsoft.CodeAnalysis.Text; | ||
using Roslyn.LanguageServer.Protocol; | ||
using Roslyn.Utilities; | ||
|
||
|
@@ -28,15 +30,52 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(DidChangeTextDocumentPar | |
{ | ||
var text = context.GetTrackedDocumentSourceText(request.TextDocument.Uri); | ||
|
||
text = GetUpdatedSourceText(request.ContentChanges, text); | ||
|
||
context.UpdateTrackedDocument(request.TextDocument.Uri, text); | ||
|
||
return SpecializedTasks.Default<object>(); | ||
} | ||
|
||
private static SourceText GetUpdatedSourceText(TextDocumentContentChangeEvent[] contentChanges, SourceText text) | ||
{ | ||
var areChangesOrdered = true; | ||
|
||
// Per the LSP spec, each text change builds upon the previous, so we don't need to translate any text | ||
// positions between changes, which makes this quite easy. See | ||
// positions between changes. See | ||
// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#didChangeTextDocumentParams | ||
// for more details. | ||
foreach (var change in request.ContentChanges) | ||
text = text.WithChanges(ProtocolConversions.ContentChangeEventToTextChange(change, text)); | ||
// | ||
// If the host sends us changes in a way such that no earlier change can affect the position of a later change, | ||
// then we can merge the changes into a single TextChange, allowing creation of only a single new | ||
// source text. | ||
for (var i = 1; i < contentChanges.Length; i++) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add some tests for this (e.g. sending changes that overlap and don't overlap and verifying that the server ends up with the correct source text? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some unit tests, they aren't as clean as I had hoped, but it should at least give some coverage |
||
{ | ||
var prevChange = contentChanges[i - 1]; | ||
var curChange = contentChanges[i]; | ||
|
||
context.UpdateTrackedDocument(request.TextDocument.Uri, text); | ||
if (prevChange.Range.Start.CompareTo(curChange.Range.End) < 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its early and I'm not 100% sure of this. Is it possible we need to compare against all previous changes? For example we have two disparate changes, then one afterwards that overlaps just the first one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chatted in person, let me know if you still have concerns. |
||
{ | ||
areChangesOrdered = false; | ||
break; | ||
} | ||
} | ||
|
||
return SpecializedTasks.Default<object>(); | ||
if (!areChangesOrdered) | ||
{ | ||
// The host didn't send us the items ordered, so we'll apply each one independently. | ||
foreach (var change in contentChanges) | ||
text = text.WithChanges(ProtocolConversions.ContentChangeEventToTextChange(change, text)); | ||
} | ||
else | ||
{ | ||
// The changes were ordered, so we can merge them into a single operation on the source text. Note that | ||
// the original change ordering was in reverse document order, whereas the WithChanges implementation works | ||
// more efficiently with them in forward document order. | ||
var newChanges = contentChanges.Reverse().SelectAsArray(change => ProtocolConversions.ContentChangeEventToTextChange(change, text)); | ||
text = text.WithChanges(newChanges); | ||
} | ||
|
||
return text; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure there is a similar method implemented somewhere in Roslyn, but I couldn't find it