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

Support non-fixed-width and icon based prefixes #38

Closed
jdtsmith opened this issue Jul 18, 2021 · 26 comments
Closed

Support non-fixed-width and icon based prefixes #38

jdtsmith opened this issue Jul 18, 2021 · 26 comments
Labels
enhancement New feature or request

Comments

@jdtsmith
Copy link
Collaborator

jdtsmith commented Jul 18, 2021

Company does a nice job lining up its overlay such that the text in buffer is aligned with the candidates, even when an icon is showing:

image

Corfu currently aligns the prefix from :affixation-function to the left:

image

It would be better to move the child frame over so the prefix is to the start of the candidate. Probably have to compute the maximum prefix width for this offset.

I also notice that when prefixes have different widths, this makes the left sides of the candidates jagged:

image

Maybe the solution is "don't do that", i.e. either space pad for text prefixes, or use identical widths for icons, etc. (EDIT: actually scratch that, you are already using specified space to right align suffixes, so no harm in left justifying candidates when prefixes are included, using the same trick).

@jdtsmith
Copy link
Collaborator Author

jdtsmith commented Jul 19, 2021

I now have a solution for this (that actually uses the method discussed by you and the emacs devs here to calculate pixel size). It works quite well even with fractional char width spaces in the prefix:

image

The issue remains that the method used to calculate the maximum width:

