Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#42] Utilize annotation and docsig function in completion-in-region #62

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

AmaiKinono
Copy link
Contributor

This implements ideas mentioned in #42 (comment).

How does it look like

image

This is selectrum-completion-in-region on a backend created by me. I use the :annotation-function for the kind (variable, function, ...), which is displayed as the prefix; and I use the :company-docsig for the signature, which is displayed using right-margin text.

How does it work

The completion-at-point function let binds completion-extra-properties to the extra props offered by backends, so we could just get the things we need from it.

What can still be improved

  • I use a italic face for those annotations, but I think it's better to also use some gray color to distinguish them from the candidate. Are you ok with me adding such face?

  • Maybe we should also handle :exit-function (see the docstring of completion-extra-properties). I haven't study what is it for. company-capf also handles :predicate (see the docstring of completion-at-point-functions.

@AmaiKinono
Copy link
Contributor Author

Just a random thought: Sometimes I found myself really want to also filter those annotations (e.g., type "function" to filter out all functions). But seems it's impossible from the fact that the prefix is originally for read-file-name function, and such filtering could be awkward if not use prescient.

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

Looks cool!

selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
@raxod502 raxod502 added the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label Apr 19, 2020
@clemera
Copy link
Collaborator

clemera commented Apr 20, 2020

@AmaiKinono

Just a random thought: Sometimes I found myself really want to also filter those annotations (e.g., type "function" to filter out all functions). But seems it's impossible from the fact that the prefix is originally for read-file-name function, and such filtering could be awkward if not use prescient.

I have thought about this for other purposes as well. I think selectrum could offer a neat unique approach to handle candidates from "multiple sources". By prefixing the query with for example "f " you could restrict candidates to only functions. Available search prefixes could be displayed in a lv like window above the prompt. That is again a feature which enhances completing-read and could make other packages depend on selectrum specific features though.

@AmaiKinono
Copy link
Contributor Author

About :predicate: We don't need to handle it, since it's passed to the predicate arg by completion-at-point, and then it's handled by completion-all-completions.

About :exit-function: I still don't quite understand it's use. I once thought it's for something like rebalancing an expression or jump out of closing parenthesis after completion, but it actually takes action based on whether the text is complete, and whether it can be further completed, see the docstring of completion-extra-properties. I feel this is meaningless for a Selectrum UI.

About "meta": There is such thing called "completion metadata", I don't understand it, but it's an (undocumented) arg of completion-all-completions, and company-capf uses it. I forgot why I use nil for it (#42 (comment)), but it works for me since I created that function.

@AmaiKinono
Copy link
Contributor Author

@clemera

I think selectrum could offer a neat unique approach to handle candidates from "multiple sources"

I am not a fan of this approach, but I've heard helm can let you filter from results offered by multiple backends simultaneously, and this is loved by helm users.

I once saw similar features in VSCode: In its "command pallete" (kind of like M-xin Emacs), prefix your input by # to search for symbols, and @ for symbols in current file. Snails takes an similar approach.

But I have no idea how could Selectrum handle "multiple sources". It's obvious that you can build your own command that put together buffers, recent files, imenu indexes and other things together, and distinguish them by a text property, then write a selectrum-refine-candidates-function that handles the prefix in user input, but I guess that's not what you want, right?

@clemera
Copy link
Collaborator

clemera commented Apr 20, 2020

But I have no idea how could Selectrum handle "multiple sources". It's obvious that you can build your own command that put together buffers, recent files, imenu indexes and other things together, and distinguish them by a text property, then write a selectrum-refine-candidates-function that handles the prefix in user input, but I guess that's not what you want, right?

There could be a general text property which defines such prefixes. For example selectrum-prefix with a value of "f" for functions to keep the example. If the user types just one letter and a space it's obvious this is not regular search and selectrum could filter all candidates to those with a selectrum-prefix value of that letter.

@raxod502
Copy link
Member

I don't like that approach. It feels very special-cased to me. On the other hand, you could apply your own text property to each candidate, and then pass a dynamic collection function to selectrum-read which inspects the first character of the user input and performs a filtering on that text property before delegating to the existing selectrum-refine-candidates-function. What do you think of that?

@clemera
Copy link
Collaborator

clemera commented Apr 22, 2020

Yes that is an easy way to do it. I do something similar with a custom switch buffer command I currently use with selectrum. I like that approach: search the whole collection unless I specify a special filter pattern. I think it is a great feature and I remember there was an issue to add it to ivy at some point.

What is you plan in regard to more useful default commands? I think it would be great to have a counsel like package which wouldn't depend on ivy. For example for switching buffers it is really useful to include the recent files, too. Those commands could be added to selectrum directly, as well but probably this would quickly lead to a situation where you have commands which depend on some selectrum features. Just curious what you think about this?

@raxod502
Copy link
Member

I think such commands should be kept in personal configurations until we accrue such a volume of them that it makes sense to design a permanent solution. By implicitly encouraging people to design their commands so that they work better with plain completing-read, we help the ecosystem as a whole.

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

Ok!

@raxod502 raxod502 merged commit f9baf53 into radian-software:master Apr 24, 2020
@raxod502 raxod502 removed the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label Apr 24, 2020
@clemera
Copy link
Collaborator

clemera commented Apr 25, 2020

I think such commands should be kept in personal configurations until we accrue such a volume of them that it makes sense to design a permanent solution. By implicitly encouraging people to design their commands so that they work better with plain completing-read, we help the ecosystem as a whole.

I agree with that in general but it would help to have a central place where users can lookup and share those commands. A wiki for selectrum would be a nice place for sharing such commands I think.

@raxod502
Copy link
Member

Good idea! The Selectrum wiki is officially open for business, then :)

