abo-abo / swiper

Ivy - a generic completion frontend for Emacs, Swiper - isearch with an overview, and more. Oh, man!
https://oremacs.com/swiper/
2.29k stars 338 forks source link

add a fallback for `counsel--M-x-externs` #2870

Open a13 opened 3 years ago

a13 commented 3 years ago

maybe it's possible to use a customizable counsel--M-x-externs-function instead of hardcoding?

this way we can not only remove (optional, but anyway) dependencies on amx/smex, but allow a user to define their own candidates function.

we can provide counsel--M-x-externs-smex and counsel--M-x-externs-amx for compatibility, of course.

See also: https://github.com/justbur/emacs-which-key/issues/288#issuecomment-841533723

basil-conto commented 3 years ago

remove (optional, but anyway) dependencies on amx/smex

There's no hard dependency; those packages are just picked up if present. For example, I use neither.

allow a user to define their own candidates function

What would be the benefit of making counsel-M-x more customisable vs writing a custom completion function using either completing-read or ivy-read?

Thanks.

a13 commented 3 years ago

There's no hard dependency; those packages are just picked up if present.

It tries to require them anyway, I'm not a fan of catch-all-the-exceptions programming

let people to embed dependencies is better than searching over a subset of known solutions

What would be the benefit of making counsel-M-x more customisable vs writing a custom completion function using either completing-read or ivy-read?

In case of which-key all we need is to wrap a call to counsel-M-x

(cl-flet ((counsel--M-x-externs-function
           (lambda () …)))
  (call-interactively #'counsel-M-x))

What would be the drawback of making it customizable?

a13 commented 3 years ago

the less intrusive solution could be just a fallback variable to set, but I still think injecting dependencies is a better approach

basil-conto commented 3 years ago

let people to embed dependencies is better than searching over a subset of known solutions

That's not the intention of counsel--M-x-externs: contributors just wanted their favourite package integrated, and the maintainer obliged.

It's already "customizable" by installing smex/amx

I know what you mean, but I wouldn't call this customisable, since as a user I have no declarative control over which collection counsel-M-x ends up using at runtime.

why write another function when counsel-M-x already has all the needed stuff?

Because there may be a cleaner design.

What would be the drawback of making it customizable?

The risk of compounding an already messy design with something even more complicated, or reinventing existing features, such as the Emacs 28 user option read-extended-command-predicate and the related completion-predicate function property.

In case of which-key all we need is to wrap a call to counsel-M-x

I think it's cleaner to parameterise functions using parameters rather than dynvars.

But even cleaner for the purpose of narrowing a collection of completion candidates is to use the existing mechanism of a predicate function.

So my question is - what is the exact purpose of the proposed feature? To filter the set of commands in obarray down to a smaller, context-dependent set? Or to completely and arbitrarily replace obarray with other collections of commands?

For the latter, we would indeed need to introduce an optional function argument or similar.

For the former, I'd rather reuse some sort of predicate.

WDYT?

a13 commented 3 years ago

contributors just wanted their favourite package integrated, and the maintainer obliged.

The maintainer could provide a mechanism to use an external function instead, looks like a more clean and general solution to me

Because there may be a cleaner design.

I wouldn't call copy-pasting 90% of an existing function to a new one "a cleaner design", or did you mean something else?

Emacs 28

It's not a stable version even in upstream, not to mention LTS distros…

To filter the set of commands in obarray down to a smaller, context-dependent set?

Does counsel--M-x-externs filter obarray? It definitely doesn't. So the answer is no.

For the latter, we would indeed need to introduce an optional function argument or similar.

an argument to what?

basil-conto commented 3 years ago

The maintainer could provide a mechanism to use an external function instead, looks like a more clean and general solution to me

That's what we're discussing. My point was that this isn't how counsel-M-x was "designed" (because it wasn't designed), and that shows in how difficult it is to customise or extend it cleanly.

I wouldn't call copy-pasting 90% of an existing function to a new one "a cleaner design", or did you mean something else?

I didn't mean that duplicating counsel-M-x would be a cleaner design, nor the only cleaner design. I was referring to the proposed dynvar function counsel--M-x-externs-function, and suggesting that there may be a cleaner alternative to that.

My original question about writing a custom alternative to counsel-M-x was to better understand what you are trying to achieve with counsel-M-x, so that any changes to it can be as clean and general as reasonably possible.

It's not a stable version even in upstream, not to mention LTS distros…

It's not released, is what you mean. But Counsel still ought to be designed with upcoming Emacs changes and facilities in mind - what's the point of designing a custom feature that's going to duplicate, clash with, or be inferior to a built-in solution in a year or two? Furthermore, if we try to use upcoming facilities and find them lacking in some way, we can provide feedback upstream and help improve them before Emacs 28 is released.

To filter the set of commands in obarray down to a smaller, context-dependent set?

Does counsel--M-x-externs filter obarray? It definitely doesn't. So the answer is no.

Of course counsel--M-x-externs filters obarray, it returns the subset of obarray that satisfies commandp (as a list of symbol-names).

But I think you misunderstood my question. I'm asking whether whick-key needs a means for:

a) completely and arbitrarily replacing the collection that counsel-M-x completes; or b) filtering and reducing the original superset of commands that counsel-M-x completes to a context-dependent subset.

Because AFAICT the latter just requires a custom predicate, e.g. by let-binding read-extended-command-predicate to a function that looks up commands in a given keymap around a call to counsel-M-x or read-extended-command.

BTW, you don't need to wait until Emacs 28 to avail of this: counsel--M-x-make-predicate already heeds read-extended-command-predicate when it's bound. Could you please check whether that could address your use case, and if not report the limitations?

OTOH completely replacing the original collection is a more intrusive (but not impossible) change because more aspects of the completion depend on it. See e.g. counsel-M-x-action - I guess this would need to be changed to check whether amx-initialized is non-nil before calling amx-rank. Perhaps there are other less obvious subtleties too.

For the latter, we would indeed need to introduce an optional function argument or similar.

an argument to what?

An optional argument to counsel-M-x. Unless I'm missing something, I think that would be more robust than let-binding a dynvar around calls to counsel-M-x.

a13 commented 3 years ago

An optional argument to counsel-M-x

an optional argument to an interactive function? Am I missing something?

Of course counsel--M-x-externs filters obarray, it returns the subset of obarray that satisfies commandp (as a list of symbol-names).

now check the code again

filtering and reducing the original superset of commands that counsel-M-x completes to a context-dependent subset.

for which-key - yes, but for amx/smex filtering is not enough! So b) isn't good enough as a general solution

completely replacing the original collection is a more intrusive (but not impossible) change

sure it's not impossible, counsel--M-x-externs completely replaces the original collection

(let ((externs (counsel--M-x-externs)))
  (ivy-read (counsel--M-x-prompt) (or externs obarray)
            :predicate (if externs
                           #'counsel--M-x-externs-predicate
                         (counsel--M-x-make-predicate))
            :require-match t
            :history 'counsel-M-x-history
            :action #'counsel-M-x-action
            :keymap counsel-describe-map
            :initial-input initial-input
            :caller 'counsel-M-x))

But Counsel still ought to be designed with upcoming Emacs changes and facilities in mind

with the stable (including LTS) versions in mind as well

basil-conto commented 3 years ago

An optional argument to counsel-M-x

an optional argument to an interactive function? Am I missing something?

Probably, since counsel-M-x already has one:

https://github.com/abo-abo/swiper/blob/040d458bce4a88f37359192061bcea5ebe87007c/counsel.el#L937-L941

now check the code again

I already did before saying anything, e.g.:

https://github.com/DarwinAwardWinner/amx/blob/37f9c7ae55eb0331b27200fb745206fc58ceffc0/amx.el#L764-L776 https://github.com/DarwinAwardWinner/amx/blob/37f9c7ae55eb0331b27200fb745206fc58ceffc0/amx.el#L820-L827

If you meant something else, then please clarify.

for which-key - yes, but for amx/smex filtering is not enough

Why not? Could you please describe how filtering obstructs amx/smex functionality? To be clear, I'm talking about filtering in the completion :predicate, which is applied after amx/smex have returned their results.

sure it's not impossible, counsel--M-x-externs completely replaces the original collection

Yes but this is an internal implementation detail that is not part of a public API. IOW, the other moving parts of counsel-M-x can easily be kept in sync with any changes to counsel--M-x-externs. Any new API we expose publicly will have to account for these currently hard-coded internal details. Hence this would be a more intrusive redesign than just making clever use of a predicate, which is already supported.

with the stable (including LTS) versions in mind as well

Neither Counsel nor Emacs really have stable or LTS versions, so I'm not sure what you mean.

a13 commented 3 years ago

Neither Counsel nor Emacs really have stable or LTS versions, so I'm not sure what you mean.

Some people still use linux, you know?

Yes but this is an internal implementation detail that is not part of a public API.

de jure - yes, de facto - there's no such thing as "internal" in emacs lisp

besides that, it depends on (if they are present in a system) smex/amx anyway, which change the behavior completely

Probably, since counsel-M-x already has one:

how would one use it without defining a new interactive function then? :]

basil-conto commented 3 years ago

Some people still use linux, you know?

Myself included. How does that relate to the current discussion?

de jure - yes, de facto - there's no such thing as "internal" in emacs lisp

I'm talking about the concrete design of counsel-M-x (e.g. its invariants), not theoretical encapsulation.

besides that, it depends on (if they are present in a system) smex/amx anyway, which change the behavior completely

Sure, but the current design takes that into account (or intends to, anyway).

This is not guaranteed for arbitrary external functions.

how would one use it without defining a new interactive function then?

Without defining another function - using a prefix argument. But what's wrong with calling counsel-M-x from another function/command?

With more specific technical details about how it's intended to be used, I can give more specific suggestions.

a13 commented 3 years ago

How does that relate to the current discussion?

there are LTS distros, so it's hard to get the freshest version of Emacs

Sure, but the current design takes that into account (or intends to, anyway).

exactly, it intends to, if something will change in a new amx/smex version - counsel-M-x couldn't guarantee that it will continue to work as expected. By introducing a customizable for that you pass on the responsibility to a user, who has to explicitly tell "I want X to be Y".

This is not guaranteed for arbitrary external functions.

amx/smex ARE

Without defining another function - using a prefix argument.

Yeah, counting 4s, remembering which number means what, U for Usability!

a13 commented 3 years ago

With more specific technical details about how it's intended to be used, I can give more specific suggestions.

I think I have to rethink all the stuff again, will tell you later

basil-conto commented 3 years ago

there are LTS distros, so it's hard to get the freshest version of Emacs

Like I said, you don't need the development version of Emacs to use read-extended-command-predicate: Counsel already supports the variable when bound, i.e. you can bind it yourself today in e.g. Emacs 24.

exactly, it intends to, if something will change in a new amx/smex version - counsel-M-x couldn't guarantee that it will continue to work as expected.

That would be considered a bug in counsel-M-x.

By introducing a customizable for that you pass on the responsibility to a user

My argument against this is that counsel-M-x currently makes some assumptions about who is responsible for the collection, so passing on the responsibility to the user with a new customisation option would require a more intrusive redesign of counsel-M-x.

This is why I'm asking whether we can already solve the problem with existing functionality, such as a custom predicate, or by making less intrusive changes, such as accepting a custom predicate as a function argument, as that would lessen the maintenance burden at no loss of generality.

amx/smex ARE

Yes, but counsel-M-x provides explicit support for them, i.e. they are officially integrated. This would not be true for some arbitrary user-provided collection.

Yeah, counting 4s, remembering which number means what, U for Usability!

I was answering your question, not suggesting this was the most usable interface.

I think I have to rethink all the stuff again, will tell you later

Thanks.

a13 commented 3 years ago

Thanks

Thanks to you for the affirmative conversation and sorry if I look rude, I really like ivy and just want it to be even better!

Yes, but counsel-M-x provides explicit support for them, i.e. they are officially integrated.

in a "magical" (for a user) way, it really confused me a bit

TMO, user have to explicitly ask for using external stuff, there are many ways to do that:

  1. make a customizable like counsel-M-x-engine (with values like nil, smex, amx)

sounds like a trade-off, but still counsel-M-x still has (optional, but…) dependencies on external libraries

  1. add separate fns counsel-M-x-smex, counsel-M-x-amx (btw, since you prefer to support external libraries yourself, counsel-M-x-which-key would fit nice here), maybe even not in counsel.el itself, but some kind of counsel-extras.el or something like that

Sounds much better to me, the only downside (debatable) I can see is user have to explicitly bind the command they want to use even when counsel-mode is on.

basil-conto commented 3 years ago

Thanks to you for the affirmative conversation and sorry if I look rude, I really like ivy and just want it to be even better!

No worries, and thanks for your suggestions and being patient with me :).

in a "magical" (for a user) way, it really confused me a bit

Yes, IMO it's not ideal either, but it's hard to change history backward-compatibly.

sounds like a trade-off, but still counsel-M-x still has (optional, but…) dependencies on external libraries

There are many existing cases where Counsel integrates with external packages, so optional external dependencies are not a problem.

(btw, since you prefer to support external libraries yourself

Do I? :) I'm just trying to help maintain/improve what's already there...

counsel-M-x-which-key would fit nice here)

What would that do? Complete commands only in the current-local-map? (Sorry, I'm not familiar with which-key.)

maybe even not in counsel.el itself, but some kind of counsel-extras.el or something like that

What would be the benefit of having a new file, when counsel.el already links to so many external packages?

Sounds much better to me, the only downside (debatable) I can see is user have to explicitly bind the command they want to use even when counsel-mode is on.

Then perhaps the user should be allowed to cycle through available/configured "backends" live?

The problem with both (1) and (2) is that they don't support arbitrary user-provided backends, which I thought was the goal of the OP. Recall that backends can't be as simple as "here's a list of commands you can complete", because there may be setup/teardown steps required - see e.g. counsel-M-x-action for some extra steps that have to be taken in the case of amx/smex.

With option (1), the user option would have to be more complicated than just symbol values (or we would need to introduce a separate e.g. alist for associating setup/teardown functions with entries in the user option).

With option (2), the user would need to write their own replacement for counsel-M-x, which is no different to the status quo.

Or am I missing something?