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

refactor: crm -> cr #496

Merged
merged 1 commit into from
Dec 27, 2021
Merged

refactor: crm -> cr #496

merged 1 commit into from
Dec 27, 2021

Conversation

bdarcus
Copy link
Contributor

@bdarcus bdarcus commented Dec 19, 2021

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.

@minad
Copy link
Contributor

minad commented Dec 19, 2021

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 embark-multitarget-actions. Then we can reconsider this addition to Embark. It would be great if we can maintain a simpler injection protocol where single candidates are the elementary unit of operation.

https://github.com/bdarcus/citar/blob/9da28efdaa024a65165a24189306943acb4bfc4f/citar.el#L875-L882

@minad
Copy link
Contributor

minad commented Dec 19, 2021

Since Embark only supports acting on multiple candidates via embark-act-all the multi selection feature is not as convenient as with crm. The user has to follow narrow down to all candidates. However I expect that we can improve this by adding an embark-select command which can be used to first select/collect candidates and then act only on the selected ones. So this regression in behavior should not hold you back from trying the CR-only approach.

@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 19, 2021

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

@minad
Copy link
Contributor

minad commented Dec 19, 2021

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.

@rudolf-adamkovic
Copy link

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

In this case, I would expect multiple-item selection because it makes sense. In the cases such as citar-open, I would expect single-item selection by default and multi-item selection with e.g. C-u (and an option to enable it by default).

@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 19, 2021

In the cases such as citar-open, I would expect single-item selection by default and multi-item selection with e.g. C-u (and an option to enable it by default).

The idea is one would use embark-act-all for that. Currently that solution doesn't allow marking specific candidates, but that is planned.

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.

@localauthor
Copy link
Contributor

localauthor commented Dec 20, 2021

Beyond the obvious, an advantage here is we could then use consult--multi to address how to mix different candidates category in "open" commands (see #491).

Does this mean you're open to adding consult as a dependency?

If so, I would refactor the changes in #491, push those here, and then close that PR.

@minad
Copy link
Contributor

minad commented Dec 20, 2021

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?

@localauthor
Copy link
Contributor

Oh I see! Yes, I made an assumption based on the naming of the consult-multi category. So pleased to find that assumption was wrong!

@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 20, 2021

@minad

If I recall correctly there has been a prior discussion of this approach where I already proposed this?

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?

(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.)

+1

@localauthor

Does this mean you're open to adding consult as a dependency?
If so, I would refactor the changes in #491, push those here, and then close that PR.

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.

@localauthor
Copy link
Contributor

Added the consult-multi change to #491. Seems to work, but I haven't put it through much testing yet.

@bdarcus bdarcus changed the title refactor: crm -> cr refactor: insert-citation, crm -> cr Dec 20, 2021
@bdarcus bdarcus changed the title refactor: insert-citation, crm -> cr refactor: crm -> cr Dec 22, 2021
@bdarcus bdarcus force-pushed the no-crm branch 2 times, most recently from 4af4730 to 8baee02 Compare December 22, 2021 18:25
@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 22, 2021

Added the consult-multi change to #491. Seems to work, but I haven't put it through much testing yet.

I've updated the other "open" commands here to use CR instead of CRM. I can rebase this on main once I merge #491.

@bdarcus bdarcus force-pushed the no-crm branch 2 times, most recently from a86e5e5 to d742ca4 Compare December 26, 2021 13:18
@bdarcus bdarcus marked this pull request as ready for review December 26, 2021 13:19
@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 26, 2021

@salutis - you want to give this a try? It changes almost all of the "open" commands, save for citar-open (see #491) to completing-read. If you do get a chance to test, please test also using them in conjunction with embark-act-all.

@rudolf-adamkovic
Copy link

Just tried it, including embark-act-all, and it works great. Sadly, I now usecitar-open exclusively (and always with to open a single item). That said, onward and upward!

@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 26, 2021

Sadly, I now usecitar-open exclusively

That'll be addressed in #491, which also adds notes and links to the results.

@rudolf-adamkovic
Copy link

We should also rename citar-open-library-files to citar-open-library-file.

@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 26, 2021

We should also rename citar-open-library-files to citar-open-library-file.

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.
@bdarcus bdarcus deleted the no-crm branch December 27, 2021 13:47
@bdarcus
Copy link
Contributor Author

bdarcus commented Dec 27, 2021

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 + embark-act-all, but that there are couple/few that would benefit from remaining as part of embark-multitarget-actions.

The main one, that could benefit from experimentation, is citar-insert-citation (and the org-cite-insert processor).

@bdarcus bdarcus mentioned this pull request Dec 27, 2021
@rudolf-adamkovic
Copy link

That was already done. Do you really see otherwise?

All good now. I think package.elreverted the package under me.

@bdarcus bdarcus mentioned this pull request Dec 28, 2021
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

Successfully merging this pull request may close these issues.

4 participants