Add `selectrum-update-candidates-function'

[clemera]

This function can be used if the set of candidates needs to be
computed dynamically. This simplifies the implementation of functions like selectrum-read-file-name which need to compute the candidate list based on user input.

The new function also takes over the transform input feature of selectrum-refine-candidates-function which now should always return a list of candidates. As selectrum is still at version 0 I thought it might be OK to change the API of existing functions. In general I think adding such a function would make the API a bit cleaner because the refine function now has a single job: filtering the candidates and writing functions which need to compute the candidate list dynamically can be written in a more straight forward way.

If you are OK with the change I can update the docs and README, too.

[raxod502]

Thanks, good idea. Alternative approach: we could have a macro or function for automating the let-binding. But your idea is probably better, as long as we don't start doing too many magic things with the new information that's available about caller intent.

[raxod502]

I have an idea for a simpler API: for selectrum-read (but not for higher-level functions wrapping it), CANDIDATES can be a function of no arguments that returns a list of candidates, instead of the list of candidates itself. In response to your concern, here's what I'm thinking for the control flow:

• In the minibuffer setup hook: set selectrum--preprocessed-candidates to CANDIDATES, transformed by the preprocessing function only if CANDIDATES is a list. In other words, selectrum--preprocessed-candidates can also be either a function or a list.
• In the minibuffer post-command hook: if selectrum--preprocessed-candidates is a function, call it before passing the result to the preprocessing and refinement functions.
• The handling of user input transforms doesn't need to change at all.

I don't think the preprocessing function should need to take the input as an additional argument. The change can be neatly compartmentalized: it just adds a new step where CANDIDATES can be turned from a function into a list, and pushes the call to the preprocessing function to later (the post-command hook) in this case. I don't see as much of a compelling argument to change the other parts of the API.

[clemera]

The main reason for my proposed change was to be able to simplify the implementation of functions such as selectrum-read-file-name. Currently one needs to do something like:

(orig-preprocess-function selectrum-preprocess-candidates-function)
(orig-refine-function selectrum-refine-candidates-function)
(selectrum-preprocess-candidates-function #'ignore)
(selectrum-refine-candidates-function
 (lambda (input candidates)
   ...code which transforms input and updates candidates...
   `((candidates . ,(funcall
                     orig-refine-function
                     new-input
                     (funcall
                      orig-preprocess-function
                      new-candidates)))
     (input . ,new-input))))

By moving the ...code which transforms input and updates candidates... to a separate function it becomes much simpler to implement a dynamic "source" and one does not have to know any details about the selectrum API. I plan to use this a lot because I want to be able to use something like a query language in some of my selectrum commands ("f " prefix to search only recent files, "b " prefix to search only buffers and so on.)

The way I understand your last suggestion you want to take the ...code which updates candidates... part (without the transform input part) and move it to a function with no arguments defined by selectrum--preprocessed-candidates. In this case you would still need a similar pattern to above I think?

[raxod502]

Oh. Good point, sorry -- I didn't inspect your change closely enough. Here are my new thoughts. I quite like the abstraction of preprocess and refine, because it is very intuitive how they should be handled and when they should be called by Selectrum. But I think the feature you described should certainly be well-supported, and the user should not have to do all this let-binding. Furthermore, when looking at the problem, I discovered a serious design flaw: rebinding the refine function as is currently done by selectrum-read-file-name and friends is not technically correct, because the new refine function returns an alist whose candidates key is itself the return value of the original refine function, which could itself be an alist (according to the API at least). It's only the fact that Selectrum doesn't use the input transformation feature by default that lets this work.

Here is what I suggest. Like I mentioned above, we can allow the collection to be a function. If it is a function, then it takes the input as an argument and returns an alist with keys candidates and input. In selectrum-read, calling the preprocess function is skipped if the collection is a function. Then in selectrum--minibuffer-post-command-hook, if the collection is a list, we proceed as normal. If it's a function, however, then we call it and then thread the returned candidates value through the preprocess and refine functions, using the returned input value for the refine function. Then we use the resulting list for display.

What do you think?

[clemera]

I have adjusted the code accordingly. One additional thought I had is that it would be nice to run selectrum-preprocess-candidates-function only when the candidates actually changed (when created dynamically, for the static case it only runs once) because otherwise a recomputation is unnecessary.

[clemera]

I did not remove the changes for selectrum-refine-function docstring because I'm not sure how you want to proceed.

[clemera]

Also note that currently buffer switching is broken because selectrum-completing-read does not work with the new collection type, selectrum-read-buffer should probably just use selectrum-read and apply the predicate itself? Also see #32

[raxod502]

selectrum-read-buffer should probably just use selectrum-read and apply the predicate itself?

Yeah, I think you're right.

[raxod502]

Looks perfect to me. Want to update the README and CHANGELOG?

[clemera]

I can do that, but you would have to help me how to proceed from here ;) When I started with this PR there was no Changelog and the Readme also changed since then. Should I merge master into this branch and then proceed? In general I'm happy for any tips if you notice I'm doing something awkward because I'm not so familiar with git/github workflows.

[raxod502]

No problem. Yes, you should merge master into your pull request branch and resolve any conflicts that arise. If you're not familiar with the process, unfortunately it is rather cumbersome and unintuitive (even for me), but there are many resources online. For formatting the changelog, here's an example of what that looks like: radian-software/prescient.el@a118a18. There are various sections, and you can get an idea of what ones are typically used by looking at the full changelog: https://github.com/raxod502/prescient.el/blob/master/CHANGELOG.md

changes addressed

[clemera]

Ok, I merged and updated the files. If I may ask another question: How do you merge this as a single commit? By that I mean what is your workflow to merge this PR as one commit in such a way that I appear in the commit list and you as a co author (as I have seen you have done in a previous PR).

[raxod502]

Great, this is a fantastic improvement and I appreciate your help.

[raxod502]

How do you merge this as a single commit?

[clemera]

Thank you! I should probably study the github UI more ;)