From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Ryan Thompson Newsgroups: gmane.emacs.bugs Subject: bug#27193: 25.2; tmm should use completing-read-default Date: Fri, 02 Jun 2017 17:37:18 +0000 Message-ID: References: <7241c3bc-d7eb-4ce3-998e-dcc21d54ef7f@default> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="001a113fe2da82342f0550fd9bbc" X-Trace: blaine.gmane.org 1496425096 22707 195.159.176.226 (2 Jun 2017 17:38:16 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 2 Jun 2017 17:38:16 +0000 (UTC) To: Drew Adams , 27193@debbugs.gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Jun 02 19:38:12 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dGqWZ-0005g5-LK for geb-bug-gnu-emacs@m.gmane.org; Fri, 02 Jun 2017 19:38:11 +0200 Original-Received: from localhost ([::1]:50964 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dGqWf-0005cm-2y for geb-bug-gnu-emacs@m.gmane.org; Fri, 02 Jun 2017 13:38:17 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:49693) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dGqWU-0005Nx-Fu for bug-gnu-emacs@gnu.org; Fri, 02 Jun 2017 13:38:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dGqWR-0002sK-6z for bug-gnu-emacs@gnu.org; Fri, 02 Jun 2017 13:38:06 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:49167) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dGqWQ-0002qU-Ui for bug-gnu-emacs@gnu.org; Fri, 02 Jun 2017 13:38:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dGqWQ-000499-5x for bug-gnu-emacs@gnu.org; Fri, 02 Jun 2017 13:38:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Ryan Thompson Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 02 Jun 2017 17:38:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 27193 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 27193-submit@debbugs.gnu.org id=B27193.149642505715903 (code B ref 27193); Fri, 02 Jun 2017 17:38:02 +0000 Original-Received: (at 27193) by debbugs.gnu.org; 2 Jun 2017 17:37:37 +0000 Original-Received: from localhost ([127.0.0.1]:51844 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dGqW0-00048R-PO for submit@debbugs.gnu.org; Fri, 02 Jun 2017 13:37:37 -0400 Original-Received: from mail-it0-f49.google.com ([209.85.214.49]:37174) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dGqVz-00048E-B6 for 27193@debbugs.gnu.org; Fri, 02 Jun 2017 13:37:36 -0400 Original-Received: by mail-it0-f49.google.com with SMTP id m47so25111797iti.0 for <27193@debbugs.gnu.org>; Fri, 02 Jun 2017 10:37:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=thompsonclan-org.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=DKmsyb0RIEW/mEVjt04zuweYOQNxwNIcSHCM+82o0JE=; b=DqDDyjEusd37A7OlhaLuU0KJLbSa2HW62C/7REUnNjeqM7I2EcPWkzJT8fyM+Czb4y 6/l7/OB2hugA81y+xVv+x4CpinHjOja/FasYvspByh2PX+4sSoM/vlCm1xSRo8aLv342 k+RVU89ktsXu90SSQPFGCcxVTe0EHpmGgRUI+wStxw677NY0C6D1fOkb2iR+asQ1nBV7 5Hqgg72doMgcLMVtY1bPoUuNGoUTvzWRc6Y6yNMkxWBdlNZQv8SvjmEqNWaZQ1gUfA6R /DKT5VZDMPYfACgK0JGj5hpat2o2a4vsrs6J/EWtcTY8GtsdqvyQ1ydQy+IdzU00N1lq LrlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=DKmsyb0RIEW/mEVjt04zuweYOQNxwNIcSHCM+82o0JE=; b=kjrfA0kWp8dKohu+Ic1rpjAK2NFeF55K/qz99a7UH81OdpMkvRPto94YSHIW7tcP0t TDqxQo7MnoQax9wjDe55nDdWxalBZH6MqB0rU80NXb98VjRtHzjRjUn6yZAQp2drXPUj uuOzPp0qJnJPa7S0+YcUK1BGjnY/KLUl7ccc6pxTWa3wOn1o8RcLLC4lZQ8HNtPwuUyZ pUaSnLvjK+50F86tHO98sZ1B89Jig023x5jXw1dnaoOHndBFTBMqHdXLB/uLhGXegPr1 O98WlIIgaKsk4b1IgzbhBxoyRRZcoqb2qfmxJOoMBSXNVwkZWjO2cmIrGeZzWD1Emggx nvxQ== X-Gm-Message-State: AODbwcC4Lzg74EmUuLAu1NzDn77KOyNo4B4DUKa1z+ejB1kCPEtULxgb jmLORTjqGEPgPtAWHDP9VAZYl6IkeVd6 X-Received: by 10.107.25.203 with SMTP id 194mr8870399ioz.182.1496425049496; Fri, 02 Jun 2017 10:37:29 -0700 (PDT) In-Reply-To: <7241c3bc-d7eb-4ce3-998e-dcc21d54ef7f@default> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:133168 Archived-At: --001a113fe2da82342f0550fd9bbc Content-Type: text/plain; charset="UTF-8" On Fri, Jun 2, 2017 at 11:26 AM Drew Adams 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. --001a113fe2da82342f0550fd9bbc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri= , Jun 2, 2017 at 11:26 AM Drew Adams <drew.adams@oracle.com> wrote:
I don't understand that argument.=C2=A0 (But to be clear, I do= n't
really care much about `tmm'.)
=C2=A0
Fa= ir enough. My post certainly assumes quite a bit familiarity with tmm. I= 9;m happy to elaborate below.
=C2=A0
I don't see that that is an argument for _preventing_ the
use of a `completing-read-function' by tmm.=C2=A0 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 `comple= ting-read', I don't see that there
is a problem to be fixed in `= tmm.el'.=C2=A0 I don't see that
its use of `completing-read'= is particularly exotic or
problematic.=C2=A0 This is it:

