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

Ideas regarding categories/annotations #41

Closed
minad opened this issue Dec 2, 2020 · 14 comments
Closed

Ideas regarding categories/annotations #41

minad opened this issue Dec 2, 2020 · 14 comments

Comments

@minad
Copy link
Contributor

minad commented Dec 2, 2020

@oantolin based on our discussion at https://www.reddit.com/r/emacs/comments/k3c0u7/consult_counselswiper_alternative_for/gef1jxo?utm_source=share&utm_medium=web2x&context=3

So far this looks to me as if we can create two relatively simply and fully independent packages. Maybe this does not work out but it seems to be worth a try.

Category package (Linneaus)

The purpose of this package is to add (or refine?) the category completion metadata of commands which don't provide a category. There are various ad-hoc possibilities to infer the category information as has been done in the Embark classifier system.

  • Package name: completion-categories, category-classifier, ...
  • What is the right point to hook into? completing-read function, completion-metadata-get?
  • Use the classifiers from embark: symbol, package and command-based
  • Maybe add some classifier based on command name if we feel like it? I am not pushing this one but it could be a simple and robust fallback for well-behaved commands. Since you have the experience using the classifier approach, you can tell if there are cases where the classifier approach breaks down.
  • Open question: How fine grained are the categories? (sufficiently, if not make our own according to Omar)
  • Open question: Can we define arbitrary categories if there is such a need? (yes, according to Omar)

Annotation package (Marginalia)

The purpose of this package is to add or refine annotations of completion metadata. Ideally the annotation functions should only rely on category information, keeping everything simple this way.

  • Package name: completion-annotations, enrich, annotate, annotate-completions, enrich-completions, ...
  • Move annotation functions from embark and consult-annotate-mode to this package
  • There should be the possibility to have multiple annotators per category to choose from (configurable)
    • Maybe we need two different annotations-string per annotation-function (one for the minibuffer and one for the occurs/completions buffer)
    • Alternatively select two annotation functions per category (one for the minibuffer and one for the occurs/completions buffer)
  • Open questions regarding consult:
    • Some of the annotations I am using right now do some formatting (e.g. moving text to the right). Can that be supported?
    • Are categories fine-grained enough? For example I am using a different annotator for variables (which shows the value) and symbols. Customization groups also have their own annotator since they use the group-documentation property. This is a bit of quirk. Faces have their own annotator in order to preview the face. These are the special cases I am having as of now.
    • What about functions which already provide "annotated" candidates? They must make sure that there is a do-nothing annotation function? This is a special case in consult and I think this can be sorted out.
@oantolin
Copy link
Owner

oantolin commented Dec 2, 2020

Thanks for opening this issue!

Category package

How about the name linneaus?

As for what to implement, I think only two of embark's classifiers are relevant (the others are not about minibuffer completion):

  • symbol: this is reported for variables, commands, and functions. I kept them together because I figured I could write an a single annotation function for all, and could make the actions applicable to all. But it would also be reasonable to separate this into command, function, variable, etc.

  • package, for package names.

We can also make it easy to override the reported category by command. Maybe if the symbol naming the command has a certain property, the package can report the command name as the category. That might be a sensible interface.

Annotation package

How about the name marginalia?

I think making it easy in the other package to have a command use its own name as the completion category, means that we can safely assume in this library that an alist of category names and annotation functions is configuration enough.

I'd put here all the consult-annotate-* functions and the function mentioned in embark-annotator-alist.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

Haha :) I thought about more descriptive names since they provide kind of a base functionality, therefore my more mundane boring names. Marginalia is very nice, we can directly use an old book as logo. Linneaus also makes sense I guess, but is not as literate.

Feel free btw to edit the original issue, just adding bullet points? Maybe that is easier for the discussion here. EDIT: But it is also easier to lose some context...

Category package

symbol: this is reported for variables, commands, and functions. I kept them together because I figured I could write an a single annotation function for all, and could make the actions applicable to all. But it would also be reasonable to separate this into command, function, variable, etc.

I stole this idea from you in consult and added consult-annotate-symbol which is very much like your function. However after that I added the consult-annotate-variable which also shows the value. The problem here is that the symbols don't distinguish the namespace and sometimes we want the variable sometimes we want the function. At this point the classifier somehow breaks down? Always use the function and for the commands which prefer the variable use the command-based classifier.

I think we also need the command category. Which detects if a symbol is a command and does not return symbol in that case but command in order to show keybindings.

package

Agree

We can also make it easy to override the reported category by command. Maybe if the symbol naming the command has a certain property, the package can report the command name as the category. That might be a sensible interface.

