Generate Documentation - Add loading state #77718
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
| var intellicodeLineCompletionsDisposable = await _suggestionManager!.DisableProviderAsync(SuggestionServiceNames.IntelliCodeLineCompletions, cancellationToken).ConfigureAwait(false); | ||
| var intelliCodeLineCompletionsDisposable = await _suggestionManager!.DisableProviderAsync(SuggestionServiceNames.IntelliCodeLineCompletions, cancellationToken).ConfigureAwait(false); | ||
| var suggestion = new DocumentationCommentSuggestion(this, _suggestionManager, intelliCodeLineCompletionsDisposable); | ||
| var suggestionSessionStarted = await suggestion.StartSuggestionSessionAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
nit: i feel this unnecessarily exposed the internal of DocumentationCommentSuggestion, (like you need to make sure the order of calls, etc.) consider passing in a lambda to calculate the proposal to StartSuggestionSessionAsync and let suggestion handles display/dismiss in it.
There was a problem hiding this comment.
thoughts on this comment Ankita?
src/EditorFeatures/Core/DocumentationComments/CopilotGenerateDocumentationCommentProvider.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
| } | ||
| }); | ||
|
|
||
| if (!taskCompletionSource.Task.IsCompleted) |
There was a problem hiding this comment.
this seems superfluous. just await the task. if it's complete, then it will no op anyways.
There was a problem hiding this comment.
commented in the code, but added that the pattern is consistent with what the platform expects.
There was a problem hiding this comment.
The idea was to avoid the cost of switching to the background thread if the task wasn't complete.
src/EditorFeatures/Core/DocumentationComments/CopilotGenerateDocumentationCommentProvider.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
| "DismissSuggestionSession", | ||
| async () => await _suggestionSession!.DismissAsync(ReasonForDismiss.DismissedDueToInvalidProposal, cancellationToken).ConfigureAwait(false), | ||
| cancellationToken).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
why is thre DIsmiss and Dispose? WHy not just one concept?
There was a problem hiding this comment.
Dismiss is on the SuggestionSession (platform owned), which is what we start to give us exclusive rights to display grey text later. Dispose is on us to allow line completions to start being displayed again since we took away their right to when we started our SuggestionSession.
There was a problem hiding this comment.
so, generally speaking, cmments like this in the PR are worth putting into the code, because the code isn't clear as to why that's going on.
| var taskCompletionSource = new TaskCompletionSource<bool>(); | ||
|
|
||
| await providerInstance.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancel); | ||
| SuggestionManager.EnqueueAction(description, async () => |
There was a problem hiding this comment.
this is super weird. they expect you to pass in an async function. but then they dont' make this function itself task-returning so you can track it?
|
done with pass. |
src/EditorFeatures/Core/DocumentationComments/DocumentationCommentSuggestion.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var taskCompletionSource = new TaskCompletionSource<bool>(); | ||
|
|
||
| await providerInstance.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancel); |
There was a problem hiding this comment.
why are we switching to the UI thread? if this is all async, why do they not do this?
| { | ||
| Logger.Log(FunctionId.Copilot_Generate_Documentation_Displayed, logLevel: LogLevel.Information); | ||
| } | ||
| await DisposeAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
i def feel like this shouldbe commented to explain what's going on. ou could even consider having a helper method with a clearer name that hten internallyc alls into DisposeAsync.
| /// </summary> | ||
| public async Task DismissSuggestionSessionAsync(CancellationToken cancellationToken) | ||
| { | ||
| await DisposeAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
same point about commenting this.
| @@ -46,37 +48,37 @@ public async Task InitializeAsync(ITextView textView, SuggestionServiceBase sugg | |||
| public async Task GenerateDocumentationProposalAsync(DocumentationCommentSnippet snippet, | |||
| ITextSnapshot oldSnapshot, VirtualSnapshotPoint oldCaret, CancellationToken cancellationToken) | |||
| { | |||
| await Task.Yield().ConfigureAwait(false); | |||
| await TaskScheduler.Default; | |||
There was a problem hiding this comment.
Maybe wait until after the !Enabled check before yielding the thread to your caller, in case you don't need to yield at all.
|
|
||
| var taskCompletionSource = new TaskCompletionSource<T>(); | ||
|
|
||
| await providerInstance.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); |
There was a problem hiding this comment.
Does SuggestionManager.EnqueueAction require enqueuing to take place on the main thread? As a scheduler, that's a weird requirement. And if it isn't a requirement, this seems wasteful to do. If you can remove the switch, you can make this whole method non-async, which improves efficiency.
There was a problem hiding this comment.
I believe so based on the pattern in the platform, but @dpugh would know more about this particular requirement.
| { | ||
| Assumes.NotNull(SuggestionManager); | ||
|
|
||
| var taskCompletionSource = new TaskCompletionSource<T>(); |
There was a problem hiding this comment.
Given the (slightly odd) design that this queue doesn't provide a way to track work, and you're hacking one in, you're potentially opening it up to a problem. When action() completes, continuations that your returned Task adds on (by awaiting the result of this method) may run inline with the action itself, holding up the queue even longer before it can work on the next thing.
You may want to create this TCS with TaskCreationOptions.RunContinuationsAsynchronously to prevent accidentally inlining arbitrary work into the queue.
| { | ||
| try | ||
| { | ||
| var result = await action().ConfigureAwait(false); |
There was a problem hiding this comment.
Since all you do here is mark a TCS complete, you can potentially improve efficiency by using ConfigureAwaitRunInline here instead.
Just be sure to combine it with my other suggestion to prevent other continuations from being inlined.
Loading_State_Generate_Docs.mp4