(comple= ting-read
=C2=A0(concat gl-str " (up/down to change, PgUp to menu):= ")
=C2=A0(tmm--completion-table (reverse tmm-km-list)) nil t nil=C2=A0(cons 'tmm--history (- (* 2 history-len) index-of-default)))
I don't see anything at all unusual about that.

And the col= lection function, `tmm--completion-table',
is likewise pretty ordina= ry, I think:

(defun tmm--completion-table (items)
=C2=A0 (lambda = (string pred action)
=C2=A0 =C2=A0 (if (eq action 'metadata)
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 '(metadata (display-sort-function . identity))=
=C2=A0 =C2=A0 =C2=A0 (complete-with-action action items string pred))))=

You say:

> tmm already pretty much relies on the assumpti= on 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. T= hat completing-read call is wrapped by "(minibuffer-with-setup-hook #&= #39;tmm-add-prompt ...)", which in turn calls=C2=A0tmm-define-keys, wh= ich 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, r= ather than completion of a substring with string matching. The result is no= t even recognizable as completing-read. The use of completing-read is merel= y an implementation detail in a system that is *not* actually doing complet= ion 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-functio= n 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 on= e and only one implementation of completing-read.


This kind of argument coul= d (inappropriately, IMO) be
applied to any number of completely normal u= ses of
`completing-read'.

I see no reason to impose a dichoto= my of either a
`completing-read-function' similar to yours or else`completing-read-default'.=C2=A0 There are likely other
benign val= ues 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 so= unds like it) you have some particular
`completing-read-function' va= lues in mind, which
you have found are incompatible with tmm's use o= f
`completing-read'.

The alterna= tive completing-read-function providers that I am aware of are of are ido-u= biquitous (written by me), ivy, and helm. I'll also include icicles, ev= en though uses some other mechanism besides modifying completing-read-funct= ion. ido-ubiquitous and ivy both have code to deactivate themselves when co= mpleting-read is called by tmm because otherwise their completion systems w= ould break it, while icicles and helm simply break tmm when enabled. Thus, = to my knowledge there is currently no other completing-read-function that d= oesn't break tmm (except for those that make exceptions specifically fo= r tmm).

If so, that's not an argument for preventing the use of
other val= ues of `completing-read-function' with tmm.
(Clearly the value `comp= leting-read-default' is fine,
for instance.)=C2=A0 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 defi= nes and
uses a particular `completing-read-function' to take
care= of the contexts where it makes sense, and to stop
itself from being use= d 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.=C2=A0 Code such as tmm should not tr= y to guess what kinds
of trouble different values of `completing-read-fu= nction'
might cause.

I don't think tmm should throw up it= s hands and say, "Gee,
there might be some values of `completing-re= ad-function'
that are troublesome, so let's just prevent all use= of
that variable."=C2=A0 That doesn't make sense, to me.

Based on my explanation above, that is preci= sely what I think tmm should do: avoid using completing-read-function entir= ely. To look at it another way, tmm was originally written to use completin= g-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.
=C2= =A0
If you want addi= tional suggestions, maybe describe just
what the problem is that your co= mpletion function causes
for tmm.=C2=A0 It's hard to offer suggestio= ns if you only
state that it is incompatible, without going into any
= detail.=C2=A0 (Not that you must ask for input about this.
But if you wo= uld like some then giving more detail might
help.)

Please use you= r 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 tr= ying to bend Emacs to fit your
`completing-read-function'.=C2=A0 I c= an understand such a
motivation, believe me; I don't ascribe a bad i= ntention
to you.=C2=A0 A guess is that you are not sure what to do,
t= o prevent inappropriate application of your value of
`completing-read-fu= nction' in this or that context.

If you think it causes trouble = in some contexts, or it
is not able to handle some contexts properly, th= en
I'd suggest you consider walling it off from those use
cases.= =C2=A0 It might take some time to discover which
contexts it causes trou= ble for, but that's OK - you
could add them as you discover them.=C2= =A0 Tmm sounds like
a start.=C2=A0

The right approach, IMO, is to teach your code= when to
use its `completing-read-function' and when not to use
i= t.=C2=A0 Put differently, consider teaching your
`completing-read-functi= on' when and where to hold back
and just punt to the default behavio= r.

This is exactly how ido-ubiquitous a= nd ivy both currently work: they essentially have a blacklist of callers fo= r which they revert to standard completion. tmm is on the blacklist for bot= h packages. Certainly, for any alternative completion system there will be = cases where it needs to fall back to standard completion. In my view, the c= ompletion system should be able to determine purely based on the calling co= ntext (i.e. its arguments and any relevant dynamically-bound variables) whe= ther 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 des= ign pattern, and someday in the future I hope to be able to remove the blac= klist 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 minibuff= er to something other than self-insert-command, then punt. However, this do= esn'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 puttin= g it at the end is too late because (in the case of ido-ubiquitous) an erro= r is triggered before reaching the end of the hook. This was partly my moti= vation for suggesting the change in tmm rather than working around it in id= o-ubiquitous: because the blacklist approach is the only way for ido-ubiqui= tous to fix it.

It's obvious that it is possible for someone to create
a = `completing-read-function' that causes trouble here
or there.=C2=A0 = 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 u= se `completing-read-function' does
not look like it heads in the rig= ht direction.=C2=A0 But
mine is just one opinion.=C2=A0 I ask only that = you think
some more about this.
=C2=A0
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 any= where else.=C2=A0

Hopefully the above clarifies my= rationale in proposing this change.
--001a113fe2da82342f0550fd9bbc--