Ok. So I guess we allow using the command to specify an arbitrary category. It can either be command-category-name or some generic category like variable. And if the command is not in the category list, use the symbol classifier? Simple.

The question is what to do about the more complicated classifiers you have in embark? Somehow register them with linneaus additional to the basic linneaus symbol/package/command classifiers? But keep them in Embark? This makes sense if not generally useful!

Annotation package:

I think making it easy in the other package to have a command use its own name as the completion category, means that we can safely assume in this library that an alist of category names and annotation functions is configuration enough.

This is a good idea, I also thought about that, maybe with a prefix/namespace "command-category-..." or something shorter? We could also namespace our categories under "linneaus-..." but then we introduce an implicit dependency. Therefore I would prefer a neutral namespace.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

@oantolin One other thing I wonder about - do we really need two packages here? My original assumption was that more of the Embark classifiers are general purpose, if only symbol, package and command-based classifiers are relevant for annotations it might be sufficient to just create a single marginalia package which contains all of what we discussed here. Since you will also require both packages for embark, that would also work for you and we can start a bit simpler without having to think too much what to put where. Independent of what comes out here, I considered factoring out the consult-annotate-mode from the consult package.

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

One other thing I wonder about - do we really need two packages here?

I proposed a single package on reddit, "(And these two libraries are probably closely related enough that they could actually be a single package.)".

After that you started this issue and started talking about two packages, so I thought you were saying no to my proposal. But if you're on board now, I really think a single package for both classification and annotations makes sense. Lets call it marginalia since you liked that name.

As for what to call the one-off category corresponding to single command, I was thinking no prefix at all, just the symbol naming the command. I doubt that would clash with any of the "real" categories.

My original assumption was that more of the Embark classifiers are general purpose, if only symbol, package and command-based classifiers are relevant for annotations [...]

It's not that the other Embark classifiers aren't general purpose, it's that they aren't for minibuffer completion. As I explained on reddit, there are other contexts in which Embark provides actions besides minibuffer completion, and the classifiers are for those.

