Skip to content
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

[LSP] Re-enable folding range provider #49070

Merged
merged 8 commits into from
Nov 6, 2020

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
@@ -58,11 +58,17 @@ public async Task<FoldingRange[]> HandleRequestAsync(FoldingRangeParams request,

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 as a code owner 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