Drop xref-location support

[minad]

See the discussion in #144. The xref-location completion category is not robust since it works with incomplete strings. I just added consult-xref which can be used as replacement for the xref show functions with preview support. My proposal is to drop the special xref to grep exporter from Embark. xref-locations are too generic. You can even define your own.

1. Drop xref-location exporter from Embark
2. Use custom consult-grep category instead of xref-location. The candidates are always in grep format, export to grep-mode is guaranteed to work. The grep-mode exporter should be moved from embark.el to embark-consult.el.
3. Maybe define a custom consult-xref location which attaches the xref-location object via a text property. This would allow to write an Embark to Grep exporter by filtering for file or (file backed) buffer locations and formatting with abbreviated/relative file paths. But this step is not very important since one can always use consult-grep or consult-ripgrep instead. Furthermore you would probably dislike the incomplete export.

[minad]

Another option: Actually Embark should export xref-location completion candidates to an xref buffer with the special xref buffer mode. This would make more sense than abusing the grep-mode buffer for this. The xref buffer is fairly limited, but this is a necessary outcome of the generic design of xref.

EDIT: But thinking again - this won't work, since you cannot obtain the xref locations from the xref-location completion candidates. I think my point still stands, xref-location completion candidates do not allow for a useful exporter. Embark snapshot is all we can do reliably. The export from consult-xref to the xref mode buffer would work however, assuming that Consult attaches the xref-location objects to the candidate strings of custom consult-xref category.

[oantolin]

OK, I merged your PR. The only thing that worried me about the PR is that with the ugly hack I wrote in #144, @Gaeric did get a working grep export for the (setq xref-show-xrefs-function #'xref-show-definitions-completing-read) case. But that's OK, @Gaeric can just keep that ugly hack, install embark-consult if necessary, and add this configuration:

(setf (alist-get 'xref-location embark-exporters-alist) #'embark-consult-export-occur))

[minad]

Yes, I am aware of that. Given that it is an ugly hack and and since it is possible to override, it is okay like this.
For xref-find-references I suspect that nobody will miss the grep-mode export. I hope we find a better solution for project-find-regexp. We could make the upstream developers aware of your Embark exporter concept and maybe they would be interested in adapting xref such that it becomes "exporter friendly", by adding the necessary metadata to the xref completion candidates. I think @dgutov is maintaining xref?

[oantolin]

Yeah, it would be nice if the xref-location objects were added to the completion candidates in a text property. Then we could have a proper Embark export to an xref mode buffer, at least. We could do that right now with consult-xref, but I'm not sure it's worth it.

[minad]

We could do that right now with consult-xref, but I'm not sure it's worth it.

Yes, this is what I proposed above (3.). I am also unsure and therefore I left it as is.

[dgutov]

I think @dgutov is maintaining xref?

More or less.

I'm not sure I understand the context of this discussion, but perhaps you should define your own xref-show-xrefs-function alternative. You can copy xref-show-definitions-completing-read and change it to your preference. This way you will have direct access to the xref objects.

[minad]

@dgutov Thank you for chiming in. Yes, if we want to modify the candidates to our liking we can always copy the upstream functions and modify it.

However the concept of the Embark package is that it allows adding actions to existing commands via the completion category. Our Marginalia package goes in the same direction and adds annotations to existing commands, even if the existing command does not provide them. While all this requires a tiny bit of integration code and cooperation of the completion system, this allows a lot of code reuse. In most cases all the required information is already present in the candidate strings alone. However not in the case of the Emacs 28 xref-show-definitions-completing-read function, which truncates and groups the candidates for a nicer display. A possible solution which would allow Embark to reuse the xref-show-definitions-completion-read function and which would allow export of the candidates to a grep-mode buffer would be to attach the xref-location as a property to the candidate strings. With this one could run project-find-regexp -> embark-export and obtain a grep-mode buffer. This buffer can then even be turned into an editable wgrep buffer.

I am unsure if this is a compelling enough reason to ask for a change in the xref-show-definitions-completing-read function. I guess in this particular case it does not hurt too much to replicate the function as you proposed, since not much code is involved.

[dgutov]

allow export of the candidates to a grep-mode buffer would be to attach the xref-location as a property to the candidate strings

Have you looked into reusing the "original" Xref buffers this way, then?

They have a slightly different format than grep-mode works with, but they do store location values in text properties.

[minad]

Have you looked into reusing the "original" Xref buffers this way, then?

Yes, we have considered this. However if we would want to use the original xref buffers the problem remains, there is still no possibility to convert from the completing-read interface via embark-export to the xref buffer, since the completion candidates are missing the xref-item/xref-location. The idea is that you perform the search, then narrow down via completing-read and then export. You can always go directly to the buffer by setting the corresponding xref-show-xrefs-functions, but then you don't have the possibility to perform the intermediate narrowing.

[dgutov]

there is still no possibility to convert from the completing-read interface via embark-export to the xref buffer

But which completing-read call is that? If you change xref-show-xrefs-function, and xref-show-definitions-function, the only completing-read calls will be your own. And you can stash whatever data you want in advance for those.

Well, there's also a completing-read call inside xref--read-identifier, but you probably weren't referring to that.

[minad]

I am referring to the completing-read call in xref-show-definitions-completing-read.

[dgutov]

If xref-show-xrefs-function and xref-show-definitions-function are set to your own values, xref-show-definitions-completing-read will never be used.

[Gaeric]

there is still no possibility to convert from the completing-read interface via embark-export to the xref buffer

But which completing-read call is that? If you change xref-show-xrefs-function, and xref-show-definitions-function, the only completing-read calls will be your own. And you can stash whatever data you want in advance for those.

Well, there's also a completing-read call inside xref--read-identifier, but you probably weren't referring to that.

Let me explain the usage

(setq xref-show-definitions-function 'xref-show-definitions-completing-read)
(setq xref-show-xrefs-function 'xref-show-definitions-completing-read)

Use (xref-find-references) or (project-find-regexp)
The xref candidates will be displayed in the minibuffer, we can type to filter the xref candidates.

We want to convert the xref candidates to a buffer,then change those candidates.

There is a command (xref-query-replace-in-results) for *xref* buffer. If we can transfrom the xref candidates to *xref* buffer, that would be great. But when we use xref-show-definitions-completing-read, we cannot generate *xref* buffer from the candidates in the minibuffer, because it converts xref-location to grep-like information, so we missed xref-location.

[dgutov]

I understand what you're asking for, just don't understand why.

(setq xref-show-definitions-function 'xref-show-definitions-completing-read)
(setq xref-show-xrefs-function 'xref-show-definitions-completing-read)

Do you actually have to do this?

[Gaeric]

I understand what you're asking for, just don't understand why.

(setq xref-show-definitions-function 'xref-show-definitions-completing-read)
(setq xref-show-xrefs-function 'xref-show-definitions-completing-read)

Do you actually have to do this?

Yes, I use project-find-regexp to search for some symbols, filter them by file path, export them to buffer and replace.

[dgutov]

Is creating an embark-specific xref-show-xrefs-function inconvenient? Is that the problem?

[oantolin]

Is creating an embark-specific xref-show-xrefs-function inconvenient? Is that the problem?

No, it wouldn't be inconvenient. I think the point was just that adding the xref objects as text properties in xref-show-definitions-completing-read would be a very small and simple change to make in the xref package, and that would allow Embark users to use xref-show-definitions-completing-read directly instead of writing their own xref-show-xrefs-function. And the embark package could add the necessary code to allow people to get xref buffers from commands that call xref-show-xrefs-function.

But it's not really a big deal, if you don't want to modify xref-show-definitions-completing-read and your advice is, "if xref-show-definitions-completing-read doesn't do exactly what you need, just write your own xref-show-xrefs-function based on it", that's fine. The consult-xref function could fulfill that role and we could add the embark integration to the embark-consult package.

[dgutov]

If you can get by well enough with some existing code, good. Because ideally you would use some extension point which was anticipate by the package maintainer rather than plug into something that might break in the future because it's unused in the core.

My minor concern with adding text properties is that it makes debugging harder (printed strings with properties are harder to read). But if someone wants to contribute it in a patch, I'll make sure to review it.

There can be a more generic approach as well. embark acts on the value of minibuffer-completion-table, right? We could add a protocol which will let you query the completion table for the "original" values corresponding to each completion, which different package could share. A patch for xref-show-definitions-completing-read could look like this (better action name welcome):

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 18fdd963fb..49e895a9d0 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1068,6 +1068,8 @@ xref-show-definitions-completing-read
                            (cond
                             ((eq action 'metadata)
                              '(metadata . ((category . xref-location))))
+                            ((eq action 'get-original)
+                             (assoc-default string collection))
                             (t
                              (complete-with-action action collection string pred)))))
                         (def (caar collection)))

Then you could use it like this:

(funcall minibuffer-completion-table "src/frame.c:(defvar tab-bar-mode)" nil 'get-original)

[minad]

There can be a more generic approach as well. embark acts on the value of minibuffer-completion-table, right? We could add a protocol which will let you query the completion table for the "original" values corresponding to each completion, which different package could share.

Having an extra protocol for Embark or similar use cases would be great! However ideally it would be made such that no changes to existing completion tables is necessary. By modifying completion-with-action one would achieve that. Completion tables can have different internal representations, e.g., hashtable and alist etc. Maybe the protocol should be defined such that for alists and hashes the original (key . value) pair is returned. For other completion tables (like lists) only the original element is returned. But before committing on a design it should be tried out first in a prototype.

There is one other change which might be good - completing-read strips properties of returned strings. I propose a change - if a string is returned which is an element of the collection it should be identical to the one present in the collection. If require-match=t is used this would ensure that you always get an element from the collection. This would be very helpful for api users. Has this been discussed before? I may create a proposal on the emacs dev mailing list for this.

[dgutov]

However ideally it would be made such that no changes to existing completion tables is necessary.

No changes would be required where the completion table already contains "real" elements, i.e. ones that you can use directly.

By modifying completion-with-action one would achieve that.

Yes, this could work too, as the default implementation of sorts. But I think it should return value when queried with key, and not a pair. After all, you should have key already available.

I propose a change - if a string is returned which is an element of the collection it should be identical to the one present in the collection.

I would like that, although there must be some reason for the existing behavior (and if it was explained in some of the previous discussions touching this subject, I don't remember it). You're welcome to M-x report-emacs-bug about this or start a thread on emacs-devel. But note that as long as you support older Emacs versions, you'll have to work around this behavior somehow anyway.

[minad]

Yes, this could work too, as the default implementation of sorts. But I think it should return value when queried with key, and not a pair. After all, you should have key already available.

Yes, that's right. So probably it would be sufficient to only return the value for alists and hashes.

But note that as long as you support older Emacs versions, you'll have to work around this behavior somehow anyway.

Yes, but the sooner it would be changed, the better. I think it is acceptable practice to ensure that packages support the two latest versions or the Emacs version present on some widespread LTS Linux distributions.

We have more possible change proposals for completing-read, we recently introduced the x-group-function extension in Consult, see https://github.com/minad/consult/wiki/Grouping-support. So if I find time for this I may also create a proposal for that.