I would want to put into this new package we are discussing all of the minibuffer completion categories and all the annotation functions we need. Ideally after this package is ready:

  • Embark can use category metadata exclusively for the minibuffer (it would still need its other classifiers for other contexts in which it provides actions or export functionality, but that's OK).

  • All of the functions Embark and Consult have for specific types of annotations can be removed from those packages.

@protesilaos
Copy link

But if you're on board now, I really think a single package for both classification and annotations makes sense. Lets call it marginalia since you liked that name.

Just some user feedback here: marginalia feels right.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

I agree with both you @protesilaos and @oantolin! Only the category prefix - there I am a bit worried and would prefer to be on the safer side (maybe also a marginalia- prefix for our categories). Another possibility I thought about: Can we add metadata fields? Because then we use the category special and use another field command-name. But that would complicate the lookup since we have to check two levels. But what we chose is in the end irrelevant if I consider purely the consult side. Consult will only provide commands. As of now consult-annotate-mode and the rest of consult are completely independent (consult commands provide categories where appropriate but that's it). So I guess it is more important to chose the categories so that it works for embark.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

@oantolin I started a first prototype here by simple extracting the code from consult (more or less verbatim) - https://github.com/minad/marginalia. I invited you there, added you as author/maintainer, but please let me know if you want to do things in another way. I am fine if the repository is managed under your account instead and I am fine if anything is changed, so please let me know if you rather would like to do things differently. But I had to play around with this a bit :) I implemented annotations based on categories but didn't add more classifiers yet from embark (only doing the command-name based thing right now). For me the approach works nicely. The code using categories is better than the consult-annotate-version and there is no degradation at all in comparison to how consult-annotate-mode works - only downside as of now is that there is still some selectrum specific code, but @clemera seems to be sorting this out right now by improving selectrum. Then this can probably go away.

@protesilaos Sorry for messing again with the faces! If marginalia works out we have to revise the consult faces in modus again.

@protesilaos
Copy link

Sorry for messing again with the faces! If marginalia works out we have to revise the consult faces in modus again.

No worries! I am prepared to review existing faces or add new ones. I am tracking the development process and will receive all the updates, but do not hesitate to ping/contact me in case you want to bring something to my attention.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

@protesilaos Thank you, that's good to hear!

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

Thanks @minad for starting the marginalia repo! I'll go take a look. I should warn you that I'm pretty busy these days, and don't actually have much time for working on Emacs packages that much, so I might be slow to respond at times.

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

There are a few things I observed and which must be done before this can replace what you have in Embark:

  1. Right now the annotate functions I have are focused on nice display in the selectrum minibuffer. They use for example the align-to property and custom faces. This is a point where we have to check which expectations you have and how well those work for you in Embark.
  2. The symbol classifier from embark should be added in some way. RIght now the command "classifier" is hard coded. Maybe there should be a list of classifiers. But maybe it is also overkill and one can just hardcode the command check and if there is no associated category to a command, check for the symbol and if that's not the case fallback on the original category metadata. There is also a package classifier in Embark, but my suggestion would be to simple add the package functions we are aware of to the command-category-alist - I feel that packages are not special enough to justify a special treatment here. We can also do it differently if you find this important.
  3. Maybe if category metadata is made available by the underlying metadata function, this should be preferred? I am not sure. Do we want to overwrite or not? Probably we want to overwrite since the marginalia categories should be more precise or at least we can aim for that.
  4. I copied over the command<->category association list from consult mode mostly, many of those can simply go away if we use the symbol classifier from Embark.
  5. I am overwriting the completion-metadata-get function - it seems to work. But needs checking as discussed before.

Maybe there are more things - but I have by no means a detailed picture of your embark codebase in mind. So there are probably things I missed and things you would do differently. I used the consult-annotate-mode as a starting point due to familiarity with it, since I just wrote it days ago.

So if you have time to look at things, I guess these are the interesting points.

@oantolin
Copy link
Owner

oantolin commented Dec 3, 2020

  1. This is the first thing I'll check: I'll activate marginalia-mode and use embark using only embark-annotation-function-metadatum for annotation and just see how it looks.

  2. Adding the symbol classifier is the second thing I'll do. :) I think I do want a package category. I dislike mainting lists of commands, even if short. I would want at least describe-package, package-install, package-delete, package-reinstall to report package as the category. If we can write the package classifier in a way that, say, commands from paradox or package commands from consult report the category as package, so much the better. All in all, I think a list of classifiers is the way to go. (Of course, I would say that: it's what I decide on for embark, so no surprise I'm also advocating for it here).

  3. My gut says you are right: we should overwrite because marginalia will be more precise. Now, that ould have drawbacks if Emacs relies on commands reporting a certain category. As far as I know, it doesn't. My insitinct is we should override the built-in categories if we want too, and revisit that decision if we notice some built-ins misbehaving.

  4. Sure.

  5. This methods works for embark. I had thought maybe we should just modify minibuffer-completion-table inside a minibuffer-setup-hook. What do you think about that method?

Embark has a few more annotations I'll add:

  • For files it has size and last modification time (I actually use this ocasionally from find-file: I normally use grid view, but sometimes will quickly switch to list view to make sure I'm opening the file I got last Tuesday file insted of the one I recieved on Thursday with an almost identical name).

  • For buffers it has modification status, major-mode and associated file if there is one. I don't really use this much, but (1) maybe one of Embark other five or so users does, and (2) why not?

@minad
Copy link
Contributor Author

minad commented Dec 3, 2020

  1. I am also fine with package and a list is also good. For now it only felt like overkill ;)

  2. I wonder if anything crucial relied on it given how often category information is not present. So we could just overwrite. But I just thought about one thing - for consult I am actually reporting correct category information. So just overwriting won't work very well. Maybe use only the command classifier for overwriting. The more fuzzy classifiers should not overwrite. We could add some priorities to the list. Priority 0 is the category of the original metadata if present. Priority 1 if we want to overwrite, e.g. for the command list, there I would want that. And then we could have negative priorities. We could also try the classifiers in order and just add the "category-category" classifier in the middle of the list.

  3. I cannot really tell which function is better. The completion-get-metadata just seemed to me as the most obvious one. But given that it is trivial it could be that some consumers access the metadata, but not through this completion-get-metadata function. Regarding the hook - I don't think there is any advantage in overwriting in the setup hook since we also have recursive minibuffers. (Edit: Thinking about it again. If we use buffer-local variables one can actually overwrite at least variables per recursive minibuffer, but can something like this be done for advices? buffer-local advices, I don't think so?)

  4. (More annotations) - Right, I forgot about those since I don't have them yet in consult. These should be added for sure!

minad referenced this issue in minad/marginalia Dec 4, 2020
The new variable marginalia-classifiers will contain functions that
are run one after another to attempt to determine the category
metadatum. The existing logic for determining the category has been
moved into marginalia-classify-by-command-name.
@minad
Copy link
Contributor Author

minad commented Dec 5, 2020

I guess this can be closed, I moved the remaining points to the marginalia issue tracker.

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

3 participants