-
Notifications
You must be signed in to change notification settings - Fork 193
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
Select range feature #10621
Select range feature #10621
Changes from all commits
142c3ed
ef995cd
a81a177
ea0ac53
0deb915
46932b3
a939694
a814ca8
8ba59f2
cd6b24a
55669a7
8ee84de
eba1b01
6656f7a
31282c8
04aa349
8dbd672
8a7d0bb
7d3c78a
842b162
4bab4dd
4297e6f
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 |
---|---|---|
|
@@ -12,11 +12,15 @@ | |
using Microsoft.AspNetCore.Razor.Language; | ||
using Microsoft.AspNetCore.Razor.Language.Components; | ||
using Microsoft.AspNetCore.Razor.Language.Extensions; | ||
using Microsoft.AspNetCore.Razor.Language.Intermediate; | ||
using Microsoft.AspNetCore.Razor.Language.Syntax; | ||
using Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Models; | ||
using Microsoft.AspNetCore.Razor.Threading; | ||
using Microsoft.CodeAnalysis.Razor.Logging; | ||
using Microsoft.CodeAnalysis.Razor.Workspaces; | ||
using Microsoft.CodeAnalysis.Text; | ||
using Microsoft.VisualStudio.LanguageServer.Protocol; | ||
using Range = Microsoft.VisualStudio.LanguageServer.Protocol.Range; | ||
|
||
namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor; | ||
|
||
|
@@ -47,51 +51,149 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeAct | |
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
} | ||
|
||
var (startElementNode, endElementNode) = GetStartAndEndElements(context, syntaxTree, _logger); | ||
|
||
// Make sure the selection starts on an element tag | ||
if (startElementNode is null) | ||
{ | ||
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
} | ||
|
||
if (!TryGetNamespace(context.CodeDocument, out var @namespace)) | ||
{ | ||
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
} | ||
|
||
var actionParams = CreateInitialActionParams(context, startElementNode, @namespace); | ||
|
||
ProcessSelection(startElementNode, endElementNode, actionParams); | ||
|
||
var resolutionParams = new RazorCodeActionResolutionParams() | ||
{ | ||
Action = LanguageServerConstants.CodeActions.ExtractToNewComponentAction, | ||
Language = LanguageServerConstants.CodeActions.Languages.Razor, | ||
Data = actionParams, | ||
}; | ||
|
||
var codeAction = RazorCodeActionFactory.CreateExtractToNewComponent(resolutionParams); | ||
return Task.FromResult<ImmutableArray<RazorVSInternalCodeAction>>([codeAction]); | ||
} | ||
|
||
private static (MarkupElementSyntax? Start, MarkupElementSyntax? End) GetStartAndEndElements(RazorCodeActionContext context, RazorSyntaxTree syntaxTree, ILogger logger) | ||
{ | ||
var owner = syntaxTree.Root.FindInnermostNode(context.Location.AbsoluteIndex, includeWhitespace: true); | ||
if (owner is null) | ||
{ | ||
_logger.LogWarning($"Owner should never be null."); | ||
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
logger.LogWarning($"Owner should never be null."); | ||
return (null, null); | ||
} | ||
|
||
var startElementNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>(); | ||
if (startElementNode is null || IsInsideProperHtmlContent(context, startElementNode)) | ||
{ | ||
return (null, null); | ||
} | ||
|
||
var endElementNode = GetEndElementNode(context, syntaxTree); | ||
return (startElementNode, endElementNode); | ||
} | ||
|
||
private static bool IsInsideProperHtmlContent(RazorCodeActionContext context, MarkupElementSyntax startElementNode) | ||
{ | ||
// If the provider executes before the user/completion inserts an end tag, the below return fails | ||
if (startElementNode.EndTag.IsMissing) | ||
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 mean we can't handle self-closing tags, like 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. Nope, this check corrects for a case where the provider executes while in the middle of typing an end tag, or right after inserting a starting tag (in those few seconds where the completion doesn't insert its corresponding end tag yet). Anyway, this is removed in the latest PR where instead of doing this check I check for error diagnostics on a |
||
{ | ||
return true; | ||
} | ||
|
||
var componentNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>(); | ||
return context.Location.AbsoluteIndex > startElementNode.StartTag.Span.End && | ||
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. tiny nit: Add a blank line between |
||
context.Location.AbsoluteIndex < startElementNode.EndTag.SpanStart; | ||
} | ||
|
||
// Make sure we've found tag | ||
if (componentNode is null) | ||
private static MarkupElementSyntax? GetEndElementNode(RazorCodeActionContext context, RazorSyntaxTree syntaxTree) | ||
{ | ||
var selectionStart = context.Request.Range.Start; | ||
var selectionEnd = context.Request.Range.End; | ||
if (selectionStart == selectionEnd) | ||
{ | ||
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
return 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. Why is this returning null here? Given something like:
Thats a single point range, where the start tag is This relates to my previous comment about why "multi-point" selection is handled differently 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. You're right, endElementNode should not be null, the new PR accounts for this and also handles processing selections differently, in general. Now I don't know if I should bring all of those changes into this branch or just wait for the new PR's to merge 😅 |
||
} | ||
|
||
// Do not provide code action if the cursor is inside proper html content (i.e. page text) | ||
if (context.Location.AbsoluteIndex > componentNode.StartTag.Span.End && | ||
context.Location.AbsoluteIndex < componentNode.EndTag.SpanStart) | ||
var endAbsoluteIndex = context.SourceText.GetRequiredAbsoluteIndex(selectionEnd); | ||
var endOwner = syntaxTree.Root.FindInnermostNode(endAbsoluteIndex, true); | ||
if (endOwner is null) | ||
{ | ||
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
return null; | ||
} | ||
|
||
if (!TryGetNamespace(context.CodeDocument, out var @namespace)) | ||
// Correct selection to include the current node if the selection ends immediately after a closing tag. | ||
if (endOwner is MarkupTextLiteralSyntax && endOwner.ContainsOnlyWhitespace() && endOwner.TryGetPreviousSibling(out var previousSibling)) | ||
{ | ||
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
endOwner = previousSibling; | ||
} | ||
|
||
var actionParams = new ExtractToNewComponentCodeActionParams() | ||
return endOwner.FirstAncestorOrSelf<MarkupElementSyntax>(); | ||
} | ||
|
||
private static ExtractToNewComponentCodeActionParams CreateInitialActionParams(RazorCodeActionContext context, MarkupElementSyntax startElementNode, string @namespace) | ||
{ | ||
return new ExtractToNewComponentCodeActionParams | ||
{ | ||
Uri = context.Request.TextDocument.Uri, | ||
ExtractStart = componentNode.Span.Start, | ||
ExtractEnd = componentNode.Span.End, | ||
ExtractStart = startElementNode.Span.Start, | ||
ExtractEnd = startElementNode.Span.End, | ||
Namespace = @namespace | ||
}; | ||
} | ||
|
||
var resolutionParams = new RazorCodeActionResolutionParams() | ||
/// <summary> | ||
/// Processes a multi-point selection to determine the correct range for extraction. | ||
/// </summary> | ||
/// <param name="startElementNode">The starting element of the selection.</param> | ||
/// <param name="endElementNode">The ending element of the selection, if it exists.</param> | ||
/// <param name="actionParams">The parameters for the extraction action, which will be updated.</param> | ||
private static void ProcessSelection(MarkupElementSyntax startElementNode, MarkupElementSyntax? endElementNode, ExtractToNewComponentCodeActionParams actionParams) | ||
{ | ||
// If there's no end element, we can't process a multi-point selection | ||
if (endElementNode is null) | ||
{ | ||
Action = LanguageServerConstants.CodeActions.ExtractToNewComponentAction, | ||
Language = LanguageServerConstants.CodeActions.Languages.Razor, | ||
Data = actionParams, | ||
}; | ||
return; | ||
} | ||
|
||
var codeAction = RazorCodeActionFactory.CreateExtractToNewComponent(resolutionParams); | ||
var startNodeContainsEndNode = endElementNode.Ancestors().Any(node => node == startElementNode); | ||
|
||
return Task.FromResult<ImmutableArray<RazorVSInternalCodeAction>>([codeAction]); | ||
// If the start element is an ancestor, keep the original end; otherwise, use the end of the end element | ||
if (startNodeContainsEndNode) | ||
{ | ||
actionParams.ExtractEnd = startElementNode.Span.End; | ||
return; | ||
} | ||
|
||
// If the start element is not an ancestor of the end element, we need to find a common parent | ||
// This conditional handles cases where the user's selection spans across different levels of the DOM. | ||
// For example: | ||
// <div> | ||
// {|result:<span> | ||
// {|selection:<p>Some text</p> | ||
// </span> | ||
// <span> | ||
// <p>More text</p> | ||
// </span> | ||
// <span> | ||
// </span>|}|} | ||
// </div> | ||
// In this case, we need to find the smallest set of complete elements that covers the entire selection. | ||
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 love this comment <3 BUT... it would be clearer to just write things as we would for test input, with the same markers, and also indicate what the expected resulting selection would be. We all need to be used to reading that syntax anyway. eg (and correct me if my guess is wrong)
|
||
|
||
// Find the closest containing sibling pair that encompasses both the start and end elements | ||
var (extractStart, extractEnd) = FindContainingSiblingPair(startElementNode, endElementNode); | ||
|
||
// If we found a valid containing pair, update the extraction range | ||
if (extractStart is not null && extractEnd is not null) | ||
{ | ||
actionParams.ExtractStart = extractStart.Span.Start; | ||
actionParams.ExtractEnd = extractEnd.Span.End; | ||
} | ||
// Note: If we don't find a valid pair, we keep the original extraction range | ||
} | ||
|
||
private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen(returnValue: true)] out string? @namespace) | ||
|
@@ -100,4 +202,59 @@ private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen | |
// and causing compiler errors. Avoid offering this refactoring if we can't accurately get a | ||
// good namespace to extract to | ||
=> codeDocument.TryComputeNamespace(fallbackToRootNamespace: true, out @namespace); | ||
|
||
private static (SyntaxNode? Start, SyntaxNode? End) FindContainingSiblingPair(SyntaxNode startNode, SyntaxNode endNode) | ||
{ | ||
// Find the lowest common ancestor of both nodes | ||
var nearestCommonAncestor = FindNearestCommonAncestor(startNode, endNode); | ||
if (nearestCommonAncestor == null) | ||
{ | ||
return (null, null); | ||
} | ||
|
||
SyntaxNode? startContainingNode = null; | ||
SyntaxNode? endContainingNode = null; | ||
|
||
// Pre-calculate the spans for comparison | ||
var startSpan = startNode.Span; | ||
var endSpan = endNode.Span; | ||
|
||
foreach (var child in nearestCommonAncestor.ChildNodes().Where(static node => node.Kind == SyntaxKind.MarkupElement)) | ||
{ | ||
var childSpan = child.Span; | ||
|
||
if (startContainingNode == null && childSpan.Contains(startSpan)) | ||
{ | ||
startContainingNode = child; | ||
if (endContainingNode is not null) | ||
break; // Exit if we've found both | ||
} | ||
|
||
if (childSpan.Contains(endSpan)) | ||
{ | ||
endContainingNode = child; | ||
if (startContainingNode is not null) | ||
break; // Exit if we've found both | ||
} | ||
} | ||
|
||
return (startContainingNode, endContainingNode); | ||
} | ||
|
||
private static SyntaxNode? FindNearestCommonAncestor(SyntaxNode node1, SyntaxNode node2) | ||
{ | ||
var current = node1; | ||
|
||
while (current.Kind == SyntaxKind.MarkupElement && current is not null) | ||
{ | ||
if (current.Span.Contains(node2.Span)) | ||
{ | ||
return current; | ||
} | ||
|
||
current = current.Parent; | ||
} | ||
|
||
return null; | ||
} | ||
} |
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.
Should this return false instead of true?
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.
Yes