From: Drew Adams <drew.adams@oracle.com>
To: Ryan <rct@thompsonclan.org>, 27193@debbugs.gnu.org
Subject: bug#27193: 25.2; tmm should use completing-read-default
Date: Fri, 2 Jun 2017 08:26:32 -0700 (PDT) [thread overview]
Message-ID: <7241c3bc-d7eb-4ce3-998e-dcc21d54ef7f@default> (raw)
In-Reply-To: <edaafdef-8fb8-36f1-cdfa-5d74d3a4e9e9@thompsonclan.org>
> Below is a patch (formatted using "git format-patch") to avoid using
> "completing-read" in tmm, instead using "completing-read-default"
> directly. The rationale is explained in the commit message. Briefly:
> tmm already pretty much relies on the assumption that completing-read
> is actually calling completing-read-default.
>
> tmm uses completing-read, but it customizes its behavior so much that
> any alternative completing-read-function will almost certainly break
> it. For example, both ido-ubiquitous and ivy have special code to
> deactivate themselves for tmm. Since tmm is effectively imeplementing
> its own completion system, it should not depend on the value of
> completing-read-function.
I don't understand that argument. (But to be clear, I don't
really care much about `tmm'.)
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.
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'.
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'.
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.
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.
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.
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.
next prev parent reply other threads:[~2017-06-02 15:26 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-02 4:50 bug#27193: 25.2; tmm should use completing-read-default Ryan
2017-06-02 15:26 ` Drew Adams [this message]
2017-06-02 17:37 ` Ryan Thompson
2017-06-02 20:45 ` Ryan Thompson
2017-06-02 21:25 ` Drew Adams
2017-06-02 21:37 ` Dmitry Gutov
2017-06-02 22:19 ` Drew Adams
2017-06-02 22:24 ` Dmitry Gutov
2017-06-02 23:08 ` Ryan Thompson
2017-06-02 23:10 ` Ryan Thompson
2017-06-02 22:52 ` Ryan Thompson
2017-06-03 0:15 ` Drew Adams
2017-06-03 0:02 ` Glenn Morris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7241c3bc-d7eb-4ce3-998e-dcc21d54ef7f@default \
--to=drew.adams@oracle.com \
--cc=27193@debbugs.gnu.org \
--cc=rct@thompsonclan.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).