-
Notifications
You must be signed in to change notification settings - Fork 54
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
refactor: crm -> cr #496
refactor: crm -> cr #496
Conversation
Thank you for working on this. If it turns out that the approach is viable, it would be great if you could remove the reliance on the https://github.com/bdarcus/citar/blob/9da28efdaa024a65165a24189306943acb4bfc4f/citar.el#L875-L882 |
Since Embark only supports acting on multiple candidates via |
The main question I have is how one would do something like this with only single selections and actions: https://github.com/bdarcus/citar/wiki/Example-functions#search-contents-of-pdfs |
Good argument! Commands like this are indeed (almost) impossible to implement without embark-multitarget-actions. One could hack something since nothing is impossible in Emacs ;) But I think this is reasonable justification to keep embark-multitarget-actions. Nevertheless I see some value in exploring a CR-only solution in Citar or at least minimize CRM usage. |
In this case, I would expect multiple-item selection because it makes sense. In the cases such as |
The idea is one would use It would keep this code simpler. Otherwise there's not much point in the change, since it just adds complexity without clear benefit. This config will fix the keybinding UI. |
Does this mean you're open to adding If so, I would refactor the changes in #491, push those here, and then close that PR. |
Hmm, to be honest I would avoid Consult as a dependency. Which Consult feature would you like to have in Citar? First, you can use the group-function. It seems that the problem is the completion category such that you can combine multiple categories like file and url? My proposal would be that you use the consult-multi category. This category is understood by Marginalia and Embark. But if you use this category no Consult dependency is implied! (In a second step I can change the name of consult-multi to multi since it is not consult specific anymore and change it in marginalia, embark and consult.) If I recall correctly there has been a prior discussion of this approach where I already proposed this? |
Oh I see! Yes, I made an assumption based on the naming of the |
We have discussed consult-multi-like functionality before, but I don't recall you proposing this specific approach before. I actually didn't realize the category was/could be independent of consult! Is that a recent change?
+1
That would likely make sense. Just to be clear, though, I'm not 100% sure yet I'll merge this. But I guess you won't solve #491 as you're hoping without it, and even if we don't change everything to CR, the "open" actions probably make sense. |
Added the consult-multi change to #491. Seems to work, but I haven't put it through much testing yet. |
4af4730
to
8baee02
Compare
a86e5e5
to
d742ca4
Compare
Just tried it, including |
That'll be addressed in #491, which also adds notes and links to the results. |
We should also rename |
That was already done. Do you really see otherwise? |
citar-select-files -> citar-select-file citar-open-library-files -> citar-open-library-file CRM -> CR: - citar-open-notes - citar-open-entry - citar-open-link This replaces completing-read-multiple with completing-read. The intention here is to favor using 'embark-act-all' for cases where one wants to act on multiple candidates.
I merged this, so we'll have to rebase #491 on main before resolving the remaining issues there. The main post summarizes my conclusions on this, which is that most commands will work fine with CR + The main one, that could benefit from experimentation, is |
All good now. I think |
This is incomplete, but is a branch to test the ideas in #495, which you should do in conjunction with 'embark-act-all'. The devil will be in the details here.
Notes
There are two categories of commands in citar:
Single action
That is, which take a single reference candidate, and do something with it. With
embark-act-all
, the command would be run once per reference.All of the "open" commands fit here. This and #491 updates these to use CR instead of CRM.
Multi action
That is, which ideally take a list of reference candidates and do something with them as one action. With
embark-act-all
, the command would be run once, on the list of references.Many of the "insert" (notably
citar-insert-citation
) and "copy" (citar-copy-reference
) commands fit in this category, as does the search-pdf-contents example, which I'd like to merge at some point.I do think it'd be nice to keep
embark-multitarget-actions
as an option for these sorts of commands, to replace with the use of CRM.