-
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
Send back icons for FAR via LSP #47882
Changes from 13 commits
fe82ec6
8b9f26d
d66e4e5
e30456b
3abf5c2
c703f1a
0b60e89
c5312b1
a805ec6
1213208
711e42d
b206219
9f10b94
20bf6f1
263c416
eab8465
e380fe6
51bd904
0f09454
9b05079
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 |
---|---|---|
|
@@ -43,6 +43,12 @@ public FindAllReferencesHandler(IMetadataAsSourceFileService metadataAsSourceFil | |
return Array.Empty<LSP.VSReferenceItem>(); | ||
} | ||
|
||
// We only support streaming results back, so no point doing any work if the client doesn't | ||
if (referenceParams.PartialResultToken == null) | ||
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. Once pull diagnostics goes in we should switch this to use the progress implementation there. 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. 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. Woops, closed mine as dupe! |
||
{ | ||
return Array.Empty<LSP.VSReferenceItem>(); | ||
} | ||
|
||
var findUsagesService = document.GetRequiredLanguageService<IFindUsagesLSPService>(); | ||
var position = await document.GetPositionFromLinePositionAsync( | ||
ProtocolConversions.PositionToLinePosition(referenceParams.Position), cancellationToken).ConfigureAwait(false); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.Editor.Shared.Extensions; | ||
using Roslyn.Test.Utilities; | ||
using Xunit; | ||
using LSP = Microsoft.VisualStudio.LanguageServer.Protocol; | ||
|
@@ -43,7 +46,7 @@ void M2() | |
Assert.Equal("M", results[1].ContainingMember); | ||
Assert.Equal("M2", results[3].ContainingMember); | ||
|
||
AssertValidDefinitionProperties(results, 0); | ||
AssertValidDefinitionProperties(results, 0, Glyph.FieldPublic); | ||
} | ||
|
||
[WpfFact(Skip = "https://github.com/dotnet/roslyn/issues/43063")] | ||
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. does this need to be skipped still? 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. I can't see the build results in the linked issue to see why they failed, but with the handler only supporting streaming I can't see how they ever passed, so was planning on unskipping when adding non-streaming support. |
||
|
@@ -78,7 +81,7 @@ void M2() | |
Assert.Equal("M", results[1].ContainingMember); | ||
Assert.Equal("M2", results[3].ContainingMember); | ||
|
||
AssertValidDefinitionProperties(results, 0); | ||
AssertValidDefinitionProperties(results, 0, Glyph.FieldPublic); | ||
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. Do we want some test for something other than FieldPublic? |
||
} | ||
|
||
[WpfFact] | ||
|
@@ -114,12 +117,13 @@ void M() | |
Assert.NotNull(results[0].Location.Uri); | ||
} | ||
|
||
private static LSP.ReferenceParams CreateReferenceParams(LSP.Location caret) => | ||
private static LSP.ReferenceParams CreateReferenceParams(LSP.Location caret, IProgress<object> progress) => | ||
new LSP.ReferenceParams() | ||
{ | ||
TextDocument = CreateTextDocumentIdentifier(caret.Uri), | ||
Position = caret.Range.Start, | ||
Context = new LSP.ReferenceContext(), | ||
PartialResultToken = progress | ||
}; | ||
|
||
private static async Task<LSP.VSReferenceItem[]> RunFindAllReferencesAsync(Solution solution, LSP.Location caret) | ||
|
@@ -129,17 +133,23 @@ private static LSP.ReferenceParams CreateReferenceParams(LSP.Location caret) => | |
SupportsVisualStudioExtensions = true | ||
}; | ||
|
||
var progress = new ProgressCollector<LSP.VSReferenceItem>(); | ||
|
||
var queue = CreateRequestQueue(solution); | ||
return await GetLanguageServer(solution).ExecuteRequestAsync<LSP.ReferenceParams, LSP.VSReferenceItem[]>(queue, LSP.Methods.TextDocumentReferencesName, | ||
CreateReferenceParams(caret), vsClientCapabilities, null, CancellationToken.None); | ||
await GetLanguageServer(solution).ExecuteRequestAsync<LSP.ReferenceParams, LSP.VSReferenceItem[]>(queue, LSP.Methods.TextDocumentReferencesName, | ||
CreateReferenceParams(caret, progress), vsClientCapabilities, null, CancellationToken.None); | ||
|
||
return progress.GetItems(); | ||
} | ||
|
||
private static void AssertValidDefinitionProperties(LSP.ReferenceItem[] referenceItems, int definitionIndex) | ||
private static void AssertValidDefinitionProperties(LSP.VSReferenceItem[] referenceItems, int definitionIndex, Glyph definitionGlyph) | ||
{ | ||
var definition = referenceItems[definitionIndex]; | ||
var definitionId = definition.DefinitionId; | ||
Assert.NotNull(definition.DefinitionText); | ||
|
||
Assert.Equal(definitionGlyph.GetImageId(), definition.DefinitionIcon.ImageId); | ||
|
||
for (var i = 0; i < referenceItems.Length; i++) | ||
{ | ||
if (i == definitionIndex) | ||
|
@@ -148,9 +158,22 @@ private static void AssertValidDefinitionProperties(LSP.ReferenceItem[] referenc | |
} | ||
|
||
Assert.Null(referenceItems[i].DefinitionText); | ||
Assert.Equal(0, referenceItems[i].DefinitionIcon.ImageId.Id); | ||
Assert.Equal(definitionId, referenceItems[i].DefinitionId); | ||
Assert.NotEqual(definitionId, referenceItems[i].Id); | ||
} | ||
} | ||
|
||
private class ProgressCollector<T> : IProgress<object> | ||
davidwengier marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
private readonly List<T> _items = new List<T>(); | ||
|
||
public T[] GetItems() => _items.ToArray(); | ||
|
||
public void Report(object value) | ||
{ | ||
_items.AddRange((T[])value); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -423,7 +423,7 @@ static DiagnosticData CreateMockDiagnosticDataWithMappedLocation(Document docume | |
var diagnostic = Diagnostic.Create(descriptor, location); | ||
return new DiagnosticData(diagnostic.Id, | ||
diagnostic.Descriptor.Category, | ||
null, | ||
message: "", | ||
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. See earlier question: are we sure we don't have to deal with diagnostics with no message? |
||
null, | ||
diagnostic.Severity, | ||
diagnostic.DefaultSeverity, | ||
|
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.
Not using tags.ToImmutableArray().GetFirstGlyph().GetImageElement()? You added the extension method... 😄
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 think I need to start this PR again.. its been rebased so many times to different branches I don't even know what is real any more