bug#27193: 25.2; tmm should use completing-read-default

[Ryan Thompson <rct@thompsonclan.org>]

[-- Attachment #1: Type: text/plain, Size: 10056 bytes --]

I should revise what I said slightly. Alternate completion implementations
might never be able to do away completely with blacklists, because things
like read-file-name also use completing-read. Still, I stand by my
statement that nothing else I'm aware of uses completing-read in the same
way as tmm.

On Fri, Jun 2, 2017 at 1:37 PM Ryan Thompson <rct@thompsonclan.org> wrote:

> On Fri, Jun 2, 2017 at 11:26 AM Drew Adams <drew.adams@oracle.com> wrote:
>
>> I don't understand that argument.  (But to be clear, I don't
>> really care much about `tmm'.)
>>
>
> Fair enough. My post certainly assumes quite a bit familiarity with tmm.
> I'm happy to elaborate below.
>
>
>> I don't see that that is an argument for _preventing_ the
>> use of a `completing-read-function' by tmm.  I see it
>> only as a statement that it might not be helpful to use
>> some particular values of `completing-read-function' with
>> tmm.
>>
>> If there is a general problem then consider adding a
>> comment in `tmm.el' (or perhaps put it in some doc
>> string) that says what you are really saying: Some
>> values of `completing-read-function' might not be
>> helpful with `tmm', and if you find that is the case,
>> consider binding that variable to nil.
>>
>> But now that I've taken a quick look (just now) at the
>>
> use by tmm of `completing-read', I don't see that there
>> is a problem to be fixed in `tmm.el'.  I don't see that
>> its use of `completing-read' is particularly exotic or
>> problematic.  This is it:
>>
>> (completing-read
>>  (concat gl-str " (up/down to change, PgUp to menu): ")
>>  (tmm--completion-table (reverse tmm-km-list)) nil t nil
>>  (cons 'tmm--history (- (* 2 history-len) index-of-default)))
>>
>> I don't see anything at all unusual about that.
>>
>> And the collection function, `tmm--completion-table',
>> is likewise pretty ordinary, I think:
>>
>> (defun tmm--completion-table (items)
>>   (lambda (string pred action)
>>     (if (eq action 'metadata)
>>         '(metadata (display-sort-function . identity))
>>       (complete-with-action action items string pred))))
>>
>> You say:
>>
>> > tmm already pretty much relies on the assumption that
>> > completing-read is actually calling completing-read-default.
>>
>> I don't see any evidence of that.
>>
>
> tmm's weirdness is not in the actual call to completing-read. That
> completing-read call is wrapped by "(minibuffer-with-setup-hook
> #'tmm-add-prompt ...)", which in turn calls tmm-define-keys, which sets up
> a bunch of non-standard key bindings, which have the effect of implementing
> a menu system with single-keypress activation of menu items, rather than
> completion of a substring with string matching. The result is not even
> recognizable as completing-read. The use of completing-read is merely an
> implementation detail in a system that is *not* actually doing completion
> of any kind. So pluggable completion backends don't make any sense for tmm,
> and I can't imagine any other value of completing-read-function that would
> make sense for tmm besides completing-read-default. Looking at the "git
> blame" output for tmm, that call to completing-read has definitely not been
> updated since completing-read-function was introduced except for minor
> bugfixes, so it makes sense that tmm would be expecting one and only one
> implementation of completing-read.
>
>
>> This kind of argument could (inappropriately, IMO) be
>> applied to any number of completely normal uses of
>> `completing-read'.
>>
>> I see no reason to impose a dichotomy of either a
>> `completing-read-function' similar to yours or else
>> `completing-read-default'.  There are likely other
>> benign values of the variable, besides just
>> `completing-read-default'.
>>
>
> I'm not trying to set a general precedent here. tmm is the only code that
> I'm aware of that uses completing-read in this way.
>
> It sounds like (and no, I haven't looked into it;
>> it just sounds like it) you have some particular
>> `completing-read-function' values in mind, which
>> you have found are incompatible with tmm's use of
>> `completing-read'.
>>
>
> The alternative completing-read-function providers that I am aware of are
> of are ido-ubiquitous (written by me), ivy, and helm. I'll also include
> icicles, even though uses some other mechanism besides modifying
> completing-read-function. ido-ubiquitous and ivy both have code to
> deactivate themselves when completing-read is called by tmm because
> otherwise their completion systems would break it, while icicles and helm
> simply break tmm when enabled. Thus, to my knowledge there is currently no
> other completing-read-function that doesn't break tmm (except for those
> that make exceptions specifically for tmm).
>
> If so, that's not an argument for preventing the use of
>> other values of `completing-read-function' with tmm.
>> (Clearly the value `completing-read-default' is fine,
>> for instance.)  That's not an argument for tmm to do
>> something to prevent all use of `completing-read-function'.
>>
>> Instead, it's an argument for the code that defines and
>> uses a particular `completing-read-function' to take
>> care of the contexts where it makes sense, and to stop
>> itself from being used in other contexts, where it might
>> cause trouble.
>>
>> Only that code knows the kinds of context where its own
>> `completing-read-function' makes sense and where it does
>> not.  Code such as tmm should not try to guess what kinds
>> of trouble different values of `completing-read-function'
>> might cause.
>>
>> I don't think tmm should throw up its hands and say, "Gee,
>> there might be some values of `completing-read-function'
>> that are troublesome, so let's just prevent all use of
>> that variable."  That doesn't make sense, to me.
>>
>
> Based on my explanation above, that is precisely what I think tmm should
> do: avoid using completing-read-function entirely. To look at it another
> way, tmm was originally written to use completing-read as an implementation
> detail, and later the function that used to be called completing-read was
> renamed to completing-read-default, but tmm was never updated to use the
> new name. This patch rectifies that.
>
>
>> If you want additional suggestions, maybe describe just
>> what the problem is that your completion function causes
>> for tmm.  It's hard to offer suggestions if you only
>> state that it is incompatible, without going into any
>> detail.  (Not that you must ask for input about this.
>> But if you would like some then giving more detail might
>> help.)
>>
>> Please use your own judgment (as I said, I don't really
>> care much about `tmm'), but a priori this sounds like
>> overkill.
>>
>> It sounds a bit like trying to bend Emacs to fit your
>> `completing-read-function'.  I can understand such a
>> motivation, believe me; I don't ascribe a bad intention
>> to you.  A guess is that you are not sure what to do,
>> to prevent inappropriate application of your value of
>> `completing-read-function' in this or that context.
>>
>> If you think it causes trouble in some contexts, or it
>> is not able to handle some contexts properly, then
>> I'd suggest you consider walling it off from those use
>> cases.  It might take some time to discover which
>> contexts it causes trouble for, but that's OK - you
>> could add them as you discover them.  Tmm sounds like
>> a start.
>
>
>> The right approach, IMO, is to teach your code when to
>> use its `completing-read-function' and when not to use
>> it.  Put differently, consider teaching your
>> `completing-read-function' when and where to hold back
>> and just punt to the default behavior.
>>
>
> This is exactly how ido-ubiquitous and ivy both currently work: they
> essentially have a blacklist of callers for which they revert to standard
> completion. tmm is on the blacklist for both packages. Certainly, for any
> alternative completion system there will be cases where it needs to fall
> back to standard completion. In my view, the completion system should be
> able to determine purely based on the calling context (i.e. its arguments
> and any relevant dynamically-bound variables) whether or not it needs to
> punt. Making this decision based on the name of the caller instead of the
> context to make this decision is admitting that not enough context was
> provided. I view it as a workaround, not a desirable design pattern, and
> someday in the future I hope to be able to remove the blacklist from
> ido-ubiquitous.
>
> In the case of tmm, the best heuristic I can think of would be to inspect
> the key bindings of all the letters and numbers. If any of them are locally
> rebound in the minibuffer to something other than self-insert-command, then
> punt. However, this doesn't work in practice because the bindings happen in
> minibuffer-setup-hook, so putting a check at the front of this hook is too
> early, and putting it at the end is too late because (in the case of
> ido-ubiquitous) an error is triggered before reaching the end of the hook.
> This was partly my motivation for suggesting the change in tmm rather than
> working around it in ido-ubiquitous: because the blacklist approach is the
> only way for ido-ubiquitous to fix it.
>
> It's obvious that it is possible for someone to create
>> a `completing-read-function' that causes trouble here
>> or there.  But such trouble is up to the causer to fix
>> or tame.
>>
>
> For the reasons described above, I would be very surprised if there was
> *any* alternative completion system that didn't break tmm.
>
> The approach of preventing code like `tmm.el' from
>> letting other code use `completing-read-function' does
>> not look like it heads in the right direction.  But
>> mine is just one opinion.  I ask only that you think
>> some more about this.
>>
>
> As mentioned above, tmm is the only code I'm aware of that does anything
> like this, and I don't see this as a general approach to be applied
> anywhere else.
>
> Hopefully the above clarifies my rationale in proposing this change.
>

[-- Attachment #2: Type: text/html, Size: 13082 bytes --]