Determine and send inline completion session result #323
Conversation
| if (input.isEmpty()) { | ||
| Activator.getLogger().info("distance traversed is 1:" + distanceTraversed); |
There was a problem hiding this comment.
Not sure if we want to keep this in. This would get logged every time backspace is hit during a typeahead.
Same with the logging call made below.
There was a problem hiding this comment.
updated forgot to remove it
| session.updateCompletionStates(List.of(session.getCurrentSuggestion().getItemId())); | ||
| session.markSuggestionAsSeen(); |
There was a problem hiding this comment.
When these two lines get run, it means that the user has typed ahead with a character that matches.
This means updateCompletionStates and markSuggestionAsSeen are going to be called for every correct character typed in a typeahead session, with the same suggestion.
Should we perhaps wrap them in a check to see if the user has just started the typeahead?
if (!session.hasBeenTypedahead()) {
session.updateCompletionStates(List.of(session.getCurrentSuggestion().getItemId()));
session.markSuggestionAsSeen();
}There was a problem hiding this comment.
I think this line would do the opposite of what we want to do here which is to mark when suggestions have been filtered because typeahead matches.
The above logic would do the opposite because it says if there is no typeahead then consider suggsestions as filtered and discard other suggestions except for current one as being filtered out.
Also wouldn't this always return true if we hit this line of code in this block(L349) session.hasBeenTypedahead() since this would only process when there is A typeahead?
are going to be called for every correct character typed in a typeahead session, with the same suggestion.
That is correct. Since this documentChange event is raised for any update, based on user's actions it could have been a backspace marking things not-discarded unseen in the previous stroke or any of the other checks before this line. Ideally the whole list of suggestions should be filtered down to match the typeahead prefix. However due to limitations in the current filtering logic we only show the current suggestion instead of other valid suggestions from the list. I think it still makes sense to have this because unless we can retain the information of the typedPrefix among other stuff this class does it is hard to know if typeahead actually diverged or not.
| }).collect(Collectors.toList())).get(); | ||
| unresolvedTasks.remove(uuid); | ||
| List<InlineCompletionItem> newSuggestions = new ArrayList<InlineCompletionItem>(); | ||
| List<String> sessionId = new ArrayList<String>(); |
There was a problem hiding this comment.
Why does this need to be a list? (It looks like we are just using the first item).
There was a problem hiding this comment.
I get an error if I try to define a local string variable and try to set it inside a lambda, In Java, variables used inside lambda expressions must be either final or effectively final (meaning they are not modified after initialization) so it wouldn't let me modify that. Lists are considered to be final during initialization so this approach works.
| if (newSuggestions == null || newSuggestions.isEmpty()) { | ||
| unresolvedTasks.remove(uuid); | ||
|
|
||
| if (newSuggestions == null || newSuggestions.isEmpty() || sessionId.get(0) == null || sessionId.get(0).isEmpty()) { |
There was a problem hiding this comment.
I have never actually paid attention to this session id field in a query result. Curious, does a session in FLARE have the same life time as a session we have defined here (QInvocationSession)? Or is it synonymous to a query?
There was a problem hiding this comment.
Yes this sessionId is set by Flare on the lsp side when it sends responses. This is assigned for session management on lsp side and also used for emitting right metrics for each session where there can be multiple in flight.
the same life time as a session we have defined here (QInvocationSession)
I have tried to do that but considering logic in this class is very coupled to unresolved Tasks/isActive it might not be very 1: 1 since we drop certain in flight queries based on empty suggestions etc.
| suggestionsContext.getDetails() | ||
| .addAll(newSuggestions.stream().map(QSuggestionContext::new).collect(Collectors.toList())); |
There was a problem hiding this comment.
Not that I have a problem with this but looking at this did make me wonder why it was called later before. Perhaps it was because if we do end up invoking an early termination we don't waste time doing this? But if it is an early termination this call would process 0 elements anyway. So I am okay with this.
There was a problem hiding this comment.
Yes I believe so. The reason I moved it is because I needed to send completion states for non-zero suggestions received but filtered out client side due to typeahead or backspace on L226 and L258. I think earlier there was no reason to track them untill these checks passed and hit a scenario where they could actually be displayed.
Description of changes:
This change adds support to send InlineCompletionSession result to lsp. This
logInlineCompletionSessionResultsnotification is used to communicate information about user interactions in UI with list of suggestions received for telemetry instrumentation.Modifications have been made to the QInvocationSession class which is responsible for managing querying the LSP and showing suggestions in the UI. It maintains a map of each suggestion to it's completion state and populates them as user sees/accepts/filters/rejects a suggestion. Final inline completion result is sent when a session is accepted or dismissed.
Testing
Verified states of accept, reject, discard and ignore were being emitted with "userDecision" and "userTriggerDecision" metrics.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.