Select range feature #10621

marcarro · 2024-07-15T20:14:55Z

Summary of the changes

Users can now extract a selected range that spans multiple sibling elements, along with their parent (or not), depending on the case.

Fixes:

davidwengier · 2024-07-15T21:22:41Z

Is it possible to merge the other PR, to make this one easier? Or is that still blocked on something?

DustinCampbell · 2024-07-15T22:29:48Z

FWIW, the build failures can be avoided by building at the command-line with build.cmd before creating your PR or pushing changes.

DustinCampbell

Note that this PR must merge after #10578.

I added a couple of comments to the unit test below, but I can't review new code in this PR because it's hard to see what changes come from #10578 or deviate from it.

DustinCampbell · 2024-07-19T19:51:39Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+    // Holding off on this test until configured correctly (fails on CI)
+    //[Theory]


You don't need to comment-out tests, just add a Skip to the [Theory]

[Theory(Skip = "Holding off on this test until configured correctly (fails on CI)")]

DustinCampbell · 2024-07-19T20:03:25Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+    //[InlineData("""
+    //<div id="parent">
+    //    [|<div>
+    //        <h1>Div a title</h1>
+    //        <p>Div a par</p>
+    //    </div>|]
+    //    <div>
+    //        <h1>Div b title</h1>
+    //        <p>Div b par</p>
+    //    </div>
+    //</div>
+    //""")]
+    //[InlineData("""
+    //<div id="parent">
+    //    <div>
+    //        <h1>Div a title</h1>
+    //        [|<p>Div a par</p>|]
+    //    </div>
+    //    <div>
+    //        <h1>Div b title</h1>
+    //        <p>Div b par</p>
+    //    </div>
+    //</div>
+    //""")]
+    //[InlineData("""
+    //<div id="parent">
+    //    <div>
+    //        <h1>Div a title</h1>
+    //        [|<p>Div a par</p>
+    //    </div>
+    //    <div>
+    //        <h1>Div b title</h1>
+    //        <p>Div b par</p>|]
+    //    </div>
+    //</div>
+    //""")]


This [InlineData] is far too bulky, IMO. Please consider one of the following:

Switch over to [MemberData]. There are plenty of examples in the repo of using [MemberData] rather than [InlineData].

Create three separate [Fact] methods that call this code with different data. (You'd want to switch this to a private method in this case.) @davidwengier has been writing a lot of tests in this style for co-hosting. Here's an example.

For tests like this one, I'll often go with the second approach. It can be helpful when a test fails to clearly see what it's input was, without having to go dig through an array of member data.

davidwengier

Thank you for rebasing! I'm a little unsure of exactly what all of the logic is doing though, and whether it's all necessary. Would love to see some more tests and perhaps a few more comments explaining things.

davidwengier · 2024-07-24T22:45:07Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        if (selectionEnd is not null && selectionEnd.Line < selectionStart.Line ||
+           (selectionEnd is not null && selectionEnd.Line == selectionStart.Line && selectionEnd.Character < selectionStart.Character))
+        {
+            (selectionEnd, selectionStart) = (selectionStart, selectionEnd);
+        }


Does this ever actually happen? I'm surprised the LSP client would ever send us a backwards range

Good catch, indeed the LSP client does not send a backwards range

davidwengier · 2024-07-24T22:49:43Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+                return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
+            }
+
+            endComponentNode = endOwner.FirstAncestorOrSelf<MarkupElementSyntax>();


I'm not quite sure how this node ends up being helpful. I understand endOwner is the node at the end of the selection, but looking up the tree for a markup element seems surprising. Can you maybe add a comment to illustrate?

For example, given a selection (between [| and |] of:

[|<div> <div> </div>]] </div>

It seems like startComponentNode would be the first div, and endComponentNode would be the second div. If the selection is extended to last closing div tag, then I think both startComponentNode and endComponentNode would be the same.

davidwengier · 2024-07-24T22:50:25Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        if (startComponentNode is null)
        {
            return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
        }


Probably should move this to above the previous bit of logic, as there is no point finding an end node if there is no start node

davidwengier · 2024-07-24T22:50:53Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

@@ -54,17 +57,55 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeAct
            return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
        }

-        var componentNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>();
+        var startComponentNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>();


Nit on naming of this, and endComponentNode, they're not necessarily components, just elements.

davidwengier · 2024-07-24T22:55:56Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+            // If component @ start of the selection includes a parent element of the component @ end of selection, then proceed as usual.
+            // If not, limit extraction to end of component @ end of selection (in the simplest case)
+            var selectionStartHasParentElement = endComponentNode.Ancestors().Any(node => node == startComponentNode);
+            actionParams.ExtractEnd = selectionStartHasParentElement ? actionParams.ExtractEnd : endComponentNode.Span.End;


Okay, I think this bit of logic answers my comment above, and explains what the endComponentNode is for. I wonder if this can be moved to that block to make things clearer, and maybe even have the whole thing extracted to a separate method?

A method that essentially answers the question "Given this start node and selection end, what is the best end position to use for extraction?"

davidwengier · 2024-07-24T23:00:53Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

@@ -100,4 +160,64 @@ 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);
+
+    public (SyntaxNode? Start, SyntaxNode? End) FindContainingSiblingPair(SyntaxNode startNode, SyntaxNode endNode)


This should be private, and hopefully static too

davidwengier · 2024-07-24T23:01:11Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        return (startContainingNode, endContainingNode);
+    }
+
+    public SyntaxNode? FindLowestCommonAncestor(SyntaxNode node1, SyntaxNode node2)


Nit: This is navigating up the tree, through node1s parents, so it is probably better being called FindHighestCommonAncestor. Having said that, "ancestor" implies that anyway, so its really FindNearestCommonAncestor

This should be private, and hopefully static too

davidwengier · 2024-07-24T23:06:16Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        var startSpan = startNode.Span;
+        var endSpan = endNode.Span;
+
+        foreach (var child in lowestCommonAncestor.ChildNodes().Where(node => node.Kind == SyntaxKind.MarkupElement))


Given this starts from a common ancestor, doesn't that mean by definition it will contain both the start and end spans, and therefore this loop will only ever run one iteration, and always return both endContainingNode and startContaininedNode as being the same node as lowestCommonAncestor?

I might be missing something though. Again, so illustrative comments would be helpful.

The ChildNodes() method won't include the ancestor itself AFAIK. The loop runs exactly 3 iterations: the first one finds startContainingNode, then the second one finds endContainingNode, then the last iteration checks that both are non-null and returns. Trying to see how to optimize this.

The ChildNodes() method won't include the ancestor itself AFAIK.

Derp, of course 🤦‍♂️

The loop runs exactly 3 iterations

Sounds like more test cases might help there too. If you can confidently say that the first iteration finds the start node, and the second finds the end node, it sounds like a very specific case where both nodes are directly next to each other. What would happen in this situation?

<common> [|<start> </start> <middle> </middle> <end> </end>|] </common>

At least by me typing that out, I now understand more what the logic is trying to do :)

davidwengier · 2024-07-24T23:07:26Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+            """;
+        TestFileMarkupParser.GetPosition(contents, out contents, out var cursorPosition);
+
+        var request = new VSCodeActionParams()


Consider extracting this common code in the tests out to a helper method

davidwengier · 2024-07-24T23:09:08Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+        var request = new VSCodeActionParams()
+        {
+            TextDocument = new VSTextDocumentIdentifier { Uri = new Uri(documentPath) },
+            Range = new Range(),


Given this is passing in an empty range, does that mean this test isn't actually testing the change in this PR?

Would like to see a bunch more tests for the various scenarios that the logic handles. With a helper method it should be hopefully straight foward. The test markup syntax already supports [| and |] as selection markers too.

+1 to this more tests would be good

DustinCampbell · 2024-07-30T15:35:41Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

@@ -9,14 +9,18 @@
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
+using ICSharpCode.Decompiler.CSharp.Syntax;


Why is this needed/desired?

DustinCampbell · 2024-07-30T15:37:38Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

        }
+        return context.Location.AbsoluteIndex > startElementNode.StartTag.Span.End &&


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

DustinCampbell · 2024-07-30T15:38:05Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        }
+
+        var endOwner = syntaxTree.Root.FindInnermostNode(endLocation.Value.AbsoluteIndex, true);
+


tiny nit: Consider removing this blank line, which is consistent with the style you used above.

DustinCampbell · 2024-07-30T15:38:50Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        {
+            return null;
+        }
+        return location;


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

DustinCampbell · 2024-07-30T15:41:10Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+
+    private static SourceLocation? GetEndLocation(Position selectionEnd, RazorCodeDocument codeDocument, ILogger logger)
+    {
+        if (!selectionEnd.TryGetSourceLocation(codeDocument.GetSourceText(), logger, out var location))


Note: GetSourceText() has been removed in main. You can avoid that merge conflict by changing this to codeDocument.Source.Text.

DustinCampbell · 2024-07-30T15:41:39Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        var startSpan = startNode.Span;
+        var endSpan = endNode.Span;
+
+        foreach (var child in nearestCommonAncestor.ChildNodes().Where(node => node.Kind == SyntaxKind.MarkupElement))


Suggested change

foreach (var child in nearestCommonAncestor.ChildNodes().Where(node => node.Kind == SyntaxKind.MarkupElement))

foreach (var child in nearestCommonAncestor.ChildNodes().Where(static node => node.Kind == SyntaxKind.MarkupElement))

DustinCampbell · 2024-07-30T15:44:56Z

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/FileUtilities.cs

-        {
-            throw new ArgumentException("The path is not rooted.", nameof(path));
-        }
+        // Add check for rooted path in the future, currently having issues in platforms other than Windows.


If you filed an issue for this, please link it here.

DustinCampbell · 2024-07-30T15:45:25Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+
+    private static void AddMultiPointSelectionToContext(ref RazorCodeActionContext context, TextSpan selectionSpan)
+    {
+        var sourceText = context.CodeDocument.GetSourceText();


Consider switching this to context.CodeDcoument.Source.Text

DustinCampbell · 2024-07-30T15:47:10Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

@@ -98,4 +248,21 @@ private static RazorCodeActionContext CreateRazorCodeActionContext(VSCodeActionP

        return context;
    }
+
+
+    private static void AddMultiPointSelectionToContext(ref RazorCodeActionContext context, TextSpan selectionSpan)


Why is context passed as ref? Based on the code below, it doesn't need to be.

DustinCampbell · 2024-07-30T15:51:08Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+        var startLinePosition = sourceText.Lines.GetLinePosition(selectionSpan.Start);
+        var startPosition = new Position(startLinePosition.Line, startLinePosition.Character);
+
+        var endLinePosition = sourceText.Lines.GetLinePosition(selectionSpan.End);
+        var endPosition = new Position(endLinePosition.Line, endLinePosition.Character);
+
+        context.Request.Range = new Range
+        {
+            Start = startPosition,
+            End = endPosition
+        };


Note: You don't have the new extension methods or VsLspFactory that merged to main with #10670 (and will be updated eventually with #10682), but you should be able to replace this code with a single helper once you have the latest main.

ryzngard · 2024-08-19T23:30:15Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

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


Should this return false instead of true?

ryzngard · 2024-08-19T23:37:50Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        // If the start element is an ancestor, keep the original end; otherwise, use the end of the end element
+        actionParams.ExtractEnd = selectionStartHasParentElement ? actionParams.ExtractEnd : endElementNode.Span.End;


nit: I just find this easier to read

Suggested change

// If the start element is an ancestor, keep the original end; otherwise, use the end of the end element

actionParams.ExtractEnd = selectionStartHasParentElement ? 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 (!selectionStartHasParentElement)

{

actionParams.ExtractEnd = endElementNode.Span.End;

}

ryzngard · 2024-08-19T23:41:17Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        }
+
+        // Correct selection to include the current node if the selection ends immediately after a closing tag.
+        if (string.IsNullOrWhiteSpace(endOwner.ToFullString()) && endOwner.TryGetPreviousSibling(out var previousSibling))


I think you're detecting cases where MarkupTextLiteral is only whitespace? It's worth doing a syntax type check to reduce creating a new string when not needed

ryzngard · 2024-08-19T23:41:46Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

        }

-        var actionParams = new ExtractToNewComponentCodeActionParams()
+        return endOwner?.FirstAncestorOrSelf<MarkupElementSyntax>();


nit: endOwner should not be null on this line, no need for ?.

ryzngard · 2024-08-19T23:45:02Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs


-        var codeAction = RazorCodeActionFactory.CreateExtractToNewComponent(resolutionParams);
+        // Check if the start element is an ancestor of the end element
+        var selectionStartHasParentElement = endElementNode.Ancestors().Any(node => node == startElementNode);


nit: clarifying the name

Suggested change

var selectionStartHasParentElement = endElementNode.Ancestors().Any(node => node == startElementNode);

var startNodeContainsEndNode = endElementNode.Ancestors().Any(node => node == startElementNode);

ryzngard · 2024-08-19T23:58:05Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+        var request = new VSCodeActionParams()
+        {
+            TextDocument = new VSTextDocumentIdentifier { Uri = new Uri(documentPath) },
+            Range = new Range(),


+1 to this more tests would be good

…erence

ryzngard · 2024-08-21T21:00:10Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        // 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;
+        }


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;

}

Worth adding a test that exercises this

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

ryzngard · 2024-08-21T21:00:45Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        //     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.
+        if (!startNodeContainsEndNode)


If you return early you can remove this check

davidwengier · 2024-08-21T22:02:45Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+
+        var actionParams = CreateInitialActionParams(context, startElementNode, @namespace);
+
+        if (IsMultiPointSelection(context.Request.Range))


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

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

davidwengier · 2024-08-21T22:04:00Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

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


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

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.

davidwengier · 2024-08-21T22:05:32Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

        {
-            return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
+            return null;


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

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 😅

davidwengier · 2024-08-21T22:07:37Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        }
+
+        // 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))


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

davidwengier · 2024-08-21T22:10:45Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

        }

-        if (!TryGetNamespace(context.CodeDocument, out var @namespace))
+        var endLocation = GetEndLocation(selectionEnd, context.CodeDocument);


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

davidwengier · 2024-08-21T22:12:24Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        //     </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.


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>

davidwengier · 2024-08-21T22:21:42Z

...AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToNewComponentCodeActionProvider.cs

+        }
+    }
+
+    private static bool IsMultiPointSelection(Range range) => range.Start != range.End;


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

davidwengier · 2024-08-21T22:23:55Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+        var context = CreateRazorCodeActionContext(request, location, documentPath, contents);
+
+        var lineSpan = context.SourceText.GetLinePositionSpan(selectionSpan);
+        request.Range = VsLspFactory.CreateRange(lineSpan);


Why not just set the initial value of Range to this, above?

lineSpan depends on context -> request so I have to initialize it first

davidwengier · 2024-08-21T22:26:26Z

...e.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToNewComponentCodeActionProviderTest.cs

+        Assert.Equal(selectionSpan.Start, actionParams.ExtractStart);
+        Assert.Equal(selectionSpan.End, actionParams.ExtractEnd);


It seems odd that this is the only test that seems to actually validate anything about the response. Is this what you were talking about with future PRs having better tests?

Yup! I mentioned tests in upcoming PR's here

davidwengier

Sounds lots a lot of comments have been actioned in future PRs, so going to approve so things can move forward

ryzngard

marcarro requested a review from a team as a code owner July 15, 2024 20:14

DustinCampbell reviewed Jul 19, 2024

View reviewed changes

marcarro force-pushed the selectRangeFeature branch from 11ec212 to 5631864 Compare July 24, 2024 17:26

davidwengier reviewed Jul 24, 2024

View reviewed changes

marcarro requested a review from a team as a code owner July 25, 2024 17:11

DustinCampbell reviewed Jul 30, 2024

View reviewed changes

ryzngard reviewed Aug 19, 2024

View reviewed changes

marcarro added 16 commits August 19, 2024 17:03

Arranging selection logic

142c3ed

Basic select range feature: practically not functional

ef995cd

Completed and corrected selected range extraction functionality

a81a177

Tried fixing nullability issues

ea0ac53

Commented out incomplete test. Must be finished

0deb915

Rebased to feature branch and updated according to new API

46932b3

Changed null assert to empty assert according to ci test results

a939694

Refactored element selection logic into methods for easier readability

a814ca8

Removed unnecessary using

8ba59f2

Added check for IsInsideProperHtmlContent

cd6b24a

Removed unnecessary iteration variable in FindContainingSiblignPair

55669a7

nits and test fixes

8ee84de

Functioning tests and fixed selection issue

eba1b01

Added null check

6656f7a

Test updates

31282c8

PR Feedback

04aa349

marcarro force-pushed the selectRangeFeature branch from 5707fd9 to 04aa349 Compare August 20, 2024 00:08

marcarro added 3 commits August 19, 2024 18:00

Adapted to new TryGetSourceLocation and clarified ambiguous Range ref…

8dbd672

…erence

Changed tests to use VsLspFactory utiltiies for multi point selection

8a7d0bb

PR Feedback

7d3c78a

marcarro requested a review from davidwengier August 21, 2024 18:54

marcarro requested review from DustinCampbell and ryzngard August 21, 2024 18:54

ryzngard reviewed Aug 21, 2024

View reviewed changes

Added early return in ProcessMultiPointSelection

842b162

davidwengier reviewed Aug 21, 2024

View reviewed changes

marcarro added 2 commits August 21, 2024 15:57

No need to check for multipoint selection

4bab4dd

PR Feedback

4297e6f

davidwengier approved these changes Aug 21, 2024

View reviewed changes

ryzngard approved these changes Aug 22, 2024

View reviewed changes

phil-allen-msft merged commit f1923ed into dotnet:features/extract-to-component Aug 22, 2024
12 checks passed

		// Holding off on this test until configured correctly (fails on CI)
		//[Theory]

		}
		return context.Location.AbsoluteIndex > startElementNode.StartTag.Span.End &&

		}

		var endOwner = syntaxTree.Root.FindInnermostNode(endLocation.Value.AbsoluteIndex, true);

	foreach (var child in nearestCommonAncestor.ChildNodes().Where(node => node.Kind == SyntaxKind.MarkupElement))
	foreach (var child in nearestCommonAncestor.ChildNodes().Where(static node => node.Kind == SyntaxKind.MarkupElement))

		// If the start element is an ancestor, keep the original end; otherwise, use the end of the end element
		actionParams.ExtractEnd = selectionStartHasParentElement ? actionParams.ExtractEnd : endElementNode.Span.End;

	var selectionStartHasParentElement = endElementNode.Ancestors().Any(node => node == startElementNode);
	var startNodeContainsEndNode = endElementNode.Ancestors().Any(node => node == startElementNode);


		var actionParams = CreateInitialActionParams(context, startElementNode, @namespace);

		if (IsMultiPointSelection(context.Request.Range))

		Assert.Equal(selectionSpan.Start, actionParams.ExtractStart);
		Assert.Equal(selectionSpan.End, actionParams.ExtractEnd);

Select range feature #10621

Select range feature #10621

Conversation

marcarro commented Jul 15, 2024 • edited Loading

Summary of the changes

davidwengier commented Jul 15, 2024

DustinCampbell commented Jul 15, 2024

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcarro Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

ryzngard left a comment

Choose a reason for hiding this comment

marcarro commented Jul 15, 2024 •

edited

Loading

marcarro Jul 25, 2024 •

edited

Loading