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

Select range feature #10621

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
142c3ed
Arranging selection logic
marcarro Jul 8, 2024
ef995cd
Basic select range feature: practically not functional
marcarro Jul 10, 2024
a81a177
Completed and corrected selected range extraction functionality
marcarro Jul 15, 2024
ea0ac53
Tried fixing nullability issues
marcarro Jul 19, 2024
0deb915
Commented out incomplete test. Must be finished
marcarro Jul 19, 2024
46932b3
Rebased to feature branch and updated according to new API
marcarro Jul 24, 2024
a939694
Changed null assert to empty assert according to ci test results
marcarro Jul 24, 2024
a814ca8
Refactored element selection logic into methods for easier readability
marcarro Jul 25, 2024
8ba59f2
Removed unnecessary using
marcarro Jul 25, 2024
cd6b24a
Added check for IsInsideProperHtmlContent
marcarro Jul 25, 2024
55669a7
Removed unnecessary iteration variable in FindContainingSiblignPair
marcarro Jul 25, 2024
8ee84de
nits and test fixes
marcarro Jul 26, 2024
eba1b01
Functioning tests and fixed selection issue
marcarro Jul 26, 2024
6656f7a
Added null check
marcarro Jul 26, 2024
31282c8
Test updates
marcarro Jul 30, 2024
04aa349
PR Feedback
marcarro Jul 30, 2024
8dbd672
Adapted to new TryGetSourceLocation and clarified ambiguous Range ref…
marcarro Aug 20, 2024
8a7d0bb
Changed tests to use VsLspFactory utiltiies for multi point selection
marcarro Aug 20, 2024
7d3c78a
PR Feedback
marcarro Aug 20, 2024
842b162
Added early return in ProcessMultiPointSelection
marcarro Aug 21, 2024
4bab4dd
No need to check for multipoint selection
marcarro Aug 21, 2024
4297e6f
PR Feedback
marcarro Aug 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -47,51 +51,169 @@ 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);

if (IsMultiPointSelection(context.Request.Range))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does a multi-point selection need special handling? Surely we need to find the right nodes to process either way?

Copy link
Author

Choose a reason for hiding this comment

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

True! This is fixed in later PR's but I'll go ahead and make this correction.

{
ProcessMultiPointSelection(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 componentNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>();
var endElementNode = GetEndElementNode(context, syntaxTree);
return (startElementNode, endElementNode);
}

// Make sure we've found tag
if (componentNode is null)
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)
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we can't handle self-closing tags, like <Comp$$onent />?

Copy link
Author

Choose a reason for hiding this comment

The 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 markupSyntaxNode.

{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
return true;
}

// 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)
return context.Location.AbsoluteIndex > startElementNode.StartTag.Span.End &&
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: Add a blank line between } and return, which is consistent with the bulk of this file.

context.Location.AbsoluteIndex < startElementNode.EndTag.SpanStart;
}

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this returning null here? Given something like:

<di$$v>
</div>

Thats a single point range, where the start tag is <div>, why would the end tag be null, and not the </div>?

This relates to my previous comment about why "multi-point" selection is handled differently

Copy link
Author

Choose a reason for hiding this comment

The 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 😅

}

if (!TryGetNamespace(context.CodeDocument, out var @namespace))
var endLocation = GetEndLocation(selectionEnd, context.CodeDocument);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need GetEndLocation to exist, this can just be var endAbsoluteIndex = context.SourceText.GetRequiredAbsoluteIndex(selectionEnd).

if (!endLocation.HasValue)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
return null;
}

var actionParams = new ExtractToNewComponentCodeActionParams()
var endOwner = syntaxTree.Root.FindInnermostNode(endLocation.Value.AbsoluteIndex, true);
if (endOwner is null)
{
return null;
}

// Correct selection to include the current node if the selection ends immediately after a closing tag.
if (endOwner is MarkupTextLiteralSyntax && string.IsNullOrWhiteSpace(endOwner.ToFullString()) && endOwner.TryGetPreviousSibling(out var previousSibling))
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a ContainsOnlyWhitespace() extension method that should be callable here, rather than realising the entire end tag as a string.

{
endOwner = previousSibling;
}

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 ProcessMultiPointSelection(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 = endElementNode.Span.End;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If the start element is an ancestor, keep the original end; otherwise, use the end of the end element
if (!startNodeContainsEndNode)
{
actionParams.ExtractEnd = endElementNode.Span.End;
}
// If the start element is an ancestor, keep the original end; otherwise, use the end of the end element
if (startNodeContainsEndNode)
{
actionParams.ExtractEnd = endElementNode.Span.End;
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a test that exercises this

Copy link
Author

Choose a reason for hiding this comment

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

Tests on this have been added in upcoming PR: #10760


// 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>
// <span>
// Selected text starts here<p>Some text</p>
// </span>
// <span>
// <p>More text</p>
// </span>
// Selected text ends here <span></span>
// </div>
// In this case, we need to find the smallest set of complete elements that covers the entire selection.
Copy link
Contributor

Choose a reason for hiding this comment

The 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)

//   <div>
//     {|result:<span>
//      {|selection:<p>Some text</p>
//     </span>
//     <span>
//       <p>More text</p>
//     </span>
//     <span></span>|}|}
//   </div>

if (!startNodeContainsEndNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you return early you can remove this check

{
// 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 IsMultiPointSelection(Range range) => range.Start != range.End;
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I'd get rid of this method. It's harder for me to reason about what "Multi point" means, than to just read the start and end checks


private static SourceLocation? GetEndLocation(Position selectionEnd, RazorCodeDocument codeDocument)
{
if (!codeDocument.Source.Text.TryGetSourceLocation(selectionEnd, out var location))
{
return null;
}

return location;
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: Consider adding a blank line between } and return

}

private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen(returnValue: true)] out string? @namespace)
Expand All @@ -100,4 +222,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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using Microsoft.CodeAnalysis.Razor.Protocol;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Newtonsoft.Json.Linq;
using Range = Microsoft.VisualStudio.LanguageServer.Protocol.Range;

namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor;

Expand Down
Loading