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

Add an option to disable multiple-candidate selection #492

Closed
rudolf-adamkovic opened this issue Dec 17, 2021 · 12 comments · Fixed by #493
Closed

Add an option to disable multiple-candidate selection #492

rudolf-adamkovic opened this issue Dec 17, 2021 · 12 comments · Fixed by #493
Labels
enhancement New feature or request

Comments

@rudolf-adamkovic
Copy link

rudolf-adamkovic commented Dec 17, 2021

I use Citar every day and have done so for quite a while now. Yet, I have never needed to select multiple items. Not once. Thus, all these RET RET confirmations at every step do nothing for me. Thus, I would like to ask for a configuration option to disable multiple-candidate selection in Citar.

P.S. Today I decided to try using MCT (Minibuffer and Completions in Tandem) instead of Vertico, and the Citar workflow became even more painful due to multiple-candidate selections.

@rudolf-adamkovic rudolf-adamkovic added the enhancement New feature or request label Dec 17, 2021
@bdarcus
Copy link
Contributor

bdarcus commented Dec 17, 2021

WDYT @roshanshariff @localauthor?

Any unintended consequences from supporting this?

One obvious question I have is whether that would apply to all commands.

@bdarcus
Copy link
Contributor

bdarcus commented Dec 17, 2021

PS - the issue with the consult crm UI can be resolved in config so that you use TAB to select, and RET to exit, in which case it behaves as CR when selecting only one.

I don't have any example code though.

@roshanshariff
Copy link
Collaborator

roshanshariff commented Dec 17, 2021

I agree with @salutis sentiment. And it's pretty easy to implement a citar-select-multiple option, for example.

But I also wonder if some commands should just not use CRM anyway:

  • citar-insert-citation could be modified to position point at the end of the citation, and when called again to just add new keys to the citation before point. Then you could insert multi-key citations by repeating the command, using the embark-quit-after-action functionality.
  • Similarly for citar-open-notes and citar-open-entry. You can just repeat the command if you want to open more than one.

EDIT: But if we could figure out the alternate keybindings @bdarcus mentioned, I might be happy with that too.

@bdarcus
Copy link
Contributor

bdarcus commented Dec 17, 2021

citar-insert-citations is a pretty critical command, so I'd have to think about and experiment a bit to be comfortable with that kind of change.

But yeah, we can think abou that.

But if we could figure out the alternate keybindings @bdarcus mentioned, I might be happy with that too.

Here's the commit where Daniel removed the bindings from Consult:

minad/consult@49b6442

This will likely be added, for example, to the Doom vertico module.

roshanshariff added a commit to roshanshariff/citar that referenced this issue Dec 17, 2021
When nil, all citar commands will use `completing-read` instead of
`completing-read-multiple`.

Closes emacs-citar#492
bdarcus pushed a commit that referenced this issue Dec 17, 2021
When nil, all citar commands will use `completing-read` instead of
`completing-read-multiple`.

Closes #492
@localauthor
Copy link
Contributor

localauthor commented Dec 17, 2021

Here's the commit where Daniel removed the bindings from Consult:

minad/consult@49b6442

Evaluating this seems to do the trick:

  (defun consult-vertico--crm-select ()
    "Select/deselect candidate."
    (interactive)
    (when (let ((cand (vertico--candidate)))
            (and (vertico--match-p cand) (not (equal cand ""))))
      (vertico-exit)))

  (defun consult-vertico--crm-exit ()
    "Select/deselect candidate and exit."
    (interactive)
    (when (let ((cand (vertico--candidate)))
            (and (vertico--match-p cand) (not (equal cand ""))))
      (run-at-time 0 nil #'exit-minibuffer))
    (vertico-exit))

  (define-key consult-crm-map [remap vertico-insert] #'consult-vertico--crm-select)
  (define-key consult-crm-map [remap exit-minibuffer] #'consult-vertico--crm-exit)

I'm adding this to my own config to try for a bit, because this behavior seems really great. We'll see.

@bdarcus
Copy link
Contributor

bdarcus commented Dec 17, 2021

I'm adding this to my own config to try for a bit, because this behavior seems really great.

Yes; it's basically the UI that we settled on in the initial vertico-crm prototype. I think the only reason he removed it from consult was it adds completion UI specific code he didn't want to maintain.

In practice, it behaves exactly the same as completing-read when you just want to select one, so you get the flexibility to select multiple without any downside.

@minad
Copy link
Contributor

minad commented Dec 18, 2021

Could it happen that Citar moves to completing-read only or is this not realistic? This would remove quite a bit of complexity. Note that embark-multitarget-actions have only been introduced to support the Citar actions which take multiple candidates at once (CRM commands). If Citar moves away from completing-read-multiple this Embark feature would not be needed. embark-act-all will stay since this is generally useful and we may still want to add a feature to select candidates one by one and then embark-act-all on them. This way one should end up with a UI which is quite similar to crm, but based purely on cr without the CRM complications.

@bdarcus
Copy link
Contributor

bdarcus commented Dec 18, 2021

@minad,

Could it happen that Citar moves to completing-read only or is this not realistic?

Yes, it's possible, depending in part on how the embark etc features evolve.

I think we also need to test out the idea @roshanshariff was floating. The code, both here and in org, has evolved a lot since I first settled on CRM.

Note that embark-multitarget-actions have only been introduced to support the Citar actions which take multiple candidates at once (CRM commands). If Citar moves away from completing-read-multiple this Embark feature would not be needed. embark-act-all will stay since this is generally useful and we may still want to add a feature to select candidates one by one and then embark-act-all on them. This way one should end up with a UI which is quite similar to crm, but based purely on cr without the CRM complications.

Is it not likely/possible that embark-multitarget-actions might still be valuable in some situations, independent of CRM? I was thinking if we did get rid of CRM, we could still use it for a couple of functions.

@minad
Copy link
Contributor

minad commented Dec 18, 2021

Is it not likely/possible that embark-multitarget-actions might still be valuable in some situations, independent of CRM? I was thinking if we did get rid of CRM, we could still use it for a couple of functions.

Maybe. But if you make sure that your single-target commands, which rely on completing-read, can be called repeatedly (this is the proposal by @roshanshariff), then there is no need at all for completing-read-multiple and for the artificial support in Embark (embark-multitarget-actions).

The code, both here and in org, has evolved a lot since I first settled on CRM.

I am not sure if you remember, but I've argued multiple times against CRM. The API is not well specified, has edge cases and overall feels broken and not well-designed. I mean it suffices to look at the default completion experience of CRM without consult-completing-read-multiple. And as as this particular issue shows consult-completing-read-multiple still does not feel good enough for the user.

The idea by @roshanshariff is similar to the picture I had in mind for an ecosystem, where completing-read-multiple does not play a significant role. You can still get multi action support, but the elementary composable units of the whole system are single-target commands based on completing-read only. I hope this also explains why I argued so strongly against extending embark-act with multi target support. I am convinced that an approach without completing-read-multiple is possible and maybe one ends up with simpler and more reusable components.

@bdarcus
Copy link
Contributor

bdarcus commented Dec 18, 2021

I am not sure if you remember, but I've argued multiple times against CRM.

I do; a few reasons why I didn't follow through then:

  1. constraints of bibtex-completion, which was extracted from the helm implementation, where all functions take a list of key inputs
  2. I wasn't clear what would be an elegant UX with that approach (and am still not certain)
  3. how to achieve it in code (more clear now, in part because of completion and release of org-cite)

@rudolf-adamkovic
Copy link
Author

@roshanshariff @bdarcus

bdarcus closed this in #493 yesterday

So much better! That said, while citar-open no longer asks for multiple "Reference" items, it still asks for multiple "Related resources" items.

@bdarcus
Copy link
Contributor

bdarcus commented Dec 18, 2021

Well, that's different code, different UI.

In principle opening related resources could be a good candidate for an embark-act-all option, in which case it might make sense to remove CRM from there entirely, rather than offering it as a configuration option. But that may be a bit premature.

In the meantime, I've created a branch to test that out in #496.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants