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

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

Closed
oantolin opened this issue Dec 29, 2020 · 14 comments

Comments

@oantolin
Copy link
Collaborator

oantolin commented Dec 29, 2020

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
Copy link
Owner

minad commented Dec 29, 2020

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
Copy link
Owner

minad commented Dec 29, 2020

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
Copy link
Collaborator Author

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
Copy link
Owner

minad commented Dec 29, 2020

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
Copy link
Collaborator Author

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
Copy link
Owner

minad commented Dec 29, 2020

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

minad commented Dec 29, 2020

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
Copy link
Owner

minad commented Dec 29, 2020

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?

@oantolin
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

minad commented Dec 29, 2020

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.

minad added a commit to minad/consult that referenced this issue Dec 29, 2020
@oantolin
Copy link
Collaborator Author

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.

oantolin added a commit to oantolin/embark that referenced this issue Dec 29, 2020
We are using this new category name because it seems that is what xref
Emacs 28 will use. See the discussion in minad/marginalia#40 and
commit minad/consult@9fd1efc
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

2 participants