Skip to content

[LSP] Re-enable folding range provider#49070

Merged
allisonchou merged 8 commits intodotnet:masterfrom
allisonchou:LSPFoldingRange
Nov 6, 2020
Merged

[LSP] Re-enable folding range provider#49070
allisonchou merged 8 commits intodotnet:masterfrom
allisonchou:LSPFoldingRange

Conversation

@allisonchou
Copy link
Contributor

Fixes #48600

The folding range provider already existed, but wasn't enabled. Roslyn on the client was providing folding ranges instead (now disabled).
For the most part, everything looks to be working properly. I found one bug involving regions at the end of files that crashes folding ranges, but I believe it's an LSP issue (filed https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1240541/).

@allisonchou allisonchou added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Oct 30, 2020
@allisonchou allisonchou requested a review from a team as a code owner October 30, 2020 08:15
var linePositionSpan = text.Lines.GetLinePositionSpan(span.TextSpan);

// Filter out single line spans.
if (linePositionSpan.Start.Line == linePositionSpan.End.Line)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this check to match local Roslyn, which filters out single-line regions.

Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there existing tests for the folding ranges handler? If not we should add them

@allisonchou allisonchou requested a review from a team November 5, 2020 02:32
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only request would be to unify the checks into an extension method on document

@allisonchou
Copy link
Contributor Author

@dibarbet There are some existing tests, it looks like some are disabled though. The disabled tests pass if null is passed in for LSP.FoldingRangeKind. It looks like LSP doesn't even use FoldingRangeKind currently, so I'm not entirely sure what the purpose of the property is.

{
internal static class DocumentExtensions
{
public static bool IsInCloudEnvironmentClientContext(this Document document)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the existing ones - it looks like either IWorkspaceContextService isn't available in the layer (like the one you linked above), or the particular DocumentExtensions isn't accessible from the callsites :/

@allisonchou allisonchou merged commit fbeed20 into dotnet:master Nov 6, 2020
@ghost ghost added this to the Next milestone Nov 6, 2020
@allisonchou allisonchou deleted the LSPFoldingRange branch November 6, 2020 04:08
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE LSP issues related to the roslyn language server protocol implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support textDocument/foldingRange for C# LSP

2 participants