@clemera
Copy link
Collaborator

clemera commented Apr 25, 2020

Nice!

@andyleejordan
Copy link
Contributor

I was just customizing the face of this, and I think inheriting from the built-in face font-lock-doc-face seems appropriate. It should look like expected "documentation" face regardless of the user's theme.

P.S. I love this, thank you all for your work!

@AmaiKinono
Copy link
Contributor Author

Glad you love it ;)

inheriting from the built-in face font-lock-doc-face seems appropriate. It should look like expected "documentation" face regardless of the user's theme.

I'm not sure about this. I saw you use lsp-mode with Selectrum, so I searched for some lsp-mode pictures (I don't use it). Some of them use very colorful/bold face for annotations, which actually looks great for me.

I think it's better to offer two faces, one for the annotation (on the left side), another for the docsig (on the right side), so the user could customize them as he/she want. Does this seem reasonable for you?

@andyleejordan
Copy link
Contributor

Isn't font-lock-doc-face a built-in? Perhaps we can find a better one, but I think in general we ought to inherit from built-in faces so that it's more likely that users' themes will automatically customize it appropriately, instead of hard-coding :foreground "#888888".

Actually, I was trying to figure out why the common part in my completions looked so ugly. Turns out that it's by default using completions-common-part. And guess what other face is right there: completions-annotations.

I'm going to open a PR deleting selectrum-completion-annotation and instead replacing its use with completions-annotations, as this makes perfect sense for what is the completions' annotations face. I'll update the docs to point users to customizing completions-annotations, completions-common-part, and completions-first-difference for this function.

Also, I agree with you, I would prefer these to be colorful, and my theme (Solarized) only customizes one of these faces, so I'm going to update it as well.

@AmaiKinono
Copy link
Contributor Author

I think in general we ought to inherit from built-in faces so that it's more likely that users' themes will automatically customize it appropriately, instead of hard-coding :foreground "#888888".

I agree with this. However, there's no essential difficulties in customizing: you use face-defface-spec as spec-type in face-spec-set, then all attributes defined by defface are gone, so it's like you are redefining the face. Then you can inherit from some other face and don't do anything else.

deleting selectrum-completion-annotation and instead replacing its use with completions-annotations

What if the user want to use one face for annotation, and another for docsig? That's why I suggested offering two faces.

@andyleejordan
Copy link
Contributor

What if the user want to use one face for annotation, and another for docsig? That's why I suggested offering two faces.

Hm, I see what you mean, people may wish to do that (including myself). Let me amend that PR; I still think we ought to default to inheritance than just making up new colors, but we could certainly provide two faces for users' to change as they see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants