bug#16943: 24.3.50; [PATCH] `icomplete-exhibit' needs `with-current-buffer' for minibuffer

bug#16943: 24.3.50; [PATCH] `icomplete-exhibit' needs `with-current-buffer' for minibuffer

[Drew Adams]

I have noticed, with both my version (icomplete+.el) and vanilla
icomplete.el, that in some cases no match of the user input is found
because the field string gets picked up from buffer *Completions*, not
from the minibuffer.

This might be because in my case I use a separate frame for the
minibuffer and for *Completions* (the latter has its input redirected to
the former).

The fix that seems most appropriate, I think, is this: wrap the body of
the `icomplete-exhibit' code with this:

 (with-current-buffer (window-buffer (active-minibuffer-window))

E.g.:

(defun icomplete-exhibit ()
  "Insert Icomplete completions display.
Should be run via minibuffer `post-command-hook'.
See `icomplete-mode' and `minibuffer-setup-hook'."
  (when (and icomplete-mode
             (icomplete-simple-completing-p)) ;Shouldn't be necessary.
    (with-current-buffer (window-buffer (active-minibuffer-window))
      (save-excursion
        (goto-char (point-max))
...

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2014-03-04 on ODIEONE
Bzr revision: 116662 kbrown@cornell.edu-20140304190249-6s13s5bwn3un3hfe
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 'CFLAGS=-O0 -g3' 'CPPFLAGS=-DGC_MCHECK=1 -Ic:/Devel/emacs/include'
 LDFLAGS=-Lc:/Devel/emacs/lib'

bug#16943: 24.3.50; [PATCH] `icomplete-exhibit' needs `with-current-buffer' for minibuffer

[Stefan Monnier]

>  (with-current-buffer (window-buffer (active-minibuffer-window))

I think this would only paper over an underlying problem: icomplete uses
a buffer-local post-command-hook, so it makes no sense that it would be
run in another buffer.
A few possibilities:
- the hook is placed in the wrong buffer, in which case it would only be
  run in the wrong buffer.
- an earlier post-command-hook function changed current-buffer.
- an earlier minibuffer-setup-hook function changed current-buffer
  (which would cause the hook to be placed in the wrong buffer).


        Stefan

bug#16943: 24.3.50; [PATCH] `icomplete-exhibit' needs `with-current-buffer' for minibuffer

[Drew Adams]

> >  (with-current-buffer (window-buffer (active-minibuffer-window))
> 
> I think this would only paper over an underlying problem: icomplete uses
> a buffer-local post-command-hook, so it makes no sense that it would be
> run in another buffer.

`icomplete-exhibit' should not use a buffer other than the minibuffer.
But it apparently does sometimes.  Hence the fix: confine it to the
minibuffer.

(FWIW, it is text like this that gets picked up from *Completions*:
  "Click <mouse-2> on a completion to select it.")

IIRC, Icomplete started out using the minibuffer, but (in the same
(or perhaps a recursive?) minibuffer activation) it later was using
*Completions*.  I've kind of forgotten just how I was reproducing this.

> A few possibilities:
>
> - the hook is placed in the wrong buffer, in which case it would
>   only be run in the wrong buffer.
    ^^^^

Clearly the minibuffer, not *Completions*, is the current buffer
for *most* invocations of `icomplete-exhibit'.  Otherwise, it would
not work in general.  It is not run *only* in the wrong buffer, for
sure.

> - an earlier post-command-hook function changed current-buffer.
> - an earlier minibuffer-setup-hook function changed current-buffer
>   (which would cause the hook to be placed in the wrong buffer).

Even if such were the case, for whatever reason, `icomplete-exhibit'
should act on the text in the minibuffer.  It should look to the
minibuffer to do its job, regardless of whether some other buffer
happens to be current when it is invoked.  If the minibuffer is
active then its input text is there for Icomplete to read and
complete.

You have said that `icomplete-exhibit' should not be invoked unless
the minibuffer is current.  Maybe so.  But maybe not.  In the case
of my fix it now does the right thing: even if invoked with
*Completions* current, it picks up text from the minibuffer.
Would you instead have it do nothing in that case (e.g., by 
preventing from being invoked)?

Icomplete should complete minibuffer text, and so should be confined
to the minibuffer.  That does not imply that it should be invoked
only from the minibuffer, i.e., only when the minibuffer is the
current buffer.  That would be a stronger constraint, whose
necessity is not demonstrated.

What is necessary and sufficient is that it be able to do its job
when the minibuffer is active.  That's not the same thing as it
being *invoked* only with the minibuffer as current buffer.

In any case, `icomplete-exhibit', and the Icomplete code more
generally, should not have a say in what other things might or
might not do wrt `post-command-hook', including while the minibuffer
is active.

`icomplete-exhibit' is meant to operate in the minibuffer.
Perhaps some other commands on `post-command-hook' do not have
that constraint, and make sense regardless of the buffer.  Enabling
`icomplete-mode' should not prevent such commands from being invoked
by `post-command-hook' just because the minibuffer might be active.

It seems to me that the proposed fix is entirely appropriate:
as long as the minibuffer is active, `icomplete-exhibit' can do
its thing.  It just needs to be limited to doing it in the
minibuffer; that's all.

Control of Icomplete should be limited to what it does.  It should
focus on the minibuffer.  But that does not say anything about
`post-command-hook' in general while the minibuffer is active.

Feel free to debug this and come up with a different fix.  But
a priori I don't think that should involve "fixing" what other
commands on `post-command-hook' might do wrt the current buffer.

bug#16943: 24.3.50; [PATCH] `icomplete-exhibit' needs `with-current-buffer' for minibuffer

[Stefan Monnier]

> `icomplete-exhibit' should not use a buffer other than the minibuffer.

So you say.  Instead I say "icomplete-exhibit' should not be called in
a buffer other than the minibuffer".

And both are wrong if icomplete-in-buffer is non-nil, of course.

> But it apparently does sometimes.

And that implies there's a bug elsewhere.


        Stefan

bug#16943: 24.3.50; [PATCH] `icomplete-exhibit' needs `with-current-buffer' for minibuffer

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> `icomplete-exhibit' should not use a buffer other than the minibuffer.
>
> So you say.  Instead I say "icomplete-exhibit' should not be called in
> a buffer other than the minibuffer".
>
> And both are wrong if icomplete-in-buffer is non-nil, of course.

This all sounds like a bug in code not in Emacs, so I'm closing this bug
report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no