(apply #'max corfu-min-width
                            (mapcar #'string-width lines))

cannot account for fractional character width spaces (or images, or invisibility, or ...). So it should probably also use the window-text-pixel-size method. In my tests this was fast enough to run every show-candidates. Let me know if you want a PR.

[Update: I can successfully calculate width now too and it works great. I do think corfu should exclude the prefix from consideration against corfu-max-width, since it may contain non-character data like icons/etc. of non-character-multiple width]

@jdtsmith
Copy link
Collaborator Author

Let me know if you are interested in this. Here's the utility function I'm calling to find both maximum displayed prefix width and "total" width (though I'll argue below the latter usage is unnecessary):

(defun corfu--max-width-pixels (lines)
  "Return the max display width in pixels of LINES."
  (let ((buffer (get-buffer-create " *corfu-width*"))
	(old-buffer (window-buffer)))
    (with-current-buffer buffer ; see discussion of bug #47712
      (erase-buffer)
      (insert lines)
      (set-window-buffer (selected-window) buffer)
      (prog1
	  (car (window-text-pixel-size nil (point-min) (point-max)))
 	(set-window-buffer (selected-window) old-buffer)))))

I'm considering a small package building on corfu (maybe corfu-kind-icons) which uses the :company-kind property and all-the-icons to put kind icons into the affixation prefix (likely by advising corfu--affixate to supply kind-based icon prefixes where not already specified). Note that LSP, Cider, and even elisp-mode inside emacs are now already specifying :company-kind.

I personally think it makes sense for corfu itself to enforce some "expectations" on the affixation candidate/prefix/suffix. One simplifying assumption is that any non-integer-character-width information (like images, variable width glyphs, etc.) comes in the prefix only. In this case, the full displayed width can be found using max_prefix_width + cw * max_string_width(candidate + suffix). It would then follow that only candidate+suffix will be subject to corfu-max-width. Also, corfu will itself take care of "aligning" the prefix into one fixed-width column, so that the candidates line up.

@jdtsmith jdtsmith changed the title Align the child frame so that candidates align with the in-buffer text Support non-fixed-width and icon based prefixes Jul 20, 2021
@minad
Copy link
Owner

minad commented Jul 20, 2021

I will look into this at some point, but it is low priority. I am not particularly fond of icons. Icons don't work well on Emacs 27, font cache issues etc. Furthermore as I already mentioned, I am critical regarding Company extensions. Why is there no attempt being made to propose general extensions to the completion metadata, as I did for the group-function? Note that of the examples you mention, both LSP and Cider implement broken completion tables. At least there are issues on the tracker here. Eglot on the other hand implements a compliant completion table.

@minad
Copy link
Owner

minad commented Jul 20, 2021

Regarding the width/alignment computation, one could either rely on the completion table to perform the alignment or we could perform the alignment in Corfu. For the completing-read annotation/affixations, I am usually relying on the table to perform the alignment, e.g., Marginalia performs the alignment itself instead of relying on the Vertico/Selectrum frontend UI. In the case of company-kind I guess the situation is different, for the icons the width computation should be performed in the frontend.

@jdtsmith
Copy link
Collaborator Author

I will look into this at some point, but it is low priority. I am not particularly fond of icons. Icons don't work well on Emacs 27, font cache issues etc. Furthermore as I already mentioned, I am critical regarding Company extensions. Why is there no attempt being made to propose general extensions to the completion metadata, as I did for the group-function? Note that of the examples you mention, both LSP and Cider implement broken completion tables. At least there are issues on the tracker here. Eglot on the other hand implements a compliant completion table.

I entirely agree that it would be preferable to have an official set of recommended extra properties to convey additional metadata information such as "kind", that company and others could use. But since company has a large user base, it's not crazy for LSP etc. to target what will work out of the box. So I'm very doubtful :company-kind will just go away at this point. I'm also not overwhelmed at the genericness of affixation-function, which doesn't really convey useful metadata, and seems far more aimed at presentation.

In any case, the prefix alignment issue isn't truly tied to icons, just non-fixed-width characters/glyphs, which could even occur for unicode browsers, etc.

@minad
Copy link
Owner

minad commented Jul 20, 2021

company-kind will get superseded as soon as something generic and better becomes available in Emacs proper. The affixation-function may also get replaced then. The idea is to have some decoration-function which returns multiple metadata fields per candidate. There has been some discussion on emacs-devel about this and I also tried a few designs, but so far nothing has come out of that.

@jdtsmith
Copy link
Collaborator Author

Regarding the width/alignment computation, one could either rely on the completion table to perform the alignment or we could perform the alignment in Corfu. For the completing-read annotation/affixations, I am usually relying on the table to perform the alignment, e.g., Marginalia performs the alignment itself instead of relying on the Vertico/Selectrum frontend UI. In the case of company-kind I guess the situation is different, for the icons the width computation should be performed in the frontend.

Given that affixation-function is new, and also given the variety of different ways you could display a simple prefix+candidate+suffix, I think it's reasonable to start with the idea that completion tables can do whatever coloring/styling/etc. they want, but alignment is the purview of the final UI. In any case, I already have an alignment solution that is working, using the helper function above. It would require a few small changes:

  • In corfu--show-candidates, compute the max character width, and max prefix pixel width (if there are any), passing the latter on to corfu--popup-show as an additional optional argument (to offset the frame x position so as to line up with in-buffer text).
  • Have corfu--format-candidate, with the help of two optional parameters:
    1. truncate the candidate + suffix (which become the only bits subject to corfu-max-width), and
    2. right-pad the prefix (if any) so as to line up the candidates.
  • Now corfu--popup-show does not need to compute widths or do the truncation, as that will have been done already (along with the candidate alignment).

Let me know if this sounds like something you'd support as a PR and I can work it up further.

company-kind will get superseded as soon as something generic and better becomes available in Emacs proper. The affixation-function may also get replaced then. The idea is to have some decoration-function which returns multiple metadata fields per candidate. There has been some discussion on emacs-devel about this and I also tried a few designs, but so far nothing has come out of that.

That would be a great solution. To be clear I'm certainly not advocating having corfu itself support company-kind, merely to be able to align candidates below the in-buffer text when there are prefixes (which can even be composed of pure characters like m and fu above). This alignment functionality will be useful for:

  1. any completion table supplying an affixation function with prefixes (even constant width ones) and
  2. any with prefixes of non-uniform width.

With this in place, I'd probably work up a small corfu add on that is company-kind aware, which could eventually be obsoleted when some real metadata improvements arrive in Emacs ≥29. Thoughts?

Thanks.

@minad
Copy link
Owner

minad commented Jul 21, 2021

I don't think it is that clear. The annotation/affixation function could also do alignment. This is all not well-defined unfortunately - this is also the main reason why Company goes with its own extensions, since the generic functionality is not well-defined enough. So I don't think the alignment approach you propose will work out. It may work out for most scenarios and it will work out for your kind extension, but not generally. The alignment can only be performed properly if the prefix is guaranteed to not already contain alignment spaces. The correct approach to implement your extension would be to directly support company-kind instead of going via the affixation-function. I may have to adjust or extract some Corfu functions to allow advising the "interesting" parts of the code which affect the alignment. In Vertico I have vertico--format-candidate which can be advised for example by extensions. My recommendation for now would be to just hack in company-kind support directly in Corfu and then we can see if it makes sense to keep it separate and how we could achieve that by restructuring the Corfu formatting and popup code. The small changes you outlined above seem reasonable.

@jdtsmith
Copy link
Collaborator Author

That would certainly be a bother if the prefixes came pre-aligned. Without some assumption there is nothing to do there that would be reliable. The "offsetting the frame to line up with in-buffer text" should work independent of that issue (though of course you'd be aligning to a "ragged edge"). But my other proposed addition (space-padding prefixes) doesn't make sense unless we can be reasonably sure that corfu will be in charge of alignment (at least between prefixes and candidates). I suppose you could look to see if a prefix ends in a (display ('space... :align-to ...) and only attempt to insert padding to align to the candidate position if not. Not sure how reliable that would be. The other option is to just say "if you want alignment, do it yourself". If corfu itself inserts prefixes (based on some hypothetical metadata from the candidate) then it would do the work of aligning them itself.

Maybe we should think on this a bit and see if something obvious comes to us. BTW, if not for "kind identifiers" (either text or icon based), what do you envision the use of prefixes might be?

@minad
Copy link
Owner

minad commented Jul 21, 2021

Maybe we should think on this a bit and see if something obvious comes to us.

Agree. I've also considered detecting the display-spaces, but this will also be brittle. I would like to wait a bit if something better comes up in Emacs proper itself. The solution proposed to Emacs proper should of course specify how alignment should be treated.

what do you envision the use of prefixes might be?

Icons are the obvious candidate, see also https://github.com/iyefrat/all-the-icons-completion. Besides that I am using prefixes for the line numbers in consult-line (preformatted, prealigned) and Emacs 28 uses the prefixes to attach the symbol class in describe-symbol. Marginalia also shows the symbol class for describe-symbol (the characters cfv...) but puts it after the candidate instead. Note that these cases are more relevant for completing-read and not completion-at-point.

@jdtsmith
Copy link
Collaborator Author

If you'd like we could split this into two: I could prepare an "offset the frame to line up the candidate when prefixes are there" and "apply corfu-max-width to candidate+suffix only, then calculate a full display width` PR and leave "prefix<->candidate alignment" for another day, and/or a better in-built Emacs solution. Then people who want prefixes will for now have to pre-align them (or use constant widths).

@minad
Copy link
Owner

minad commented Jul 21, 2021

For now I prefer to do nothing on that front, it is better to wait a bit more. In the long term it would be good to have a proper solution for nicely aligned icons and annotations.

Why would we want to restrict the max-width only to candidate+suffix? The max-width is not that relevant anyways, it is just a precaution to ensure that the popup stays within reasonable bounds. It does not matter if it is a bit smaller or larger. More precise width computations may make sense, but do we have issues with the current method right now? Does the current method lead to artifacts?

@jdtsmith
Copy link
Collaborator Author

Does the current method lead to artifacts?

Only that once you admit alignment, icons, wide characters, etc. "width" is now a concept of pixels not characters. But if we preserve the idea that candidate+suffix are "characters" there is no need to measure their widths (only the prefix's). Not such a big issue.

@dgutov
Copy link

dgutov commented Nov 8, 2021

Y'all might be interested in this thread: company-mode/company-mode#1088

@minad
Copy link
Owner

minad commented Nov 8, 2021

Copying my comment from #72 (comment) here for better visibility:


It would be nice to have an icon package first, which could be used by both Corfu and Company. See my comment #18 (comment). If we add support directly to Corfu (e.g. via auto detecting if the icon package is available) or if it should be done via a separate package is up to debate. The icon package should ideally provide an api similar to the all-the-icons package, but should be based on svg.

  • icon-for-file
  • icon-for-mode
  • icon-for-kind
  • ...

@jdtsmith
Copy link
Collaborator Author

jdtsmith commented Nov 8, 2021

I had seen that, which is in fact where I got the idea for this little translator for text-based kind prefixes. These icons are monochrome, but pretty well-designed and extensive (search). E.g. function is quite nice. I may give them a try. @minad suggests making my kind-affixator corfu-independent, which means it would work with any UI that respects the affixation-function prefix, allowing users to fork and substitute their own icons without touching company/corfu/etc. code.

@dgutov
Copy link

dgutov commented Nov 8, 2021

FWIW, Company icons (or rendering of kind via other means) are user-customizable. One can swap for another icon set by providing another directory, or write different rendering logic.

I suppose a frontend-agnostic setup for kind -> icon mapping wouldn't hurt, but since people usually choose just one completion frontend, it probably isn't very important.

which means it would work with any UI that respects the affixation-function prefix

Meaning, not Company.

@minad minad closed this as completed Nov 9, 2021
@minad minad reopened this Nov 9, 2021
@minad
Copy link
Owner

minad commented Nov 9, 2021

@jdtsmith The monochrome icons look indeed nice. There is really a lot of design freedom here. It may involve changing the background color as you did in your corfu-kind package and which was also discussed in the company issue, where some inspiration was taken from sublime text. While I like the design, this certainly complicates the integration a bit in comparison to vscode like icons.

@dgutov
Copy link

dgutov commented Nov 10, 2021

where some inspiration was taken from sublime text

Atom, actually.

this certainly complicates the integration a bit in comparison to vscode like icons

I'm probably out of context, but if the shared package just provides a function mapping icon name to file name (with exact directories configurable by the user), this icon set can work just as good as VS Code's one. As long as it has all the necessary icons.

@minad
Copy link
Owner

minad commented Nov 10, 2021

I'm probably out of context, but if the shared package just provides a function mapping icon name to file name (with exact directories configurable by the user), this icon set can work just as good as VS Code's one. As long as it has all the necessary icons.

What I meant is that for vscode icons you just show the icons without additional styling while for monochrome icons you apply additional styling like backgrounds etc. I am not yet sure on how to integrate this best in Corfu. Maybe Corfu will just get a corfu-kind-formatter-function customizable variable which can be set by an icon provider.

@dgutov
Copy link

dgutov commented Nov 10, 2021

I think whether to use a colored background (or not), probably doesn't depend on whether the icons are monochrome. Seems like an orthogonal setting.

But I suppose monochrome icons could define some special per-icon foregrounds.

@jdtsmith
Copy link
Collaborator Author

Nice thing I'm finding about monochrome is a) there are giant libraries of them so people can pick exactly what they want, b) there is nice synergy with text-only "icons" in the sense that color conveys plenty of the meaning (variable? font-lock-variable-name-face).

@jdtsmith
Copy link
Collaborator Author

jdtsmith commented Nov 10, 2021

@dgutov BTW, is the list of possible kind's described somewhere? What's to stop backends from providing un-specified kind values? Is t meant to be a catchall?

@dgutov
Copy link

dgutov commented Nov 10, 2021

BTW, is the list of possible kind's described somewhere?

The docstring of company-backends answers this question.

Is t meant to be a catchall?

Yup.

What's to stop backends from providing un-specified kind values?

Nothing, I suppose. Aside from the worry that the users won't see the appropriate icons beside the "unknown" kinds.

@minad
Copy link
Owner

minad commented Nov 10, 2021

I added the corfu-kind-formatter function in #75. It should be possible to plug this together with @jdtsmith's kind-icon library soon. kind-icon should probably also work for Company!

@minad minad closed this as completed Nov 10, 2021
@dgutov
Copy link

dgutov commented Nov 10, 2021

kind-icon should probably also work for Company!

Maybe after it's in GNU ELPA. Or it can be an optional setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants