Skip to content

Commit

Permalink
feat(completion): Fix handling of multiple providers (onivim#2202)
Browse files Browse the repository at this point in the history
There were several issues with completion, in terms of handling multiple completion providers:
- We'd make multiple requests, in some cases, showing unnecessary duplicates of items
- Unregistration was not handled in the previous strategy
- We'd wait for all completion requests to go through before showing completion UX
- We did not use the `sortText` or `filterText` properties
- When committing a completion, the completion UX might pop back up (ie, like onivim#1171)

This improves the state management of the completion experience by moving it to the `Feature_LanguageSupport` module, handling dynamic registration / unregistration,  keeping track of 'committed' completions (so we don't re-trigger until a new completion meet is encountered).

Fixes onivim#1171 onivim#1133 onivim#1172

__TODO:__
- [x] Fix syntax scope handling (restore the previous functionality)
- [x] Restore the `Control+N`/`Control+P` behavior (along with arrow keys) for cycling completions
- [x] Add trigger character support
- [x] Does this impact onivim#1133 ?
- [x] Add trigger command (onivim#1172) 
- [x] Handle `insert` / `normal` transition
- [x] Test the `isActive` / context key to ensure it's being triggered
- [x] Handle cursor movement (onivim#2167)
- [x] Investigate proper `insertText` handling (deserialize range, kind, additional edits, command):
  - C++ - extra spaces introduced
  - elixir - `/4` is added
  - Java - `System.out.println() : void`
- [x] Handle snippets in a simple way (zero out placeholders, move cursor to first placeholder)
- [x] Hook up completion details subscription / resolve
  • Loading branch information
bryphe authored Aug 2, 2020
1 parent db64211 commit f482df2
Show file tree
Hide file tree
Showing 55 changed files with 2,136 additions and 1,134 deletions.
1 change: 0 additions & 1 deletion bench/lib/EditorSurfaceBench.re
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ let editor = (editor, buffer, state: State.t) => {
bufferHighlights={state.bufferHighlights}
bufferSyntaxHighlights={state.syntaxHighlights}
diagnostics={state.diagnostics}
completions={state.completions}
tokenTheme={state.tokenTheme}
languageSupport={state.languageSupport}
mode={Feature_Vim.mode(state.vim)}
Expand Down
5 changes: 5 additions & 0 deletions development_extensions/oni-dev-extension/extension.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ function activate(context) {
provideCompletionItems: (document, position, token, context) => {
return [vscode.CompletionItem("HelloWorld"), vscode.CompletionItem("HelloAgain")]
},
resolveCompletionItem: (completionItem, token) => {
completionItem.documentation = "RESOLVED documentation: "+ completionItem.label;
completionItem.detail = "RESOLVED detail: " + completionItem.label;
return completionItem;
}
}),
)

Expand Down
3 changes: 2 additions & 1 deletion integration_test/ExtHostCompletionTest.re
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ runTestWithInput(
~timeout=30.0,
~name="Validate we get some completions from the 'oni-dev' extension",
(state: State.t) =>
Array.length(state.completions.filtered) > 0
state.languageSupport
|> Feature_LanguageSupport.Completion.availableCompletionCount > 0
);
});
3 changes: 2 additions & 1 deletion integration_test/LanguageCssTest.re
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ runTestWithInput(
~timeout=30.0,
~name="Validate we also got some completions",
(state: State.t) =>
Array.length(state.completions.filtered) > 0
state.languageSupport
|> Feature_LanguageSupport.Completion.availableCompletionCount > 0
);

// Finish input, clear diagnostics
Expand Down
3 changes: 2 additions & 1 deletion integration_test/LanguageTypeScriptTest.re
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ runTestWithInput(
~timeout=30.0,
~name="Validate we also got some completions",
(state: State.t) =>
Array.length(state.completions.filtered) > 0
state.languageSupport
|> Feature_LanguageSupport.Completion.availableCompletionCount > 0
);

// Fix error, finish identifier
Expand Down
20 changes: 8 additions & 12 deletions integration_test/lib/ExtensionHelpers.re
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,11 @@ let waitForNewCompletionProviders =
waitForState(
~name=
"Getting count of current suggest providers: " ++ originalDescription,
(State.{languageFeatures, _}) => {
let current = LanguageFeatures.getCompletionProviders(languageFeatures);
(State.{languageSupport, _}) => {
let current =
Feature_LanguageSupport.Completion.providerCount(languageSupport);

Log.info("Current suggest providers: ");
List.iter(id => Log.info("-- " ++ id), current);

existingCompletionCount := List.length(current);
existingCompletionCount := current;
true;
},
);
Expand All @@ -55,13 +53,11 @@ let waitForNewCompletionProviders =
waitForState(
~timeout=10.0,
~name="Waiting for new suggest providers: " ++ originalDescription,
(State.{languageFeatures, _}) => {
let current = LanguageFeatures.getCompletionProviders(languageFeatures);

Log.info("Current suggest providers: ");
List.iter(id => Log.info("-- " ++ id), current);
(State.{languageSupport, _}) => {
let current =
Feature_LanguageSupport.Completion.providerCount(languageSupport);

List.length(current) > existingCompletionCount^;
current > existingCompletionCount^;
},
);
};
36 changes: 0 additions & 36 deletions src/Core/ConfigurationParser.re
Original file line number Diff line number Diff line change
Expand Up @@ -157,35 +157,6 @@ let parseAutoClosingBrackets:
| _ => Never
};

let parseQuickSuggestions: Yojson.Safe.t => quickSuggestionsEnabled = {
let decode =
Json.Decode.(
field("other", bool)
>>= (
other =>
field("comments", bool)
>>= (
comments =>
field("strings", bool)
>>= (strings => succeed({other, comments, strings}))
)
)
)
|> Json.Decode.decode_value;
json =>
switch (json) {
| `Bool(enabled) => {other: enabled, comments: enabled, strings: enabled}
// TODO: Parse JS objects of the form:
// { "other": bool, "comments": bool, "strings": bool }
| _ =>
json
|> decode
|> Result.value(
~default={other: false, comments: false, strings: false},
)
};
};

let parseString = (~default="", json) =>
switch (json) {
| `String(v) => v
Expand Down Expand Up @@ -341,13 +312,6 @@ let configurationParsers: list(configurationTuple) = [
editorHighlightActiveIndentGuide: parseBool(json),
},
),
(
"editor.quickSuggestions",
(config, json) => {
...config,
editorQuickSuggestions: parseQuickSuggestions(json),
},
),
(
"editor.renderIndentGuides",
(config, json) => {
Expand Down
12 changes: 0 additions & 12 deletions src/Core/ConfigurationValues.re
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,6 @@ type fontSmoothing =
| Antialiased
| SubpixelAntialiased;

type quickSuggestionsEnabled = {
other: bool,
comments: bool,
strings: bool,
};

type fontLigatures = [ | `Bool(bool) | `List(list(string))];

type t = {
Expand All @@ -55,7 +49,6 @@ type t = {
editorMinimapMaxColumn: int,
editorInsertSpaces: bool,
editorIndentSize: int,
editorQuickSuggestions: quickSuggestionsEnabled,
editorTabSize: int,
editorHighlightActiveIndentGuide: bool,
editorRenderIndentGuides: bool,
Expand Down Expand Up @@ -108,11 +101,6 @@ let default = {
editorTabSize: 4,
editorRenderIndentGuides: true,
editorHighlightActiveIndentGuide: true,
editorQuickSuggestions: {
other: true,
comments: true,
strings: true,
},
editorRenderWhitespace: None,
editorRulers: [],
terminalIntegratedFontFile: Constants.defaultFontFile,
Expand Down
5 changes: 5 additions & 0 deletions src/Core/Filter.re
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ type result('a) = {
highlight: list((int, int)),
};

let map = (f, result) => {
item: f(result.item),
highlight: result.highlight,
};

let makeResult = ((maybeMatch, item)) =>
switch (maybeMatch) {
| Some(match) => {
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Filter.rei
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ type result('a) = {
highlight: list((int, int)),
};

let map: ('a => 'b, result('a)) => result('b);

let rank:
(string, ('a, ~shouldLower: bool) => string, list('a)) => list(result('a));

Expand Down
23 changes: 23 additions & 0 deletions src/Exthost/ChainedCacheId.re
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
open Oni_Core;

[@deriving show]
type t = (int, int);

let decode = {
Json.Decode.(
list(int)
|> and_then(
fun
| [cacheId0, cacheId1] => succeed((cacheId0, cacheId1))
| _ => fail("Expected (int, int)"),
)
);
};

let encode = {
Json.Encode.(
((cacheId0, cacheId1)) => {
[cacheId0, cacheId1] |> list(int);
}
);
};
32 changes: 32 additions & 0 deletions src/Exthost/CompletionKind.re
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
[@deriving show]
type t =
| Method
| Function
Expand Down Expand Up @@ -59,3 +60,34 @@ let ofInt =
| 26 => Some(Issue)
| 27 => Some(Snippet)
| _ => None;

let toInt =
fun
| Method => 0
| Function => 1
| Constructor => 2
| Field => 3
| Variable => 4
| Class => 5
| Struct => 6
| Interface => 7
| Module => 8
| Property => 9
| Event => 10
| Operator => 11
| Unit => 12
| Value => 13
| Constant => 14
| Enum => 15
| EnumMember => 16
| Keyword => 17
| Text => 18
| Color => 19
| File => 20
| Reference => 21
| Customcolor => 22
| Folder => 23
| TypeParameter => 24
| User => 25
| Issue => 26
| Snippet => 27;
1 change: 1 addition & 0 deletions src/Exthost/Edit.re
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
open Oni_Core;

module SingleEditOperation = {
[@deriving show]
type t = {
range: OneBasedRange.t,
text: option(string),
Expand Down
1 change: 1 addition & 0 deletions src/Exthost/Exthost.re
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module Extension = Exthost_Extension;
module Protocol = Exthost_Protocol;
module Transport = Exthost_Transport;

module ChainedCacheId = ChainedCacheId;
module CodeLens = CodeLens;
module Color = Color;
module Command = Command;
Expand Down
54 changes: 50 additions & 4 deletions src/Exthost/Exthost.rei
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ module Extension = Exthost_Extension;
module Protocol = Exthost_Protocol;
module Transport = Exthost_Transport;

module ChainedCacheId: {
[@deriving show]
type t;
};

module Label: {
[@deriving show]
type segment =
Expand Down Expand Up @@ -43,6 +48,7 @@ module CompletionContext: {
};

module CompletionKind: {
[@deriving show]
type t =
| Method
| Function
Expand Down Expand Up @@ -122,6 +128,7 @@ module MarkdownString: {

module Edit: {
module SingleEditOperation: {
[@deriving show]
type t = {
range: OneBasedRange.t,
text: option(string),
Expand Down Expand Up @@ -252,17 +259,47 @@ module RenameLocation: {
};

module SuggestItem: {
module InsertTextRules: {
[@deriving show]
type rule =
| KeepWhitespace // 0b001
| InsertAsSnippet; // 0b100

[@deriving show]
type t;

let matches: (~rule: rule, t) => bool;
};
module SuggestRange: {
[@deriving show]
type t =
| Single(OneBasedRange.t)
| Combo({
insert: OneBasedRange.t,
replace: OneBasedRange.t,
});
};

[@deriving show]
type t = {
chainedCacheId: option(ChainedCacheId.t),
label: string,
kind: CompletionKind.t,
detail: option(string),
documentation: option(string),
documentation: option(MarkdownString.t),
sortText: option(string),
filterText: option(string),
insertText: option(string),
insertTextRules: InsertTextRules.t,
suggestRange: option(SuggestRange.t),
commitCharacters: list(string),
additionalTextEdits: list(Edit.SingleEditOperation.t),
command: option(Command.t),
};

let decode: Json.decoder(t);
let insertText: t => string;
let filterText: t => string;
let sortText: t => string;
};

module ReferenceContext: {
Expand Down Expand Up @@ -555,14 +592,13 @@ module SignatureHelp: {
};

module SuggestResult: {
[@deriving show]
type t = {
completions: list(SuggestItem.t),
isIncomplete: bool,
};

let empty: t;

let decode: Json.decoder(t);
};

module SymbolKind: {
Expand Down Expand Up @@ -1668,6 +1704,16 @@ module Request: {
) =>
Lwt.t(SuggestResult.t);

let resolveCompletionItem:
(
~handle: int,
~resource: Uri.t,
~position: OneBasedPosition.t,
~chainedCacheId: ChainedCacheId.t,
Client.t
) =>
Lwt.t(SuggestItem.t);

let provideDocumentHighlights:
(
~handle: int,
Expand Down
Loading

0 comments on commit f482df2

Please sign in to comment.