minad / marginalia

:scroll: marginalia.el - Marginalia in the minibuffer
GNU General Public License v3.0
782 stars 27 forks source link

Find a general solution for field adjustment #95

Closed minad closed 2 years ago

minad commented 3 years ago

Unfortunately the current implementation hard codes almost all annotators. Users can provide their own annotators. The approach worked well so far. Nevertheless it would be nice to have the ability to configure annotators without replacing them entirely. Ivy-rich builds up annotators completely dynamically from a configuration. I am not sure I want to go that way, it feels a bit indirect. I prefer plain functions.

cc @tecosaur @iyefrat

tecosaur commented 3 years ago

Personally, I don't feel the current approach is too bad as long as the annotators aren't big monolithic functions. If something can be done nicely and minimally that would be good, I don't currently have any good ideas though.

minad commented 3 years ago

I agree. Decomposing the function is a first good step. Using plain functions feels lispy (but not customize friendly!) and only works well if the functions are not monolithic as you point out.

iyefrat commented 3 years ago

Perhaps a consult-customize type macro that lets you easily modify specific fields in the annotators?

memeplex commented 3 years ago

I've recently reported this downstream at doom emacs tracker: https://github.com/hlissner/doom-emacs/issues/5566. I think the problem is pretty terrible for some of their menus, specially the ones involving elisp symbols, which usually are very long.

image

image

minad commented 3 years ago

@memeplex The alignment issue is a different issue than the issue being discussed here. The alignment problems for small windows are well known. The problem is that the annotation-function is called for each candidate separately, such that we cannot determine the width of the first column. It may make sense to just assume an arbitrary fixed width for small windows to avoid this ugly staggering effect. But I am not sure if there is a solution which gives perfect alignment under all circumstances given that we don't have access to the full information. Another approach would be to replace the annotation-function with a decoration-function which pushes the alignement from Marginalia to the frontend UI, but this requires an extension of the completing-read metadata and coordination with Emacs upstream. In any case, patches or experimentation is welcome.

memeplex commented 3 years ago

I don't know enough of the issues discussed here, but if mine is a different one perhaps I could file a new report for it. In due time I may take a closer look at it and see if I can help.

It may make sense to just assume an arbitrary fixed width for small windows to avoid this ugly staggering effect. But I am not sure if there is a solution which gives perfect alignment

Probably not, but the above screenshots show a situation that will probably profit from anything short of perfect.

The problem is that the annotation-function is called for each candidate separately, such that we cannot determine the width of the first column.

Why are you mentioning only the first column? Isn't it a problem with any column? I see the first column is given to marginalia, but anyway wrt to alignment isn't it in a situation of not knowing in advance the width of every other column?

And can marginalia change the first column? I mean does it return a modified/annotated full row ready to be shown, or just the remaining columns (the annotations themselves)?

minad commented 3 years ago

Probably not, but the above screenshots show a situation that will probably profit from anything short of perfect.

Indeed. On the other hand when I experimented a bit with different ways to do the alignment I didn't really find a satisfactory solution. The whole alignment story only works well for large frames. For me the issue is also not that pressing since I never use small Emacs frames where this matters. But it is understandable if this issue bothers a lot of people.

Why are you mentioning only the first column? Isn't it a problem with any column? I see the first column is given to marginalia, but anyway wrt to alignment isn't it in a situation of not knowing in advance the width of every other column?

No, the other columns have a known width depending on the field type and the minibuffer window width. Large field values are truncated., e.g., the documentation string in the last column.

And can marginalia change the first column? I mean does it return a modified/annotated full row ready to be shown, or just the remaining columns (the annotations themselves)?

Only the remaining columns - the first column is what I call the candidate. The remaining columns are a single string as produced by Marginalia. You can look into the documentation of completing-read in particular the annotation-function and the affixation-function. However the documentation of these Emacs facilities is notoriously bad - in the end one has to read the code.

memeplex commented 3 years ago

Only the remaining columns - the first column is what I call the candidate. The remaining columns are a single string as produced by Marginalia.

So then isn't indentation a responsibility shared with the provider of the candidate? It seems like marginalia is not the part to blame here after all. If the candidates were truncated to a maximum length then all other columns would be rightly aligned, wouldn't them? The provider of the candidate would have to ensure not to hide parts that disambiguate between candidates that share a common part, and other tricky stuff like that, but there seems to be nothing that marginalia can do to help in that regard. Of course, the candidate provider may leverage a library that offers this kind of smart fixed-size formatting, maybe this is something like what vertico already does, I don't know enough of this "new completing ecosystem" yet.

minad commented 3 years ago

I pushed some patch which should improve the alignment for small windows. See https://github.com/minad/marginalia/commit/11df65904ad92fe6969c5867642d3b0b02a3562a. It is not perfect of course but it avoids the ugly staggering effect for small windows. Please give it a try.

memeplex commented 3 years ago

Thanks for the prompt answer. I've been testing your changes. As they are they have null effect since (window-width) is exactly 140 for my laptop. But as soon as I change the threshold to, say, 150, the output is way better, not perfect but like night and day in comparison. Let me understand and play a bit with the code to provide you better feedback. Again, thank you very much.

