Skip to content

Conversation

andreyorst
Copy link
Contributor

@andreyorst andreyorst commented Jul 3, 2021

Fix #3017

This adds icon mapping for various completion kinds, allowing company-mode users to enable icons for completion items.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

@bbatsov
Copy link
Member

bbatsov commented Jul 6, 2021

Can you attach some screenshot of the end result?

This adds icon mapping for various completion kinds, allowing company-mode users to enable icons for completion items.
@andreyorst
Copy link
Contributor Author

Can you attach some screenshot of the end result?

sure!

methods variables and macros
image

namespace:

image

defrecord:

image

I'm not sure how to get to other icons, I don't have the code that will trigger those :)

("static-field" field)
("static-method" interface)
("type" parameter)
("var" variable))
Copy link
Member

@expez expez Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use constant for this (or vice versa) so you can easily tell the difference between local and global variables in the completion list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to change most of those afterwards anyways. E.g. I don't think that macro is a misc, but rather a built-in and so on. But I think there's little point in debating this too much in this particular PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this?
image

I think constant may be a bit misleading, but maybe it's ok. I'll let @bbatsov decide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many nuances to consider and this has to be aligned with how clojure-mode works internally as well. That's why I think we can sort it out later. Just update the changelog and for now we're good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. I don't think that macro is a misc

Btw I've changed this to use macro icon, but given that there's no macro icon in company mode, it will default to misc icon automatically. Originally I've used misc because I was not aware of this mechanism. Maybe we can submit an issue to company mode so they could include a macro icon? Or maybe this should be done to vscode, because the icons are from there, and this will benefit to other plugins that use NREPL, like Calva.

Also triangle square and circle kinda seem like a good icon to represent macros, as this is really puzzling :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. We basically mirror VS Code's type. Not to say we can't extend the list, but getting nice icons can be a problem.

Anyway, check out https://github.com/emacs-mirror/emacs/blob/6ec3cf1ccb5380acc376e89140b8d3a7fa4e471a/lisp/progmodes/elisp-mode.el#L647-L655

Since there are no "real" keywords in Lisp, reusing this kind for macros and special forms seemed like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding local variables, we use the value kind (following the example of some LSP servers). Which differentiates them nicely.

Copy link
Contributor Author

@andreyorst andreyorst Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are no "real" keywords in Lisp, reusing this kind for macros and special forms seemed like a good idea.

Clojure actually does have real keywords, and CIDER annotates those with <k>:

image

Though the icon kinda suggests that this is language-level keyword, not data keyword, so maybe a different icon could be used for those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this context I actually meant keywords in the sense of "built-ins"/"reserved words". That's how elisp-mode and clojure-mode work. Anyways, not a big deal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreyorst These kind of keywords are present in Elisp as well.

But that's not what is usually called a keyword in most programming languages (in Ruby, we call them "symbols", for example). And as someone who routinely works with several languages, I like to see annotations (and icons) have more universal meanings.

@bbatsov bbatsov merged commit fe8cf24 into clojure-emacs:master Jul 6, 2021
@bbatsov
Copy link
Member

bbatsov commented Jul 6, 2021

Thanks!

@andreyorst andreyorst deleted the patch-1 branch July 6, 2021 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

company-mode now supports icons, but CIDER's backend needs adjusting

4 participants