-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Readme Intro #1
Comments
Good idea. Note that I haven't done sorting via the completion metadata yet. I have to find out how the metadata callback works. If you have time it would be great if you go a bit over the stuff I added so far. There are a few places marked with TODOs, which should be resolved but I haven't done yet or have to find the right solution first. |
Added a note in 8d0a860. I will polish the readme more and more when this thing becomes more mature. |
The manual describes the callback at (let* ((alist '(("GNU" . "GNU is not Unix")
("Emacs" . "Eight Megabytes And Constantly Swapping")))
(annotf (lambda (str)
(format " (%s)" (cdr (assoc str alist))))))
(completing-read "Candidates: "
(lambda (str pred action)
(if (eq action 'metadata)
`(metadata
(annotation-function . ,annotf)
(cycle-sort-function . identity)
(display-sort-function . identity))
(complete-with-action action alist str pred))))) |
I will look through the code on the weekend |
Thank you for the example. I will play around a bit and see how that works. One major downside of the standard api I observed is that completing-read does not yet preserve properties (only in 28). This complicates everything unnecessarily. And I think we want to support at least Emacs 26.
Looking forward to it! |
Given that Emacs will eventually gain support via |
Here is a corresponding issue for selectrum. |
Yes, consult could detect if there is either emacs 28 or a completion framwork and refuse to work otherwise. Another thing I wonder about are annotations. The Emacs completion system only supports suffix annotations and this is too weak for many things. What do you propose in that case? Simply use the selectrum property keys? For now I introduce "defcustom consult-property-prefix" etc, but seems to be more like a fig leaf... |
I just discovered that we could add prefixes using the (completing-read "Candidates: "
`(,(propertize "GNU" 'display "some-prefix: GNU")
,(propertize "Emacs" 'display "otherprefix: Emacs"))) |
Oh, the other frameworks also get the match highlighting wrong but at least the selection works as usual. |
I don't think there is a way to highlight the matches in this case...(I mean highlighting the parts matched by your input, the selection issue can get fixed). |
Hmm, this seems to be some kind of abuse of the display attribute? I wouldn't try to bend the api too much, the same holds regarding returning propertized text. I only think that's ok since you say that in emacs 28 this is proper api use. Otherwise I would rather fallback to the selectrum api. This is cleaner than bending an api, we don't really own. I think using these prefix selectrum properties is ok, since this degrades gracefully. Things don't look so nice but will work at least. |
Yes, the better way would be to propose adding the prefix feature to the metadata in Emacs. As you said there is no problem using the selectrum properties as long this isn't supported. |
I don't have time for an extensive review but looks all pretty good to me, I don't have much to add for now. For a quicker way for computing line numbers see here. From a user perspective I'm not a big fan of making the preview behaviour a default and think it would be good to have a single setting to disable/enable it for all but that is just me ;) |
Thank you for your help! I feel pretty confident right now regarding my elisp skills. Your help and excessively hacking a few days helped a lot ;) There are only very few TODOs in the consult.el. If you have time at some point, it would be great if you just read over those and give your opinion (I also made github issues for some of the TODOs).
Yes, I've seen that before. But my quick tests didn't instill confidence. This is not a robust approach. I rather go with some algorithmic changes. E.g. consult--outline-candidates should now be O(n).
I generally agree that packages should stay out of the way. But in this case I enable everything which I consider in a "working state" and which I would like to use myself. This way the user gets the full power right away. And you have to bind the commands explicitly yourself after all, so my criterion regarding going out of the way still holds :) |
Sure, it's a matter of taste and I can disable those manually but as I said it would be nice to be able to disable this by one general variable because every time you add a new command users (I ;)) would need to update the config. One more thing regarding the review, in |
Ok, I misread you! Maybe it makes sense to add some override variable? But I am not sure if it is worth the effort. I prefer to have fine grained settings here since I may want to disable some previews. For example the buffer preview can be very annoying if you are not accustomed to it. But the consult-line preview is essential I think in order to allow jumping between search and buffer for recursive editing. So I would want to enable that.
Ok, this is good to know. I can probably use this too for other places where I am using a stack which I push/pop in setup/exit. |
Yes :) Thanks for implementing this! |
Turns out, the implementation is also much better than before! :) |
Maybe helm and ivy should also be mentioned so people don't get the impression this wouldn't work for them. AFAIK helm fully supports the API, ivy (last time I checked) also supports it but doesn't support annotations or sorting defined via completion metadata.
The text was updated successfully, but these errors were encountered: