Remove 'help-circle.svg' (question mark) icon from unknown autocompletions

[smolck]

This is an idea of how to remove the 'help-circle.svg' icon from autocompletions that are unknown to clean up the look for completions with unsupported or unimplemented kinds (like those of LanguageClient-Neovim):

Any feedback would be much appreciated! Currently this works by filling in an empty image in the popupmenu if there is an error with the icon's gdk_pixbuf (which is created from returning the "None" string in the get_icon_name_for_kind function).

See #47.

[badosu]

I think we should hide icons only when all are unknown, otherwise it will introduce visual inconsistency when a LSP plugin is supported but kinds are not matched due to a minor issue (customization, outdated handling on gnvim) or simply absent for that particular item.

[smolck]

Good point! So we find a different icon for text completions, and then if all of the completions are unknown, we show no icons. I actually got this functionality to work in a local clone of mine. It uses a boolean parameter in the create, get_icon_name_for_kind and get_icon_pixbuf functions in completion_item_widget.rs to tell get_icon_name_for_kind if other popupmenu items have images to determine if it should show images for unknown completions.

[smolck]

Now the pmenu looks like this if any of the completions are known (note the bottom unknown one):

And this if none are known:

However, I used the help-circle icon for a placeholder. Any ideas for a different icon for text/unknown completions? (Or should we keep the help-circle?)

[vhakulinen]

Showing the help-circle when there are some known kinds is the way to go, so we avoid visual inconsistencies (just like @badosu noted). On a case when there are no icons shown (e.g. all kinds are unknown), remember to add some padding to the left side of the actual text. I would also avoid differing the code paths too much.

Regarding the issue that you are still facing: how to detect when a kind is known or not. I would try this: In nvim_bridge, make the completion item's kind be type of CompletionItemKind which would be an enum (that would look something like this):

pub enum CompletionItemKind {
    Function,
    Constructor,
    File,
    Folder,
    //...
    Unknown,
}

impl CompletionItemKind {
    pub fn is_unknown(&self) -> bool {
        match self {
            CompletionItemKind::Unknown => true,
            _ => false,
        }
    }
}

// Detecting if some of `items` is unknown/known:
items.iter().find(|item| !item.is_unkown()).is_some())

I didn't try this, so I'm not completely sure how it'll play out. We already have a "know set" of kinds, might has well make the type system aware of it.

I'll try to add some notes to other parts of the code some other day (its getting late here).

[smolck]

Thanks @vhakulinen for the info, I will try to get that working. If it goes well I’ll push a commit for review.

[smolck]

So, how does this look for nvim_bridge.rs? Is this at all like what you were thinking @vhakulinen?

diff --git a/src/nvim_bridge.rs b/src/nvim_bridge.rs
index 72a9777..fe0eec5 100644
--- a/src/nvim_bridge.rs
+++ b/src/nvim_bridge.rs
@@ -211,10 +211,79 @@ pub enum OptionSet {
     NotSupported(String),
 }
 
+#[derive(Clone)]
+pub enum CompletionItemKind {
+    Class,
+    Color,
+    Constant,
+    Constructor,
+    Enum,
+    EnumMember,
+    Event,
+    Function,
+    File,
+    Folder,
+    Field,
+    Interface,
+    Keyword,
+    Method,
+    Module,
+    Operator,
+    Property,
+    Reference,
+    Snippet,
+    Struct,
+    Text,
+    TypeParameter,
+    Unit,
+    Unknown,
+    Value,
+    Variable,
+}
+
+impl CompletionItemKind {
+    pub fn new_from_str(kind: &str) -> Self {
+        match kind {
+            "class" => CompletionItemKind::Class,
+            "color" => CompletionItemKind::Color,
+            "constant" => CompletionItemKind::Constant,
+            "constructor" => CompletionItemKind::Constructor,
+            "enum" => CompletionItemKind::Enum,
+            "enum member" => CompletionItemKind::EnumMember,
+            "event" => CompletionItemKind::Event,
+            "function" => CompletionItemKind::Function,
+            "file" => CompletionItemKind::File,
+            "folder" => CompletionItemKind::Folder,
+            "field" => CompletionItemKind::Field,
+            "interface" => CompletionItemKind::Interface,
+            "keyword" => CompletionItemKind::Keyword,
+            "method" => CompletionItemKind::Method,
+            "module" => CompletionItemKind::Module,
+            "operator" => CompletionItemKind::Operator,
+            "property" => CompletionItemKind::Property,
+            "reference" => CompletionItemKind::Reference,
+            "snippet" => CompletionItemKind::Snippet,
+            "struct" => CompletionItemKind::Struct,
+            "text" => CompletionItemKind::Text,
+            "type parameter" => CompletionItemKind::TypeParameter,
+            "unit" => CompletionItemKind::Unit,
+            "value" => CompletionItemKind::Value,
+            "variable" => CompletionItemKind::Variable,
+            _ => CompletionItemKind::Unknown,
+        }
+    }
+   pub fn is_unknown(&self) -> bool {
+        match self {
+            CompletionItemKind::Unknown => true,
+            _ => false,
+        }
+    }
+}
+
 #[derive(Clone)]
 pub struct CompletionItem {
     pub word: String,
-    pub kind: String,
+    pub kind: CompletionItemKind,
     pub menu: String,
     pub info: String,
 }
@@ -679,7 +748,7 @@ fn parse_redraw_event(args: Vec<Value>) -> Vec<RedrawEvent> {
                     for item in unwrap_array!(args[0]) {
                         let item = unwrap_array!(item);
                         let word = unwrap_str!(item[0]).to_owned();
-                        let kind = unwrap_str!(item[1]).to_owned();
+                        let kind = CompletionItemKind::new_from_str(&unwrap_str!(item[1]).to_owned());
                         let menu = unwrap_str!(item[2]).to_owned();
                         let info = unwrap_str!(item[3]).to_owned();

I made changes in other files to accommodate this diff, but this is the core of the change, so I want to make sure this is solid before I settle on my other diffs (and put them up for review).

[smolck]

One question: should I implement a get_kind function (in the enum) that returns the corresponding string of a CompletionItemKind? The reason I ask is because of the set_tooltip_text function called in the CompletionItemWidgetWrap::create in completion_item_widget.rs (set_tooltip_text needs the kind as a string). The thing is, I don’t want to put needless logic in that function, but I also don’t want to do the same for the enum if it will only be used once. Should I make a function that does that in completion_item_widget.rs, or do something else entirely?

[smolck]

Changes have been pushed for review; any suggestions are welcome and much appreciated. Also, note that I have commented out the set_tooltip_text function in CompletionItemWidgetWrap::create for now, until I know what we want to do there.

[vhakulinen]

Remember to run cargo fmt.

[smolck]

Changes have been pushed for review.

(Removed WIP from title since this only needs maybe a bit more polish.)

[vhakulinen]

Looking good! Just couple minor things that need to be fixed.

[smolck]

@vhakulinen I’ve pushed the changes you requested. If there is anything left that needs fixing/changing please let me know.

As a side note, thank you for all of your help with this PR and my previous one. Taught (and still teaching) me valuable information about Rust, in addition to better, more efficient and idiomatic ways of doing things.

[smolck]

This is a little strange to me. In the below case (Python completions), the behavior is as expected:

in that the bottom, unknown completion text is in line with the others.

However, here (C++ completions) that is not the case:

Notice how the bottom (unknown) completion text is out of line with the others. I don't know if this is important enough to need fixing, but wanted to point it out nonetheless. If anyone knows how to fix this or what's causing it, please let me know.

Also, here is how it looks when all completions are unknown, just as an FYI:

[badosu]

Try adding a dbg!(item.menu) and seeing if the text itself coming from the LSP is padded (for the 1st issue).

[badosu]

LGTM 👍

[vhakulinen]

Merged! Thanks for your efforts!

I made couple small changes: other was to remove unnecessary clone (effectively a copy) and added missing padding on info item when the icon is not shown.