-
-
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
Add consult-xref. #216
Add consult-xref. #216
Conversation
Add functions to let consult work with xref with preview.
Thanks. This has been considered before in #189 and #46. I am not sure if is justified to have this function as part of Consult given that Emacs itself will offer this function soon and given the xref package on ELPA. The preview function you proposed is not sufficiently robust since xref can open files which may not be desired. There is consult--jump-preview and consult--grep which implements a more robust preview. Consult xref preview should at least take a similar approach. I will add some more comments in the code. Note: the xref branch is only a feature branch/experiment, not something in a useable state. |
(funcall open) | ||
(funcall preview nil t)) | ||
(t (let ((cand-xref (cdr (assoc cand collection)))) | ||
(xref-pop-to-location cand-xref nil))))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of pop-to-location, preview and open should be used for a robust preview.
(xref-pop-to-location cand-xref nil))))))) | ||
|
||
(defun setup-consult-xref () | ||
"Set up consult with xref. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup function should be removed, since the user can use setq directly.
:history nil | ||
:require-match t | ||
:predicate nil | ||
:default def |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Predicate nil is unnecessary.
(def (caar collection))) | ||
(cdr (assoc (consult--read collection | ||
:prompt "xref results: " | ||
:history nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
History nil is unnecessary or should be replaced with some history variable.
(require 'xref) | ||
|
||
(defun xref-show-definitions-consult (fetcher alist) | ||
"Let the user choose the target definition with completion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name should be consult-xref-... to follow the package convention.
;; FIXME: Groups are not always file names, but they often | ||
;; are. At least this shouldn't make the other kinds of | ||
;; groups look worse. | ||
(let ((common-prefix (try-completion "" xref-alist))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to determine the prefix using try-completion? Why is it necessary to mark the prefix with a special face?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misread the substring call below, the prefix isn't highlighted, but chopped off. The rest of of the file name is given a special face.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I still think it would be better to not use try-completion to find the common prefix.
If I understand correctly, the only difference between this function and the one in xref is the previews, but many of the search results will be in unopened files and consult usually doesn't have previews that require reading a file from disk. So, unless that policy changes, the previews here would have to be only for file that randomly happen to be open already, I guess. Maybe file-opening previews could be admitted in Consult but off by default? Also, is just having previews enough of a distinction for a command in Consult? I don't think there are currently examples of commands where the preview is the only distinguishing feature, are there? But also, is there anything wrong with that? Maybe previews are already worthwhile. |
@oantolin consult-grep and consult-recent-file have file preview which is written with some safety precautions. This should be added to the preview here. But I am unsure if it is sufficient as a distinction to justify a separate command. At some point I considered implementing preview in the style of Marginalia via the completion category for existing commands. |
When I merge consult-xref, I think I should also restructure consult a little bit such that we do not stuff more and more into the main consult.el file with the respective dependencies. This PR already proposes to create a consult-xref file.
Such a restructuring would come without downsides for the user and ensure that only the necessary components are loaded. See also #89. |
f939f16
to
1ccec4f
Compare
The consult-xref feature request has been noted on the wishlist #6. I close this PR since it is not mergeable in this form. |
I added a |
Thanks a lot for the update. I'll check out the new function. |
Add functions to let consult work with xref with preview. Half of it is basically copied from the
xref-show-definitions-completing-read
function in xref.el and added a preview function for consult.The current xref branch looks a bit outdated and hopefully this could help with it.