minad / marginalia

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

In Emacs 28, project-find-regexp can be configured to use the minibuffer, if so, detect and report grep category #40

Closed oantolin closed 3 years ago

oantolin commented 3 years ago

Over at oantolin/embark#47, @jixiuf mentioned that in Emacs 28 you will be able to configure project-find-regexp to use the minibuffer to choose a search result. I understand the default will not use the minibuffer. It would be good if Marginalia can detect when project-find-regexp is using the minibuffer in this way, and have the category grep reported in that case.

As usual, my Emacs snapshot lags behind master and I don't actually have that feature, so I'm writing this issue so we don't forget.

In the meantime, I told @jxiuf to put (project-find-regexp . grep) in marginalia-command-categories.

minad commented 3 years ago

Another option is to use consult-grep instead of project-find-regexp. consult-grep supports consult-grep-directory-hook which can be customized such that the project root directory is used. By default the default-directory is used. This approach is compatible with both project.el and projectile.el.

minad commented 3 years ago

I don't know what the best way is to support project-find-regexp - probably the best way is to use marginalia-command-categories. Users can simply to this themselves. I don't think we can use prompt detection: https://github.com/emacs-mirror/emacs/blob/0326cddc7bb2a90924af200057b4e2ef263924c8/lisp/progmodes/xref.el#L1029

oantolin commented 3 years ago

Maybe the best thing to do is to only document this, then. Tell people that if they configure Emacs so that project-rind-regexp uses the minibuffer, they might also want to configure Marginalia to report grep as the category (specially if they use Embark).

minad commented 3 years ago

Maybe the best thing to do is to only document this, then. Tell people that if they configure Emacs so that project-rind-regexp uses the minibuffer, they might also want to configure Marginalia to report grep as the category (specially if they use Embark).

Yes, exactly we should document this very clearly - probably both in the marginalia readme and in the embark readme. I think it is better if we do not maintain long lists of commands. Commands which can be supported using catch-all prompt classifiers or other classifiers - these we should obviously support here!

oantolin commented 3 years ago

Ah, browsing the source code for the future Emacs 28 I learned that this xref--show-defs-minibuffer function will be renamed xref-show-definitions-completing-read and reports xref-location as the category metadatum! So we can close this issue without doing anything, just add support for xref-location in Embark, and also, possibly change Consult so it reports xref-location instead of grep.

minad commented 3 years ago

@oantolin That's good. But I am not sure if we can report xref-location for consult-grep since we are not using xref locations as of now, but items '(string file line). But maybe this does not matter as long as the string has the grep output format.

Alternatively i could change the location to use xref-items/xref-file-locations. For now I've not seen an advantage in doing so, I've rather seen disadvantages due to the complexity/generality of xref (I think I am not a huge fan of xref and neither of oop and clos/eieio).

oantolin commented 3 years ago

From the screenshot in oantolin/embark#47 I gather that these grep results are one possible form of an xref-location, so it wouldn't be a lie to report that category. I think xref-locations can also point to a buffer instead of a file, but it's ok to not take advantage of that possibility.

oantolin commented 3 years ago

But I am not sure if we can report xref-location for consult-grep since we are not using xref locations as of now, but items '(string file line).

That's a fiction internal to Consult, the world outside sees only the "file:line:string" strings. And this xref-show-definitions-completing-read function seems to think it is OK to report xref-location for strings that look like that.

minad commented 3 years ago

That's a fiction internal to Consult, the world outside sees only the "file:line:string" strings. And this xref-show-definitions-completing-read function seems to think it is OK to report xref-location for strings that look like that.

That's good to know. I've never really been sure if there is not some place where completing-read secretly takes a look at the candidate alist values ;)

minad commented 3 years ago

Do you have a reference, which shows the allowed formats of xref-location? One problem is for example the path - see https://github.com/minad/consult/commit/f8df1b14a3ce752eb5d9673dee93301d11f770b7! We report paths relative to the grep directory and I am certain that this is not compatible with xref-locations. Alternatively I could use absolute paths but not display them via 'display propertize?

oantolin commented 3 years ago

That's a fiction internal to Consult, the world outside sees only the "file:line:string" strings. And this xref-show-definitions-completing-read function seems to think it is OK to report xref-location for strings that look like that.

That's good to know. I've never really been sure if there is not some place where completing-read secretly takes a look at the candidate alist values ;)

I'm pretty sure there isn't. But even if there were, Marginalia annotators and Embark exporters only recieve the candidate as a string.

oantolin commented 3 years ago

Do you have a reference, which shows the allowed formats of xref-location? One problem is for example the path - see minad/consult@f8df1b1! We report paths relative to the grep directory and I am certain that this is not compatible with xref-locations. Alternatively I could use absolute paths but not display them via 'display propertize?

Sorry, no reference, but @jixiuf's screenshot in oantolin/embark#47 shows relative paths.

minad commented 3 years ago

Sorry, no reference, but @jixiuf's screenshot in oantolin/embark#47 shows relative paths.

Ok, but this does not sound particularly robust, every buffer can have its own default-directory. I can still change this to xref-location though and we will see.

oantolin commented 3 years ago

OK, I'll update Embark to use xref-location and, as you say we will see if there are any problems. I'll close this too.