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

Is it possible to show annotations in icomplete-vertical? #16

Closed
minad opened this issue Nov 30, 2020 · 15 comments
Closed

Is it possible to show annotations in icomplete-vertical? #16

minad opened this issue Nov 30, 2020 · 15 comments

Comments

@minad
Copy link
Contributor

minad commented Nov 30, 2020

I am working on https://github.com/minad/consult and would like to make it compatible with icomplete-vertical. Consult overwrites some annotation functions. The provided annotation however are only shown in the Completions buffer, but not in the icomplete minibuffer. Is there are possibility to make this work? Is there support for annotations in icomplete?

@oantolin
Copy link
Owner

oantolin commented Nov 30, 2020

As far as I know there is currently no support for annotations in icomplete. It probably isn't hard to add though. I think I would try propertizing the completion candidates to display the annotations. That way they would be visible, but not affect completion.

As far as I know, the annotations built-in to Emacs are all very short, only 1-3 characters. Even (horizontal) icomplete should probably show those. I'm guessing consult uses longer annotations, those should probably only be shown in icomplete-vertical. Maybe in the future Emacs should even have separate metadata functions for short and long annotations. The completions buffer and icomplete could show the short ones, and things like icomplete-vertical or the new detailed view in the completions buffer could show the longer annotations.

@basil-conto
Copy link

As far as I know, the annotations built-in to Emacs are all very short, only 1-3 characters.

To an extent, but not always. For example, Emacs 28 has started showing command aliases and key bindings during M-x completion as annotations by default.

In addition to annotations, there exist now also "affixations", which can get quite large; see abo-abo/swiper#2732 for an example.

Finally, completions-format can now also take on the value one-column for single-column output in *Completions*.

HTH.

@minad
Copy link
Contributor Author

minad commented Nov 30, 2020

@oantolin @basil-conto Thank you!

I think I would try propertizing the completion candidates to display the annotations. That way they would be visible, but not affect completion.

It would be nice if there would be some standard for this. Note that selectrum already defines a few annotation properties which could be replaced with more standard ones.

As far as I know, the annotations built-in to Emacs are all very short, only 1-3 characters. Even (horizontal) icomplete should probably show those. I'm guessing consult uses longer annotations, those should probably only be shown in icomplete-vertical. Maybe in the future Emacs should even have separate metadata functions for short and long annotations. The completions buffer and icomplete could show the short ones, and things like icomplete-vertical or the new detailed vied in the completions buffer could show the longer annotations.

Yes that would be a possibility. But I hope more that it is possible to somehow unify/simplify things in the Emacs completion system. The affixation function unifies some things but also adds complexity.

@oantolin
Copy link
Owner

Finally, completions-format can now also take on the value one-column for single-column output in *Completions*.

That is what I meant by "the new detailed view": I had forgotten the actual name of the format.

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

I've added initial support to display annotations in commit 272e525. I have a few questions about possible refinements:

  • I'm displaying them right next to the candidate, just like the *Completions* buffer does. I guess a fancy annotation function can use :align-to to put the annotation elsewhere. Does this seem sufficient?

  • I'm displaying the annotation in the completions-annotations face. Should I instead make a new face that inherits from that one? (@protesilaos always gives me good advice on faces).

  • You now always get annotations if available. I personally see no downside to always having annotations, but maybe this should be controlled by a user option.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

I'm displaying the annotation in the completions-annotations face. Should I instead make a new face that inherits from that one? (@protesilaos always gives me good advice on faces).

I just discussed this with @clemera in radian-software/selectrum#250 and we decided there to not add a face if there is already a face present in the string returned by the annotation-function. Otherwise add a face, selectrum uses its own face derived from completions-annotations. But you could also use completions-annotations directly I guess.

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

I used to add the face using font-lock-prepend-text-property and now I switched to font-lock-append-text-property. Do you think that's good enough? That still merges the face with completions-annotations, but the attributes in the face present in the annotation string take precedence. I just tried customize-face with marginalia-mode active (I'm a marginalia user now! :P) and it looks fine, the specific qualities of each face do come through.

@protesilaos
Copy link
Contributor

I'm displaying the annotation in the completions-annotations face. Should I instead make a new face that inherits from that one?

I think it is okay to use completions-annotations directly. Though also check what @minad writes about selectrum.

My approach is this:

  • Is the construct's scope the same as that of the generic face it uses? Then there is no need for a new face even if that were to be a mere wrapper that inherits the generic one. Your case here fits this scenario.

  • Is the construct's scope different than what its generic face suggests? A new face should be created. For example, the "autoload" special comments in Elisp get the font-lock-warning-face though they are not really a warning---they could, in principle, use their own face, even if only to inherit from that one. Same for the M-x vc-dir buffer: all those faces are hard-wired, so if you want to make that buffer present directories as in dired, you have to change font-lock itself (so break your syntax highlighting).

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

Thanks @protesilaos! I'll stick with completions-annotations then. Now it's just a matter of deciding whether always to merge in completions-annotations as the last face or to refrain from using it all together when the annotation has faces. (I currently merge.)

@oantolin oantolin closed this as completed Dec 3, 2020
@oantolin oantolin reopened this Dec 3, 2020
@protesilaos
Copy link
Contributor

Now it's just a matter of deciding whether always to merge in completions-annotations as the last face or to refrain from using it all together when the annotation has faces.

Please help me understand this "merge" better (I believe I use it elsewhere but need to be sure). The text-to-be-annotated has some properties and those get combined with completions-annotations. So if the text has, say, a :foreground it is overruled by the equivalent of completions-annotations. But if it has a unique property like :height 1.5, it retains it because completions-annotations does not specify anything of its own.

Is that the gist of it? If so, you would need to check whether that "merging" can lead to problems (maybe with size and spacing, in the case of distinct font families). If not, then it sounds good for now.

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

That is also what I understand merging face attributes does. (You mention the "text-to-be-annotated", but it is more accurate to say we are discussing the "text-of-the-annotation".)

You can merge in either order. What you described, which is completions-annotations taking precedence, is what the first version of this feature (272e525) did. In commit d89f4ea I changed the order, so that now the face attributes in the annotation take precedence.

We definitely don't want completions-annotations to take precedence, so this change is good. The remaining question is: would it be even better to not merge with completions-annotations at all when the annotation has some faces applied?

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

The choice @clemera took in selectrum is to not merge if there are faces already present in the annotation string. Our interpretation of this is somehow that the annotation function takes control over the face if faces are in the string. This is much better for example for the marginalia-annotate-faces function, which would get messed up if you merge. It makes sense though to add the completions-annotations face if no face is present in the annotation string, as a default annotation face.

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

Yep, I just came to the same conclusion after trying it both ways: don't merge, only apply face if none present. I'll push commits to that effect both here and to embark in a minute.

Done! 94684cc

@minad
Copy link
Contributor Author

minad commented Dec 4, 2020

Cool! It is great if the two completion systems are consistent here!

@minad
Copy link
Contributor Author

minad commented Dec 4, 2020

@oantolin I tested this with marginalia-mode. Works great! I guess we can close this!

@minad minad closed this as completed Dec 4, 2020
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

No branches or pull requests

4 participants