Add citar--select-multiple

[bdarcus]

This replaces completing-read-multiple with a simple alternative,
courtesy of minad (see #160), that calls completing-read in a loop.

See #582

---

This works, but needs testing, and polish. Details:

1. history doesn't work currently, even though I added a "history" argument.
2. I assume since this uses grouping, it won't work pre-Emacs 28. But that was true for consult-crm. Need to address.
3. Will this work with Embark? Currently it doesn.t
4. ideally we could have this work like the vertico-crm prototype; TAB selects and deselects, and RET completes. Is that possible, @minad?

On 4, maybe want to borrow from earlier consult?

I've added my adaptation here in a commit for citar-vertico.el, though it doesn't work correctly currently. When I do M-x I have to use TAB to select the command, which is obviously not what we want. Rather, I only want this rebinding to happen with citar--select-multiple. But I don't really understand this code. Any suggestions?

cc @aikrahguzar @localauthor

[minad]

Finally, ideally we could have this work like the vertico-crm prototype; TAB selects and deselects, and RET completes. Is that possible, @minad?

I think this would require completion UI specific code.

[bdarcus]

I think this would require completion UI specific code.

As was the case with consult-crm initially. I guess

Perhaps I can just include it for vertico, either in the code, or in the example config?

@iyefrat: you sorted this out for consult-crm; any suggestions or tips?

[iyefrat]

When it comes to getting the custom bindings to work, it is indeed completion UI specific, the code in doom (linked in this comment) only worked for vertico.

I'm not sure what you mean about M-x though, since under the doom keybinding variant RET will select and exit single selections. Also, you probably want the custom crm stuff to only apply for citar and not vertico as a whole, no?

[bdarcus]

you probably want the custom crm stuff to only apply for citar and not vertico as a whole, no?

Yes, and specifically citar--select-multiple, which is a simpler version of consult-crm, that does not use CRM.

I'm thinking I need to maybe do something on that function, but not sure what.

[bdarcus]

@minad - this can't work with Embark; can it?

[aikrahguzar]

I finally tried this out and it works well. The only two differences from the current situation are:

1. Having to press enter on empty selection to exit. I think this might be fixed by the citar-vertico package which I haven't tried yet.
2. Selected candidates show up in both group which I find confusing. This is due to carrying around selected list which I think is unnecessary since we can check if a candidate is selected using the hash-map that is there. Changing citar--select-multiple to this,

(defun citar--sort-by-selection (selected-hash cands)
  "Sort the candidates by putting those in SELECTED-HASH first"
  (let ((selected)
        (others))
    (dolist (cand cands (nreverse (nconc others selected)))
      (if (gethash (substring-no-properties cand) selected-hash)
          (push cand selected)
        (push cand others)))))

(defun citar--select-multiple (prompt candidates &optional history)
  "Select multiple CANDIDATES with PROMPT.
HISTORY is the 'completing-read' history argument."
  ;; Because completing-read-multiple just does not work for long candidate
  ;; strings, and IMO is a poor UI.
  (let ((selected-hash (make-hash-table :test #'equal)))
    (while
        (let ((item
               (completing-read
                (format "%s (%s/%s): " prompt
                        (hash-table-count selected-hash)
                        (length candidates))
                (lambda (str pred action)
                  (if (eq action 'metadata)
                      `(metadata (display-sort-function . (lambda (cands) (citar--sort-by-selection ,selected-hash cands)))
                                 (cycle-sort-function . ,#'identity)
                                 (group-function
                                  . ,(lambda (cand transform)
                                       (pcase (list (not (not transform)) (gethash (substring-no-properties cand) selected-hash))
                                         ('(nil nil) "Select Multiple")
                                         ('(nil t)   "Selected")
                                         ('(t   nil) cand)
                                         ('(t   t  ) (propertize cand 'face 'highlight))))))
                    (complete-with-action action candidates str pred)))
                nil
                t
                nil
                history ; FIX
                "")))
          (unless (equal item "")
            (cond
             ((gethash item selected-hash)
              (remhash item selected-hash))
             (t
              (puthash item t selected-hash)))
            t)))
    (hash-table-keys selected-hash)))

brings it closer to the old behavior. I am not sure if citar--sort-by-selection is necessary but without it the Selected group sometimes jumps to the bottom. This happens when I select multiple candidates and then deselect one of them. Is there a way to order groups?

[bdarcus]

@aikrahguzar

1. doesn't work ATM, and I don't know how to make it work.
2. I hadn't thought about sort, or noticed a problem, but here's the original consult sort function.

[aikrahguzar]

@aikrahguzar

1. doesn't work ATM, and I don't know how to make it work.

One way I can think of: define another function that is vertico-exit but in addition sets a variable to t. We break the loop when this variable is t. But we need to bind this function at the right place (how does consult implement command specific keymaps?). Maybe this isn't worth it or maybe it can be added somewhere other than citar since it has to depend on the details of the completion system. I will probably hack something for myself.

Maybe there is a way of setting such a variable using minibuffer-exit-hook that less dependent on the completion framework.

2. I hadn't thought about sort, or noticed a problem, but here's the [original consult sort function](https://github.com/minad/consult/commit/8e9d5e73582a38c1c48d5d392c42870aac1e9abf#diff-f5a84ba2f501aa963898f4185aece1ea6eba0c084be8ed36ac936b2a30dc8017L2717).

There is no problem with your code but with that a selected candidate is in both the groups. I tried to fix that but that causes groups to exchange order sometimes and sort function fixes that. The one I wrote is basically the same as the consult if the original sort function for the command is identity which is the case here. Do we need the added flexibility here?

[bdarcus]

There is no problem with your code but with that a selected candidate is in both the groups.

I basically copied Daniel's first iteration of this idea, in #160. I think he figured out refinements for issues like this in the consult implementation.

Do we need the added flexibility here?

Flexibility?

Did you see my Embark question, BTW?

[aikrahguzar]

Do we need the added flexibility here?

Flexibility?

I mean citar doesn't allow users to set their own sort function for candidates. Do we want that to be possible? Otherwise what I wrote should be fine.

Did you see my Embark question, BTW?

I did, but I am out of loop on embark. I have used it as a bit by now (SPC a is often a nice way to see options). I know there have been changes in how citar deals with embark but I am too hazy on details to know how this change relates to embark.

[aikrahguzar]

Poking around a bit, I think embark can be supported. We need all the candidates in the "Selected"group. Group function can be recovered from completion-metadata and using that a version of embark-consult--crm-selected seems doable.

[bdarcus]

I mean citar doesn't allow users to set their own sort function for candidates. Do we want that to be possible?

I don't think so.

[bdarcus]

I added your mods @aikrahguzar.

[aikrahguzar]

I added your mods @aikrahguzar.

I don't see any changes.

[bdarcus]

I added your mods @aikrahguzar.

I don't see any changes.

Oops; seems I forgot to push!

Fixed.

[bdarcus]

I've rebased this and fixed some conflicts, and also updated the issues list in the main post.

Notably, embark doesn't work correctly with this (maybe we need a specialized target finder?), and I'd really like to figure out the keybinding issue.

[aikrahguzar]

Notably, embark doesn't work correctly with this (maybe we need a specialized target finder?), and I'd really like to figure out the keybinding issue.

Assuming the current support for embark is due to embark-consult--crm-selected, I think this should work if added to embark-candidate-collectors (I haven't tested it),

(defun citar--embark-selected ()
  "Return selected candidates from `citar--select-multiple' for embark."
  (when-let (((eq minibuffer-history-variable 'citar-history))
             (metadata (embark--metadata))
             (group-function (completion-metadata-get metadata 'group-function))
             (cands (all-completions
                     "" minibuffer-completion-table
                     (lambda (cand)
                       (and (equal "Selected" (funcall group-function cand nil))
                            (or (not minibuffer-completion-predicate)
                                (funcall minibuffer-completion-predicate cand)))))))
    (cons (completion-metadata-get metadata 'category) cands)))

It is using embark--metadata so that function will need to be lifted in to citar.

Getting selected candidates can actually be simplified if the selected-hash in citar--select-multiple is made a dynamic variable though that isn't a completely safe thing to do.

[bdarcus]

It is using embark--metadata so that function will need to be lifted in to citar.

Maybe I'm just tired today, but why?

Wouldn't the function only get called by embark, and so that function would be loaded?

[aikrahguzar]

Maybe I'm just tired today, but why?

Wouldn't the function only get called by embark, and so that function would be loaded?

That should work then. I was thinking that every thing defined by a package can only use functions/variables defined by packages transitively required by it, but that is definitely not how elisp works.

[bdarcus]

Assuming the current support for embark is due to embark-consult--crm-selected, I think this should work if added to embark-candidate-collectors (I haven't tested it),

Just getting back to this @aikrahguzar.

I have, for now, added that function to a separate citar-embark.el file, with the idea to possibly move some of the other Embark-related stuff there.

BTW, I tested this, and it doesn't seem to work, but I think something is screwed up with embark in general on my system ATM.

Sigh ... Emacs.

If you have time, could you see about testing it?

And on another matter, this branch has diverged a fair bit from main, so it creates conflicts, which are not easy me to resolve.

So if you do test it, and get your git setup, you can also feel free to create in a new PR, and I'll close this one.

[aikrahguzar]

If you have time, could you see about testing it?

I actually didn't know what to test. I think the command is embark-act-all but it doesn't seem to be bound anywhere and I haven't figured out how to call embark from minibuffer although I have started using SPC a on occasion. Is it bound somewhere on vartico-map or do I need to bind it myself?

And what would be the simplest test to perform?

And on another matter, this branch has diverged a fair bit from main, so it creates conflicts, which are not easy me to resolve.

So if you do test it, and get your git setup, you can also feel free to create in a new PR, and I'll close this one.

I will do this during coming days. Removal of consult-crm has landed in doom so I have more impetus to get this sorted :) but it might not happen until next weekend.

[bdarcus]

Oh, that's right; you're also on Doom.

So embark-act should be bound in the minibuffer to C-;. You should see, when doing that the citar commands accessible from there.

If your function doesn't work correctly, you will see generic commands.

On my install, all of the sudden that binding isn't working at all.

[aikrahguzar]

Oh, that's right; you're also on Doom.

So embark-act should be bound in the minibuffer to C-;. You should see, when doing that the citar commands accessible from there.

If your function doesn't work correctly, you will see generic commands.

I will give it a try tomorrow.

On my install, all of the sudden that binding isn't working at all.

C-h is one vanilla keybinding I have learned. You can use C-h k C-; to figure out what is overriding it.

[bdarcus]

C-h is one vanilla keybinding I have learned. You can use C-h k C-; to figure out what is overriding it.

I knew about that, but forgot!

There's something really weird about when I do that. When I press C-;, it doesn't actually register, but instead inserts that text into the main doom screen.

Shrug. Maybe it's time for me to reinstall Doom and recompile everything.

[aikrahguzar]

The embark problem is due to metadata not having a category and citar--select-multiple not let binding embark-transformer-alist like citar-select-alist does. Doing those two things fixes the citar commands not appearing in embark-act map. And embark-act-all offers to act on the select references but from there I don't know what it is supposed to do.

My takeaway is that there should be more code-sharing between citar-select-ref and citar--select-multiple and idealy the later should call the former in a loop but that is not straightforward due to metadata differences. I will try to cook up a PR soon.

[bdarcus]

And embark-act-all offers to act on the select references but from there I don't know what it is supposed to do.

This is a newer feature we were hoping would enable us to lessen dependence on multiple selection over time (though if we get this working well, we might not want that).

It allows you, when using CR, to run commands against multiple candidates; for example, to mark some subset of them, or run against all.

My takeaway is that there should be more code-sharing between citar-select-ref and citar--select-multiple and idealy the later should call the former in a loop but that is not straightforward due to metadata differences. I will try to cook up a PR soon.

Great; thanks!

I'm going to close this for now then.