Support setting different selectrum-display-action for different command?

[BooAA]

As title, maybe we can have selectrum-display-action-alist (similar to ivy's ivy-display-function-alist), so that user can customize different display action for different command.

[clemera]

Thanks for the suggestion. Command names aren't a nice abstraction for this, a command can call completing-read multiple times for example, this is why ivy-read needs to use the :caller argument. With Selectrum you can apply session specific settings by setting the variables locally in minibuffer-with-setup-hook like this:

(minibuffer-with-setup-hook
    (lambda ()
      (setq-local selectrum-should-sort-p nil)
      (setq-local selectrum-display-action 'selectrum-display-full-frame))
  (completing-read
   "Buffer lines: " (split-string (buffer-string) "\n" t)))

[clemera]

To make it easier to modify existing commands without advice I once suggested to allow letting users define properties like:

(put command-name 'selectrum-should-sort-p t)

I like properties better in this case because there is not really a need to have a central place for all display actions to be defined in and also frees us from the need to add such mapping lists for each variable users might want to customize on a per session basis. But this still suffers from the problem I mentioned above (a command might call cr multiple times).

[clemera]

I just thought for the case commands use completing-read multiple times we could allow lists where the items define the nth sessions settings:

(put command-name 'selectrum-should-sort-p '(t ni t)

[clemera]

When #98 would be solved that could be a user friendly way to allow such customizations.

[clemera]

If we go with this we should still provide a macro setting these instead of letting users use put directly which gives us more freedom if we need to adjust something later.

[BooAA]

If we go with this we should still provide a macro setting these instead of letting users use put directly which gives us more > > freedom if we need to adjust something later.

Sounds great, thanks for your quick and nice reply.

[clemera]

Also mentioning #243 here and pinging @minad as this is related. There we talked about better API to configure selectrum around completing-read calls for developers.

[minad]

Using properties is not my favorite solution. @oantolin and I talked about using properties in the context of marginalia to set the command category (I proposed this in some discussion there), but we both concluded that it is generally not a good solution. It is better to keep data in a central list for inspection. I think the exact same argument applies here. Marginalia is configured via alists, if you want to do per-command config, the same should ideally be done here.

Symbol properties only make sense if there is a performance problem with the alist lookup I think or in more specialised cases - but as far as I see it, this is exactly the same scenario as the one @oantolin and I had in marginalia.

[clemera]

It is better to keep data in a central list for inspection.

Hehe, I guess @oantolin will consider this funny as this was also the point I brought up for the property approach he once used in embark. But I don't think the same applies here: The user does not need a single place to inspect how he has configure all commands, what information does he gain from this? I think the decision here is per command and it does not matter where the other properties are used and what values they have.

[minad]

Well, it is still nice to have the information at sight for the reader and via the help view. It is more like a general guideline, to only use properties in very special cases where there is no alternative e.g. due to performance when you don't want the O(n) lookup. Technically there is nothing wrong with one or the other approach. For example if the reader would like to configure a command and wants to see which commands already have some setting, this is very helpful. If properties are used this is much more hassle, since you have to iterate the symbol table etc.

[clemera]

For example if the reader would like to configure a command and wants to see which commands already have some setting, this is very helpful.

Good point, but there is also the downside I mentioned above that we would have to add an alist for each variable.

[minad]

Good point, but there is also the downside I mentioned above that we would have to add an alist for each variable.

I see no difference, you have to define a property per variable you want to configure.

Note, that I generally disagree with the approach. As I proposed in #244, I would expose all customizable variables via properties and not mix-up properties and variables. Have properties as the sole means of configuring selectrum. This is what I would prefer.

At the same time, I don't feel strongly about it. Maybe there are deeper reasons to not do what I am proposing. Independent of what you choose here, it will work out :)

[clemera]

I see no difference, you have to define a property per variable you want to configure.

I was talking about this:

(put command-name 'selectrum-should-sort-p '(t ni t)

With the alist approach we would have selectrum-sort-alist, selectrum-display-action-alist...

Note, that I generally disagree with the approach. As I proposed in #244, I would expose all customizable variables via properties and not mix-up properties and variables. Have properties as the sole means of configuring selectrum. This is what I would prefer.

I don't think there is any mixup, you are probably talking about the keyword args passed to seleectrum-read here? We can remove any ambiguity by making everything configurable through variables. My goal is also to have a single way to configure things.

At the same time, I don't feel strongly about it. Maybe there are deeper reasons to not do what I am proposing. Independent of what you choose here, it will work out :)

Differing opinions help to find the best solution and I'm often wrong and I'm thankful when I notice so thanks for your comments. I'm not fully decided yet and surely want to wait for @raxod502 opinion before acting on this.

[minad]

I was talking about this: (put command-name 'selectrum-should-sort-p '(t ni t)
With the alist approach we would have selectrum-sort-alist, selectrum-display-action-alist...

Sure, but here you define that 'selectrum-should-sort-p is a valid property. Therefore you are still defining something, but maybe with less boilerplate.

I don't think there is any mixup, you are probably talking about the keyword args passed to seleectrum-read here? We can remove any ambiguity by making everything configurable through variables. My goal is also to have a single way to configure things.

Yes, there are these keyword arguments. I don't see how you are going to achieve your goal. Do you remove these arguments completely? I think we are agreeing that it is good to have a single way of doing things. From my pov, this is currently the keyword argument approach, from your side it is the variable approach. This makes sense since I mostly worked with the keywords for now. From my api consumer side I would be happy with only doing the config via keywords.

Btw, why is selectrum-should-sort-p called like this? It is a flag, not a predicate.

[clemera]

I don't see how you are going to achieve your goal. Do you remove these arguments completely?

No they can stay, my approach would be to expose variables for those keywords that currently have no or no public one. Usually you shouldn't use selectrum-read but if you need to the keywords would have precedence like you would expect.

Btw, why is selectrum-should-sort-p called like this? It is a flag, not a predicate.

My guess is it's a misnomer that happened early and just stayed this way for backward compat reasons, @raxod502 might be able to provide more details. Maybe I should open an issue for this.

[minad]

No they can stay, my approach would be to expose variables for those keywords that currently have no or no public one. Usually you shouldn't use selectrum-read but if you need to the keywords have precedence like you would expect.

Well ok, but this not what I find clean. You still have two approaches then and you have to look which thing to configure with which method.

[clemera]

When you use completing-read there is a single way, selectrum-read shouldn't be used, as I said elsewhere I would prefer to make it private even.

[minad]

@clemera Ok at this point everything is done using variables then? I still don't see how you will do it. Properties are not the right way, alists are neither. I want to set variables dynamically per call.

[clemera]

Properties/alists would be for user configuration to set them per call there is minibuffer-with-setup-hook which I know you don't like but it does the job.

[minad]

@clemera I am currently using the setup hook, it is okay. If I can set everything via variables there it is not that bad. But right now, the only things I am overwriting are the initial-input, which should rather be fixed as in #254 and the option to move the default to the top, which is used by consult-line.

[clemera]

It just occurred to me that when we provide a config helper for user configuration as I mentioned above we can also save all command settings in some accessible variable for the user to inspect. The user would use something like:

(selectrum-configure 'show-kill-ring
                     'selectrum-should-sort-p nil
                     'selectrum-display-action 'selectrum-display-full-frame)

Internally we put the properties on the command and when selectrum gets called we automatically set the corresponding variables by checking for selectrum-... properties of the command symbol. This has the advantage that there is nothing else to do when we add new config variables they can automatically be used for general user configuration and command based user configuration. And for developers there is minibuffer-with-setup-hook to configure the variables for some call. I think this would capture all uses cases in a nice way.

[minad]

Ok, but why is this more accessible than an alist per command (command . variable-list) pairs? It is still not possible to see all modified commands by looking at one place. You have to grep for selectrum-configure or grep for putting some selectrum-configs.

[clemera]

You mean externally? As I see it it each time we add a variable we would need to add an alist and then also add code to handle that alist. Internally we could also use alists or whatever and also log the settings to some variable so the user can look them up if this is desired.

[minad]

Yes, this I meant that it is possible to inspect all the configured settings for all commands. But I would avoid to have two different things, like using properties and using a log for inspection since this is just unnecessary complexity and could potentially get out of sync. I would just use an alist. Or just use properties, but then you don't have the introspection possibility.

[clemera]

Oh I got you wrong as what you suggests seems to be yet another way. Actually sounds good to me!

[clemera]

To be clear you suggest adding one variable like selectrum-settings-alist with commands mapped to variable lists, right?

[clemera]

This way we wouldn't need to add any additional alists for each setting, I think this combines all things we have talked about and I don't see any downsides.

[clemera]

Though I still think the alist should probably be internal (making it internal makes clear this shouldn't be bound or used for any purposes, it is only there for inspection). selectrum-configure can then be used to configure things for ease of use and also giving us the freedom to change things down the road.

[minad]

selectrum-settings-alist - yes

[clemera]

selectrum-settings-alist - yes

Okay, I was wondering because I was thinking we were talking about an alist for each variable before, this is also what ivy does/did?. One variable for all settings sounds much better.

[minad]

@clemera No idea about ivy, but I think I wasn't clear about this. But it goes more in the direction what I had in mind in #243 with the single selectrum-options variable.

[clemera]

Not that you suggested it but I don't think the idea there could be combined with a user config setting as the user should have the last word how something should behave. If developers use the same variable to configure the very next call this wouldn't allows us to respect the user settings. This is also why I would like the alist to be private, no one should use it except the user.

[minad]

And how can the command author overwrite settings? Via minibuffer-setup-hook and just setting the variable?

[clemera]

I try to summarize how I would like things to work: Under normal operation users configure their default settings by setting the variables. Developers can configure specific calls by setting the same variables locally in minibuffer-with-setup-hook to adjust for certain needs the command might have. If the user disagrees with some of these settings or just wants some special setup for a command they use selectrum-configure and their wish is command ;)

[minad]

Ok. It seems the selectrum-configure has highest priority and is per-command, right? That is the difference from setting variables directly?

[oantolin]

Btw, why is selectrum-should-sort-p called like this? It is a flag, not a predicate.

I do that too: I use the p or -p suffix for both booleans and boolean-valued functions.

[minad]

@oantolin hmm is this a common thing in elisp?

[clemera]

@oantolin @minad The elisp manual (info "(elisp) Coding Conventions") recommends to avoid it for variable names:

   • If the purpose of a function is to tell you whether a certain
     condition is true or false, give the function a name that ends in
     ‘p’ (which stands for “predicate”).  If the name is one word, add
     just ‘p’; if the name is multiple words, add ‘-p’.  Examples are
     ‘framep’ and ‘frame-live-p’.  We recommend to avoid using this ‘-p’
     suffix in boolean variable names, unless the variable is bound to a
     predicate function; instead, use a ‘-flag’ suffix or names like
     ‘is-foo’.

[minad]

I recommend that too 😁

[clemera]

Ok. It seems the selectrum-configure has highest priority and is per-command, right? That is the difference from setting variables directly?

@minad Yes per-command and highest priority. It's purpose would be to save user from writing advice or similar.

[minad]

@clemera I didn't have a use-case for this before but it makes sense.

[clemera]

Maybe that could be eventually combined with marginalia so users could also configure things based on the classification there. As @oantolin mentioned elsewhere command based is a problem as the command might prompt for info multiple times. We could alleviate that by allowing defining a list of values where the nth value corresponds to the nth prompt of a command but this makes it bit to much implementation dependent.

[oantolin]

@oantolin hmm is this a common thing in elisp?

I don't know, and I should stop doing it, since the 'p' stands for predicate. I think I haven't done it for any publicly named variables.

[clemera]

Ups, sorry I thought marginalia would also provide the classification support for embark now and as such would become a general command "classification" library.

[oantolin]

Maybe that could be eventually combined with marginalia so users could also configure things based on the classification there. As @oantolin mentioned elsewhere command based is a problem as the command might prompt for info multiple times. We could alleviate that by allowing defining a list of values where the nth value corresponds to the nth prompt of a command but this makes it bit to much implementation dependent.

Well, technically even that isn't fully correct, since a command can call completing-read a variable number of times, or call without fixed types, like maybe the second call sometimes prompts for a file and sometimes for a buffer, depending on the results of the first call.

[oantolin]

Ups, sorry I thought marginalia would also provide the classification support for embark now and as such would become a general command "classification" library.

Well, marginalia is only about classifying and annotating minibuffer completion, embark deals with more than just the minibuffer, but everything that is within the scope of marginalia has now been removed from embark.

[clemera]

Well, technically even that isn't fully correct, since a command can call completing-read a variable number of times, or call without fixed types, like maybe the second call sometimes prompts for a file and sometimes for a buffer, depending on the results of the first call.

Ooh, right I haven't thought about that, good points. But I guess in practice that doesn't matter as much. Usually you have a simple command like show-kill-ring and want to configure that to display full screen for example.

Well, marginalia is only about classifying and annotating minibuffer completion, embark deals with more than just the minibuffer, but everything that is within the scope of marginalia has now been removed from embark.

Okay thanks, I haven't followed the development to closely.

[clemera]

@oantolin Maybe we will also allow to define additional things like command name, prompt, completion category... to make it possible to uniquely configure specific prompts in more complicated commands.

[minad]

Maybe we will also allow to define additional things like command name, prompt, completion category... to make it possible to uniquely configure specific prompts in more complicated commands.

If that can be avoided here, I would prefer that. Marginalia offers exactly that - per command category override. I think it does make sense to not add too many selectrum specifics which won't work anywhere else.

[clemera]

I probably have explained myself badly. If we only allow to use the command name for selectrum-configure the user can't configure certain commands which have more complex prompt patterns as @oantolin lined out. To still allow that we can let the user provide additional metrics to configure exactly the prompt they want. This is only about selectrum users wanting to configure selectrum, the commands would still work anywhere else, but the selectrum configuration is selectrum specific of course.

[minad]

@clemera If you implement configuration via metadata, it would also be possible to overwrite the metadata per command via the approach taken by marginalia.

[clemera]

That is appealing, I also like that you would be able to configure settings based on category and such.

[raxod502]

Hmmm, although I tried to find all the relevant threads before responding in #244 (comment)... looks like I failed! So my comment there might be a little outdated as I hadn't yet read this thread. I guess this thread is where I should have posted it, anyway, as it seems quite relevant. I'll take a scroll through this thread soon and see if I have more thoughts. I still have a backlog of well over 100 conversations in my inbox... doing my best to keep up with all the exciting developments!

[minad]

@raxod502 Yes, the discussion is quite distributed. The same happened over in the consult issue tracker when @clemera, @oantolin and I discussed some consult things :) But from your comment #244 (comment) it is clear that you got all the essentials. I will respond shortly there.

[raxod502]

The elisp manual (info "(elisp) Coding Conventions") recommends to avoid it for variable names

I was unaware of this convention until now, so you will probably find such style violations across all of my packages. I'll try to name future variables according to the guide.