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

Implementation of new editor completion API #29016

Merged
merged 39 commits into from Nov 27, 2018

Conversation

@ivanbasov
Copy link
Contributor

commented Aug 1, 2018

No description provided.

@ivanbasov ivanbasov requested review from dotnet/roslyn-ide as code owners Aug 1, 2018

@ivanbasov ivanbasov removed the request for review from dotnet/roslyn-interactive Aug 1, 2018

@ivanbasov ivanbasov added this to Awaiting Triage in Editor Completion via automation Aug 1, 2018

@ivanbasov ivanbasov added this to the 16.0 milestone Aug 1, 2018

@ivanbasov ivanbasov added the Area-IDE label Aug 1, 2018

@ivanbasov ivanbasov moved this from Awaiting Triage to Working in Editor Completion Aug 1, 2018

@tmat

This comment has been minimized.

Copy link
Member

commented Aug 4, 2018

What is the relationship of this PR to #25770? What's new here? #Resolved

@sharwell
Copy link
Member

left a comment

Marking as request changes for the sole purpose of making sure I don't forget to review this one.

@ivanbasov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@tmat , this is a merge of #25770 with recent biths of master-vs-deps. I'll close #25770 as soon as I'll merge this one and migrate another PR depending on #25770 to new master-bs-deps bits.

@sharwell , an interesting approach to make notes :-) So, could you please review?

@jasonmalinowski , @jinujoseph, @dotnet/roslyn-ide , pinging ... do we have reasons to keep this work pending for a long time once again? #Resolved

@jasonmalinowski
Copy link
Member

left a comment

Comments so far. I still need to review EditorAsyncCompletionItemManager.cs.

The custom commit code to me is particularly worrisome -- at first glance it has a number of bugs around handling of snapshots. If it's right I'd appreciate a lot more comments as to why it's right.

For the tests, I think we should figure out a better way to share the test assets rather than a copy/paste. xunit allows (among other things) you to use inherited test classes to try running the same tests with some different context. It might require a bit of adding a layer of indirection, but I think it'll make it a lot easier to understand this PR and maintain this code even in the short term. I'm guessing we won't be deprecating the old one for a few previews, and in that time we might have other work to be doing for language support.

Show resolved Hide resolved src/EditorFeatures/Core/CommandHandlers/SignatureHelpCommandHandler.cs Outdated
Show resolved Hide resolved ...es/Core/Implementation/IntelliSense/Completion/AsyncCompletionService.cs Outdated
Show resolved Hide resolved ...EditorFeatures/Core/Implementation/IntelliSense/Completion/Controller.cs Outdated
Show resolved Hide resolved ...iSense/EditorCompletionImplementation/CompletionCommitManagerProvider.cs Outdated
Show resolved Hide resolved ...iSense/EditorCompletionImplementation/CompletionCommitManagerProvider.cs Outdated

public async Task<object> GetDescriptionAsync(EditorCompletion.CompletionItem item, CancellationToken cancellationToken)
{
// TODO: Should string.Empty returns here be null to avoid an empty popup? https://github.com/dotnet/roslyn/issues/27420

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

Makes sense to me. Why the bug? #Resolved

return string.Empty;
}

var documentId = workspace.GetDocumentIdInCurrentContext(triggerBuffer.AsTextContainer());

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

Isn't this all just triggerBuffer.CurrentSnapshot.GetDocumentInCurrentContext()? #WontFix

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Sep 25, 2018

Author Contributor

Could not find GetDocumentInCurrentContext in CurrentSnapshot


In reply to: 208429747 [](ancestors = 208429747)

return string.Empty;
}

var service = (CompletionServiceWithProviders)document.GetLanguageService<CompletionService>();

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

Same as next function below? Can we reuse? #Resolved

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Aug 8, 2018

Author Contributor

The method just below is unused. Removing it. :-)


In reply to: 208429831 [](ancestors = 208429831)

return (CompletionServiceWithProviders)document.GetLanguageService<CompletionService>();
}

public Task HandleViewClosedAsync(ITextView view) => Task.CompletedTask;

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

What should we be doing here? Anything? #Resolved


