Add keep-selection arg to selectrum-exhibit

[clemera]

@minad As we discussed this lets you call selectrum-exhibit without updating the index which is allows for better handling of async completions as shown in #306.

[minad]

Thank you for working on this! Is this correct? I do not want to preserve the index, but the selected candidate string. Otherwise the candidate could magically change when new candidates come in. See the discussion in minad/consult#91.

[clemera]

As long as new candidates are added at the end this should work for your purposes, but please give it some testing.

[minad]

Or does the index correspond to the index in the original (non refined) candidate list. Then it would be okay given we append only. This is the case right now with the consult async implementation.

[minad]

There is (nth index selectrum--refined-candidates) in the code where index=selectrum--current-candidate-index, so I think this is not correct.

[clemera]

I'm not sure what you mean, my last commit was about something else.

[clemera]

Can you explain your last comment a bit more?

[minad]

1. If I have the candidates "alpha beta gamma", current candidate index is i=0
2. User enters "b", the candidates are refined to "beta", current candidate index is i=0
3. Async pushes another candidate "base", refined candidates are "base beta", current candidate index should be i=1

So if you just retain the index while candidates are appended, it is not sufficient. Does this explain it? But this makes only sense if you sort.

[minad]

So what I think we need:

1. remember the current candidate string
2. refresh candidate list and refined candidate list
3. recompute the current candidate index by searching the list of refined candidates

[clemera]

Thanks, so it can break with sorting, too bad. I hope I find time to look at it tomorrow, the next few days I won't be online as much.

[minad]

Sure, no hurry! Thanks again! Have a nice Christmas and holidays!

[clemera]

Thanks and thanks for testing, I wish you the same!

[clemera]

The problem you described is fixed now, is there anything else you think we should consider?

[minad]

@clemera Thank you. Great to hear. I did not test it yet, but I will try it soon and then I will come back to you in case more is needed! Originally I had a problem with pressing the up down keys before entering some string for filtering. In that case the up down keys don't work directly.

[clemera]

Great, I had not noticed the up/down key issue before and currently I can not see it so when you give it a try please let me know if it still exists.

[minad]

@clemera I just tried it and it works very well - thank you!

Regarding the patch, I wonder if it is necessary to introduce the keep-candidate variable? I generally try to reduce global state since it can easily get messed up. I am increasingly worried by the number of global variables in selectrum. If there is a possibility to reduce this or avoid introducing new ones, it would probably be good. @oantolin also recently reduced global state in Embark. Then there is this macro to save global state. Would it be possible to replace this all with closures or minibuffer local state, which would work automatically with recursive minibuffers? This is at least the route I am taking in Consult - everything should be minibuffer-local.

[minad]

@clemera The global state avoidance is also one thing why I dislike configuring selectrum via setting global variables, instead preferring selectrum-metadata if calling via completing-read or keyword arguments if calling via selectrum-read.

[minad]

why not call the hook here with the current candidate to keep as argument? Maybe the selectrum--minibuffer-post-command-hook should be extracted into a separate selectrum--post-command function which is then called from both here and from the selectrum--minibuffer-post-command-hook. I don't think it is the best style to call hook functions from somewhere else, they should only be called via run-hooks after putting them in the appropriate hook list.

[clemera]

I followed your suggestions, I also don't want to add an argument to the hook function itself. Could you test this with your async function?

[clemera]

Regarding the patch, I wonder if it is necessary to introduce the keep-candidate variable?

I agree, it works both ways and the variable allows other use cases we haven't thought of but I also don't see any other ones so I think adding an argument to the hook instead is good.

The global state avoidance is also one thing why I dislike configuring selectrum via setting global variables, instead preferring selectrum-metadata if calling via completing-read or keyword arguments if calling via selectrum-read.

I have described the benefits I see with this approach here. I think its very important to let other callers reuse your code but adjust the settings for their purposes.

[minad]

@clemera I agree that using a variable gives more flexibility. But I would avoid this kind of premature globalisation if possible.

In the case of the configuration I agree. I am only not fond of it. But it is not that I don't see why it is useful :)

[minad]

Looks good from my side! This is the last missing piece for perfect async support in Consult :)

[minad]

I will do a test now - didn't test it before only inspected what you did and was satisfied.

[clemera]

Okay, thanks!

[minad]

@clemera works well! As expected! :)

[minad]

@clemera Can you merge this or is something else missing?

[clemera]

Nothing missing just me in front of the computer ;)