memeplex commented 3 years ago

Ok, I think I now understand how it works. I believe the lower threshold of 140 is too conservative, I would change it to 150 (I'm on a MacBook Pro 13'' with 15pt font -so a bit larger than average- and already at 140 characters). But, besides that, there seems to be some room for improvement: if you know the max width of the annotations or some estimate of it and you also know the width of the window, then your decision may be influenced by the relationship between those two numbers instead of only by the width of the window. For example the find-file menu looks worse now when left aligned (nothing terrible, but was nicer before) and this is because it has more spare room compared with what it has to show. Instead, the describe-variable menu is pretty tight and has been greatly benefited from you change.

EDIT: simplified a bit because I realize one of the two alternatives I described was likely implied by what you said before.

minad commented 3 years ago

Hmm, for now I reverted back to the simpler old alignment code with more aggressive adjustment of the value of marginalia-truncate-width=window-width/3. The rough idea is that the annotators need 2*marginalia-truncate-width (e.g. documentation string + variable value). And then there should be still enough space for the candidate. Before this change I had set marginalia-truncate-width=window-width/2 which obviously didn't give a satisfactory result. In my tests it works better now for smaller windows.

However if your screen is too small as in your case with 140 there is no chance we can get a better result. There is simply not enough room for the information. For example the docstring usually has a length of ~70, so for a window width of 140 you will already be in the regime where a lot of truncation has to happen. Also consult-buffer with the recent file listing and long path names will quickly lead to long candidate strings.

To be honest for small screens I would just turn off the annotations or experiment with custom annotators which omit the information you are not interested in. This goes now more in the direction of this particular issue of "field adjustment" - currently the annotators cannot be configured on a field/column level. There is no possibility to hide a certain column, e.g., the file permissions.

memeplex commented 3 years ago

Now it's actually perfect, you nailed it!

minad commented 3 years ago

Now it's actually perfect, you nailed it!

Nah, now it is unfortunately worse for large windows. I am not willing to compromise there. For now I reverted back to the old settings. I suggest you tune marginalia-truncate-width=window-width/3 for your setup. The problem is that not all annotators are written such that marginalia-truncate-width=window-width/3 makes sense. Maybe we should tweak this on a case by case basis for each annotator. However I think there is probably no one size fits it all solution.

memeplex commented 3 years ago

Oh :( I see, I will think about it and let you know if I find a sensible solution, I still need to understand some details of the implementation. Thank you very much anyway, at least the describe-variable and find-file screens were 100% right in my laptop for a moment ;).

memeplex commented 3 years ago

I believe a simple heuristic along the following lines may do it:

Then if menu is dense and screen is small, truncate to window / 3, otherwise truncate to window / 2.

Of course each annotator will have to specify if its menu is dense or sparse, but that seems simple enough and the sensible default would be sparse (for now, the only dense case I've seen is describe-variable).

Does that make any sense to you?

minad commented 3 years ago

This doesn't sound like a good solution. You should also take even smaller screens into account. For these it doesn't work well.

To really solve the issue one would have to take the width of the candidates into account, e.g., truncate-width = (window-width - max-candidate-width) / 2. But there is another complication - we don't have access to the maximum candidate width if the annotation-function is used. We have access to the maximum candidate width of the displayed candidates however if the affixation-function is used. But then we will get caching issues, since each annotation is cached separately. This will then lead to artifacts. Caching is necessary since some annotators are too slow when scrolling, e.g., the file annotator.

So the whole implementation and API design is kind of a dead end and should be reworked. There is no simple fix given the current design. Such a reworked design would be a larger undertaking and rather be like a Marginalia 2.0 with reworked caching, customizable field attributes (this issue) and separation of alignment from the annotation field computation. The alignment should then be performed after the caching and take the candidate widths into account.

But even this design would probably not be perfectly satisfactory since the annotators have to produce a string in the end which is then just concatenated with the candidate string in the UI. This is all very primitive. A real solution would be if the columns would be aligned entirely by the UI in the style of a tabulated-list, but as I mentioned this requires a better API provided by Emacs. We cannot use the annotation/affixation-function to achieve this.

minad commented 3 years ago

A possible solution for formatting on the side of the UI is for example the tabulated-list-mode. Embark already uses it for its tabular formatting. Candidates and annotations could simply be passed from the completion table to the UI as lists.

(with-temp-buffer (setq tabulated-list-format [("first" 10 nil) ("second" 10 nil) ("third" 10 nil)] tabulated-list-entries '((a ["a1" "a2" "a3"]) (b ["b1" "b2" "b3"]) (c ["c1" "c2" "c3"]) (d ["d1" "d2" "d3"]))) (tabulated-list-print) (buffer-substring-no-properties (point-min) (point-max)))

minad commented 2 years ago

Close as wontfix. The current annotators are good enough. I suggest to write custom annotators as plain Elisp functions, instead of complicating the existing infrastructure with configurability. If someone has good proposals, they are still welcome of course.