private RoslynCompletionItem GetOrCreateRoslynItem(EditorCompletion.CompletionItem item)
{
if (item.Properties.TryGetProperty<RoslynCompletionItem>("RoslynItem", out var roslynItem))

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

Constant since I'm guessing this is hardcoded in other places? #Resolved

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Aug 8, 2018

Author Contributor

the only other instance is in test state. so, it think constant is not necessary.


In reply to: 208430212 [](ancestors = 208430212)

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Sep 28, 2018

Member

@ivanbasov Still having a constant would mean that's possible to determine. #Resolved

@sharwell sharwell force-pushed the ivanbasov:asynccompletion branch from d25db1a to 8b0988d Aug 8, 2018

@sharwell
Copy link
Member

left a comment

This pull request should not be merged until we fix the rename tracking for items in the OldCompletion folder. I implemented the change locally and am working with @ivanbasov to correct the pull request.

@sharwell sharwell referenced this pull request Aug 8, 2018

Closed

Fork completion tests #29166

@sharwell

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

@ivanbasov I figured out the file copy tracking, but it's not the most pleasant thing ever. This pull request is blocked on #29166; after that is merged I'll update this pull request and it will be much better. #Resolved

{
if (!IsAfterDot(data.Snapshot, session.ApplicableToSpan))
{
return Task.FromResult(new EditorCompletion.FilteredCompletionModel(ImmutableArray<EditorCompletion.CompletionItemWithHighlight>.Empty, 0));

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

Should the editor provide a FilteredCompletionModel.Empty? #Pending

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Sep 25, 2018

Author Contributor

@amwieczo@microsoft.com please take a look


In reply to: 208708360 [](ancestors = 208708360)

}

// We need to filter if a non-empty strict subset of filters are selected
var selectedFilters = data.SelectedFilters.Where(f => f.IsSelected).Select(f => f.Filter).ToImmutableArray();

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

.SelectedFilters isn't the filters that are selected? @AmadeusW? #Resolved

This comment has been minimized.

Copy link
@AmadeusW

AmadeusW Aug 13, 2018

Contributor

This is complete set of filters, specifically CompletionFilterWithStates. I see how "SelectedFilters" is a poor name for "all filters"

Each element of this enumeration has a reference to the CompletionFilter and two bools: IsAvailable and IsSelected

Language service is expected to

  1. Filter the completion items based on typed text,
    which determines the IsAvailable property (true if at least one completion item of that filter is present)
  2. Filter the results based on filter's IsSelected property (value is set by user) #Resolved

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Sep 28, 2018

Member

So are we renaming this? 😄 #Resolved

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Sep 29, 2018

Author Contributor

Yes, copying the original response by @amwieczo@microsoft.com

This is complete set of filters, specifically CompletionFilterWithStates. I see how "SelectedFilters" is a poor name for "all filters"

Each element of this enumeration has a reference to the CompletionFilter and two bools: IsAvailable and IsSelected

Language service is expected to

  1. Filter the completion items based on typed text,
    which determines the IsAvailable property (true if at least one completion item of that filter is present)
  2. Filter the results based on filter's IsSelected property (value is set by user)

In reply to: 208708695 [](ancestors = 208708695)

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Sep 29, 2018

Author Contributor

yes, here are our selectedFilters. And there are Editor's selected filters which are all filters. ugh


In reply to: 221393614 [](ancestors = 221393614)

}
else
{
if (data.InitialTrigger.Reason == EditorCompletion.InitialTriggerReason.Deletion ||

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 8, 2018

Member

Do this check first before MatchesFilterText since it's a lot cheaper? #ByDesign

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Sep 26, 2018

Author Contributor

It seems that MatchFilterText should have another priority over the reasons comparison: if there is a Match, we set matchedFilterText: true even if it is the deletion. So, we cannot swap condition checks.


In reply to: 208709031 [](ancestors = 208709031)

@ivanbasov ivanbasov force-pushed the ivanbasov:asynccompletion branch from 29d5f57 to 54c1e92 Aug 9, 2018

@ivanbasov ivanbasov requested a review from dotnet/roslyn-infrastructure as a code owner Aug 10, 2018

@ivanbasov ivanbasov force-pushed the ivanbasov:asynccompletion branch from e78363e to e57a325 Aug 10, 2018

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

Can there be a general 'ping' when this is ready for reviewing? #Resolved

@ivanbasov ivanbasov force-pushed the ivanbasov:asynccompletion branch from 6c94b68 to 85273b2 Nov 26, 2018

ivanbasov added some commits Nov 27, 2018

@jasonmalinowski
Copy link
Member

left a comment

Looks good enough for an initial merge, with the understanding we still have a lot to clean up. A few last questions, but I don't think that'll result in any changes needed at this point.

I am still very concerned about the editor API's design around us having to be responsible for filtering and sorting other items that aren't our own in the projection case. It seems like that's still going to require some redesigns, but that's not really a problem with this PR per se but rather something they'll need to address.

if (change.NewPosition.HasValue)
{
view.TryMoveCaretToAndEnsureVisible(new SnapshotPoint(updatedCurrentSnapshot, change.NewPosition.Value));
view.TryMoveCaretToAndEnsureVisible(new SnapshotPoint(subjectBuffer.CurrentSnapshot, change.NewPosition.Value));

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Nov 27, 2018

Member

Shouldn't this also be making a snapshot point against the appropriate snapshot and translating it forward? i.e. the updatedCurrentSnapshot was correct here but not further below? (This also presumes TryMoveCaretAndEnsureVisible translates forward instead of crashing.) #Resolved

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Nov 27, 2018

Author Contributor

Let us consider this as a separate issue: #31383


In reply to: 236495952 [](ancestors = 236495952)

{
// Need the trigger snapshot to calculate the span when the commit changes to be applied.
// It should be inserted into a property bag within GetCompletionContextAsync for each item created by Roslyn.
// If not found here, Roslyn should not make a commit.

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Nov 27, 2018

Member

How would we get here in the first place? You already filtered non-Roslyn items earlier? #Resolved

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Nov 27, 2018

Author Contributor
  1. We can miss to provide triggerSnapshot by mistake.

  2. Let us assume there is another provider wants Roslyn to perform a commit and provided CompletionSource.RoslynItem above. We can proceed but we also need triggerSnapshot in the case.


In reply to: 236497307 [](ancestors = 236497307)

return CommitResultUnhandled;
}

// Commit with completion serivce assumes that null is provided is case of invoke. VS provides '\0' in the case.

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Nov 27, 2018

Member
Suggested change
// Commit with completion serivce assumes that null is provided is case of invoke. VS provides '\0' in the case.
// Commit with completion service assumes that null is provided is case of invoke. VS provides '\0' in the case.
``` #Resolved
if (textTypedSoFar.Length > 0)
{
return item.DisplayText.StartsWith(textTypedSoFar, StringComparison.CurrentCultureIgnoreCase) ||
item.FilterText.StartsWith(textTypedSoFar, StringComparison.CurrentCultureIgnoreCase);

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Nov 27, 2018

Member

Didn't we also do filtering explicitly in en-us for people using non-English things trying to type English framework names? #Resolved

This comment has been minimized.

Copy link
@ivanbasov

ivanbasov Nov 27, 2018

Author Contributor

#31384


In reply to: 236497871 [](ancestors = 236497871)

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

I am still very concerned about the editor API's design around us having to be responsible for filtering and sorting other items that aren't our own in the projection case. It seems like that's still going to require some redesigns, but that's not really a problem with this PR per se but rather something they'll need to address.

Agreed. In the meantime though, would it be better for us to not filter/sort these items?

I ask because otherwise we know how this will likely pan out. We'll have the code for it, dependencies will be taken on it, and it will taken 15+ years to finally get removed. :)

Basically, if this is merged in, i put odds at 99% that it never ever gets removed. But if we don't add it, it will force the issue of implementing things in a proper fashion. Thoughts @jasonmalinowski

@ivanbasov ivanbasov merged commit af42e74 into dotnet:master-vs-deps Nov 27, 2018

3 of 5 checks passed

windows_debug_vs-integration_prtest Build finished.
Details
windows_release_vs-integration_prtest Build finished.
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI #20181126.34 succeeded
Details

Editor Completion automation moved this from Working to Completed Nov 27, 2018

@ivanbasov ivanbasov deleted the ivanbasov:asynccompletion branch Nov 27, 2018

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.