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

Support multi-caret when inserting doc comments #54741

Merged
merged 9 commits into from
Feb 7, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 10, 2021

gif

Fixes #54391

@Youssef1313 Youssef1313 requested a review from a team as a code owner July 10, 2021 08:46
@CyrusNajmabadi
Copy link
Member

I'm ok with this (esp. with how minimal the code is). but would def want a test for this :)

@CyrusNajmabadi
Copy link
Member

Is thsi fixing some reported issue?

@Youssef1313
Copy link
Member Author

Is thsi fixing some reported issue?

Yeah I forgot to add it. Will link it from PR description: #54391

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 10, 2021

Test infrastructure doesn't seem to be working with multi-caret (or possibly I can't write a proper test?). I'll look into how to fix that.

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi I was unable to write a test. Can you take a look so that we can get it merged? Thanks!

…bstractDocumentationCommentCommandHandler.cs
@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 17, 2022
@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Can you take a look please?

@sharwell
Copy link
Member

sharwell commented Feb 4, 2022

@Youssef1313 should be possible to write an integration test for this in the new integration test project

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

An integration test should be easy in the new integration test project

@Youssef1313
Copy link
Member Author

@sharwell Which project? Can you point me please? Thanks!

@sharwell
Copy link
Member

sharwell commented Feb 7, 2022

image

@sharwell
Copy link
Member

sharwell commented Feb 7, 2022

The current tests have examples for how to do a bunch of different things. If you don't see a similar example for something you need, feel free to ask.

@Youssef1313
Copy link
Member Author

@sharwell I couldn't find a test that does multi-caret. When I tried to do it I got System.ArgumentException : Saw multiple occurrences of $$

        [IdeFact]
        [WorkItem(54391, "https://github.com/dotnet/roslyn/issues/54391")]
        public async Task TypingCharacter_MultiCaret()
        {
            var code =
@"
//$$
class C1 { }

//$$
class C2 { }

//$$
class C3 { }
";
            await SetUpEditorAsync(code, HangMitigatingCancellationToken);
            await TestServices.Input.SendAsync('/');
            var expected =
@"
/// <summary>
/// $$
/// </summary>
class C1 { }

/// <summary>
///
/// </summary>
class C2 { }

/// <summary>
///
/// </summary>
class C3 { }
";

            await TestServices.EditorVerifier.TextContainsAsync(expected, cancellationToken: HangMitigatingCancellationToken);
        }

@Youssef1313
Copy link
Member Author

Also tried:

//$$
class C1 { }

//[||]
class C2 { }

//[||]
class C3 { }

But fails with System.Runtime.InteropServices.COMException : Error HRESULT E_FAIL has been returned from a call to a COM component.

@davidwengier
Copy link
Contributor

@Youssef1313 I took the liberty of updating the test. The issues were:

  • The test framework didn't support multi-caret, so I added some basic support
  • You didn't have a constructor in your test class, so didn't get a solution and project automatically created, so when calling SetupEditorAsync the COM exception occurred, because there wasn't an editor to setup.
  • You weren't asserting the caret position, so the test was expecting to see $$ in the actual editor
  • You missed a couple of bits of whitespace

All in all though, you did a great job (unsurprisingly 😁)

@Youssef1313
Copy link
Member Author

@davidwengier Thanks a lot!!

@MartyIX
Copy link
Contributor

MartyIX commented Feb 7, 2022

@Youssef1313 Do you have more up-to-date gif? Just curious. ;-)

@Youssef1313
Copy link
Member Author

@MartyIX Oh looks like the software I used produced a bad GIF 😄

Will build the branch and update it soon.

@sharwell sharwell merged commit efd773e into dotnet:main Feb 7, 2022
@ghost ghost added this to the Next milestone Feb 7, 2022
@Youssef1313 Youssef1313 deleted the multi-caret-doc-comments branch February 8, 2022 02:45
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete of summary fails when cursor is placed in multiple spots.
7 participants