HEAD: Bug in simple.el/minibuffer.el causing display-completion-list to delete not only word to be completed but everything in file before it

HEAD: Bug in simple.el/minibuffer.el causing display-completion-list to delete not only word to be completed but everything in file before it

[Arnold Schwaighofer]

Problem:
In current HEAD display-completion-list deletes not only word to be
completed but everything in file before it.

example:
(defun my-complete-test ()
  (interactive)
  (with-output-to-temp-buffer "*Completions*"
    (display-completion-list
       '("test1" "test2" "test345")
       "test")))

and call that function in a buffer on the word "test" (M-x
my-complete-test) with some text preceeding "test". deletes everything
before word "test" instead of just replacing it.

This also happens when using the complete-tag function :(.

My interpretation of the problem :).

minibuffer.el: display-completion-list
 (set (make-local-variable 'completion-base-size) (or (cdr last) 0))
sets completion-base-size to 0
which causes everything to be deleted before the word to be completed.

the problem
seems to be related to completion-setup-function in simple.el checking
for completion-base-size where i believe it should check for
completion-all-completions-with-base-size
is it possible that it should read:

Index: lisp/simple.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/simple.el,v
retrieving revision 1.924
diff -r1.924 simple.el
5527c5527
<       (unless completion-base-size
---
>       (unless completion-all-completions-with-base-size

after applying that patch completion works for me.

bug#163: marked as done (HEAD: Bug in simple.el/minibuffer.el causing display-completion-list to delete not only word to be completed but everything in file before it)

[Emacs bug Tracking System]

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


Your message dated Thu, 14 Aug 2008 20:16:05 -0400
with message-id <87ljyzuo8q.fsf@stupidchicken.com>
and subject line Re: 23.0.60; lisp-complete-symbol erases extra text
has caused the Emacs bug report #163,
regarding HEAD: Bug in simple.el/minibuffer.el causing display-completion-list to delete not only word to be completed but everything in file before it
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact don@donarmstrong.com
immediately.)


-- 
163: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=163
Emacs Bug Tracking System
Contact don@donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 6986 bytes --]

From: "Arnold Schwaighofer" <arnold.schwaighofer@gmail.com>
To: bug-gnu-emacs@gnu.org
Subject: HEAD: Bug in simple.el/minibuffer.el causing display-completion-list to delete not only word to be completed but everything in file before it
Date: Mon, 28 Apr 2008 14:22:25 +0200
Message-ID: <b8c6f2610804280522o68256194yb99e57eef4c33aee@mail.gmail.com>

Problem:
In current HEAD display-completion-list deletes not only word to be
completed but everything in file before it.

example:
(defun my-complete-test ()
  (interactive)
  (with-output-to-temp-buffer "*Completions*"
    (display-completion-list
       '("test1" "test2" "test345")
       "test")))

and call that function in a buffer on the word "test" (M-x
my-complete-test) with some text preceeding "test". deletes everything
before word "test" instead of just replacing it.

This also happens when using the complete-tag function :(.

My interpretation of the problem :).

minibuffer.el: display-completion-list
 (set (make-local-variable 'completion-base-size) (or (cdr last) 0))
sets completion-base-size to 0
which causes everything to be deleted before the word to be completed.

the problem
seems to be related to completion-setup-function in simple.el checking
for completion-base-size where i believe it should check for
completion-all-completions-with-base-size
is it possible that it should read:

Index: lisp/simple.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/simple.el,v
retrieving revision 1.924
diff -r1.924 simple.el
5527c5527
<       (unless completion-base-size
---
>       (unless completion-all-completions-with-base-size

after applying that patch completion works for me.




[-- Attachment #3: Type: message/rfc822, Size: 1141 bytes --]

From: Chong Yidong <cyd@stupidchicken.com>
To: "Drew Adams" <drew.adams@oracle.com>, "Arnold Schwaighofer" <arnold.schwaighofer@gmail.com>
Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 591-done@emacsbugs.donarmstrong.com, 163-done@emacsbugs.donarmstrong.com
Subject: Re: 23.0.60; lisp-complete-symbol erases extra text
Date: Thu, 14 Aug 2008 20:16:05 -0400
Message-ID: <87ljyzuo8q.fsf@stupidchicken.com>

I've checked in a fix for the lisp-complete-symbol erasing buffer text
problem.

bug#591: 23.0.60; lisp-complete-symbol erases extra text

[Drew Adams]

emacs -Q
 
In *scratch*, type this, and leave cursor after the final `for':
 
format-decode-buffer
forward-char
 
for
 
Then hit `M-TAB'. Choose one of the completions using mouse-2 in *Completions*.
 
Only the final `for' should be completed, but instead all of the text
in the buffer is replaced by the chosen completion.

This is a regression from Emacs 22.
 

In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-06-29 on LENNART-69DE564
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
-fno-crossjumping'
 

bug#591: marked as done (23.0.60; lisp-complete-symbol erases extra text)

[Emacs bug Tracking System]

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


Your message dated Thu, 14 Aug 2008 20:16:05 -0400
with message-id <87ljyzuo8q.fsf@stupidchicken.com>
and subject line Re: 23.0.60; lisp-complete-symbol erases extra text
has caused the Emacs bug report #591,
regarding 23.0.60; lisp-complete-symbol erases extra text
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact don@donarmstrong.com
immediately.)


-- 
591: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=591
Emacs Bug Tracking System
Contact don@donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 3248 bytes --]

From: "Drew Adams" <drew.adams@oracle.com>
To: <emacs-pretest-bug@gnu.org>
Subject: 23.0.60; lisp-complete-symbol erases extra text
Date: Mon, 21 Jul 2008 21:43:27 -0700
Message-ID: <003101c8ebb5$7c107380$0200a8c0@us.oracle.com>

emacs -Q
 
In *scratch*, type this, and leave cursor after the final `for':
 
format-decode-buffer
forward-char
 
for
 
Then hit `M-TAB'. Choose one of the completions using mouse-2 in *Completions*.
 
Only the final `for' should be completed, but instead all of the text
in the buffer is replaced by the chosen completion.

This is a regression from Emacs 22.
 

In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-06-29 on LENNART-69DE564
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
-fno-crossjumping'
 




[-- Attachment #3: Type: message/rfc822, Size: 1165 bytes --]

From: Chong Yidong <cyd@stupidchicken.com>
To: "Drew Adams" <drew.adams@oracle.com>, "Arnold Schwaighofer" <arnold.schwaighofer@gmail.com>
Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 591-done@emacsbugs.donarmstrong.com, 163-done@emacsbugs.donarmstrong.com
Subject: Re: 23.0.60; lisp-complete-symbol erases extra text
Date: Thu, 14 Aug 2008 20:16:05 -0400
Message-ID: <87ljyzuo8q.fsf@stupidchicken.com>

I've checked in a fix for the lisp-complete-symbol erasing buffer text
problem.

bug#591: 23.0.60; lisp-complete-symbol erases extra text

[Chong Yidong]

> emacs -Q
> 
> In *scratch*, type this, and leave cursor after the final `for':
> 
> format-decode-buffer
> forward-char
> 
> for
> 
> Then hit `M-TAB'. Choose one of the completions using mouse-2 in *Completions*.
> 
> Only the final `for' should be completed, but instead all of the text
> in the buffer is replaced by the chosen completion.

Hi Stefan,

This bug was introduced by your patch:

2008-04-13  Stefan Monnier  <monnier@iro.umontreal.ca>

 minibuffer.el (display-completion-list):
 Handle all-completions's new base-size info to set completion-base-size.

    (with-current-buffer standard-output
      ...
	(insert "Possible completions are:\n")
        (let ((last (last completions)))
          ;; Get the base-size from the tail of the list.
          (set (make-local-variable 'completion-base-size) (or (cdr last) 0))
          (setcdr last nil)) ;Make completions a properly nil-terminated list.
	(completion--insert-strings completions))))

This sets completion-base-size to 0, which causes the completions buffer
to delete everything in the Lisp buffer when you make a selection.

I don't understand why we need the (completely undocumented) hack
introduced here, where the cdr of the last item on the completions list
gives completion-base-size.  Does anything else in Emacs depend on this?
Why not simply add a new optional argument to display-completion-list?

bug#591: 23.0.60; lisp-complete-symbol erases extra text

[Drew Adams]

> From: Chong Yidong Sent: Tuesday, July 29, 2008 1:46 PM
> Hi Stefan,
> This bug was introduced by your patch:
>  2008-04-13  Stefan Monnier  <monnier@iro.umontreal.ca>
>  minibuffer.el (display-completion-list):
>  Handle all-completions's new base-size info to set 
>  completion-base-size.
> 
> This sets completion-base-size to 0, which causes the 
> completions buffer to delete everything in the Lisp
> buffer when you make a selection.
> 
> I don't understand why we need the (completely undocumented)
> hack introduced here, where the cdr of the last item on the
> completions list gives completion-base-size.  Does anything
> else in Emacs depend on this? Why not simply add a new
> optional argument to display-completion-list?

I won't pronounce on the hack of putting the base size into the cdr; only Stefan
knows if that is necessary and the right approach. I will say about that change
only that it required me to change my own code in a few places. For example, you
can no longer just apply `length' to the list, because it is no longer a true
list.

My reason for replying to your mail is instead to pass along the code that
Icicles uses for `choose-completion-string', in case it helps. I don't provide
it as a diff for straightforward patching, but rather as something you might
want to think about. The essential differences are these:

1. I use this:
(delete-region
  (+ base-size
     (if mini-p (minibuffer-prompt-end) (point-min)))
  (if mini-p (point-max) (point)))

instead of this:
(delete-region (+ base-size (field-beginning)) (point))

2. After deleting the region I do this:
(when mini-p (goto-char (point-max)))

Here is my definition:

(defun choose-completion-string (choice &optional buffer base-size)
  "Switch to BUFFER and insert the completion choice CHOICE.
BASE-SIZE, if non-nil, says how many characters of BUFFER's text
to keep.  If it is nil, we call `choose-completion-delete-max-match'
to decide what to delete.
If BUFFER is the minibuffer, then exit the minibuffer, unless one of
the following is true:
 - it is reading a file name, CHOICE is a directory, and
   `icicle-dir-candidate-can-exit-p' is nil
 - `completion-no-auto-exit' is non-nil
 - this is just a `lisp-complete-symbol' completion."
  (let* ((buffer (or buffer completion-reference-buffer))
         (mini-p (minibufferp buffer)))
    ;; If BUFFER is a minibuffer, barf unless it's currently active.
    (if (and mini-p
             (or (not (active-minibuffer-window))
                 (not (equal buffer (window-buffer
                                     (active-minibuffer-window))))))
        (error "Minibuffer is not active for completion")
      ;; Set buffer so buffer-local `choose-completion-string-functions' works.
      (set-buffer buffer)
      (unless (run-hook-with-args-until-success
               'choose-completion-string-functions
               choice buffer mini-p base-size)
        ;; Insert completion into buffer where completion was requested.
        (if base-size
            (delete-region
             (+ base-size (if mini-p (minibuffer-prompt-end) (point-min)))
             (if mini-p (point-max) (point)))
          (choose-completion-delete-max-match choice))
        (when mini-p (goto-char (point-max)))
        (insert choice)
        (remove-text-properties
         (- (point) (length choice)) (point) '(mouse-face nil))
        ;; Update point in the window that BUFFER is showing in.
        (let ((window (get-buffer-window buffer 0)))
          (set-window-point window (point)))
        ;; If completing for the minibuffer, exit it with this choice,
        ;; unless this was a `lisp-complete-symbol' completion.
        (and (not completion-no-auto-exit)
             (equal buffer (window-buffer (minibuffer-window)))
             (or minibuffer-completion-table
                 (and icicle-mode
                      (or icicle-extra-candidates icicle-proxy-candidates)))
             (not (eq 'lisp-complete-symbol icicle-cmd-calling-for-completion))
             ;; Exit the minibuffer if `icicle-dir-candidate-can-exit-p',
             ;; or not reading a file name, or chosen file is not a directory.
             (if (or icicle-dir-candidate-can-exit-p
                     (not (eq minibuffer-completion-table
                              'read-file-name-internal))
                     (not (file-directory-p (field-string (point-max)))))
                 (exit-minibuffer)
               (let ((mini (active-minibuffer-window)))
                 (select-window mini)
                 (when minibuffer-auto-raise
                   (raise-frame (window-frame mini))))))))))

HTH - Drew

bug#591: 23.0.60; lisp-complete-symbol erases extra text

[Stefan Monnier]

>> In *scratch*, type this, and leave cursor after the final `for':
>> 
>> format-decode-buffer
>> forward-char
>> 
>> for
>> 
>> Then hit `M-TAB'. Choose one of the completions using mouse-2 in *Completions*.
>> 
>> Only the final `for' should be completed, but instead all of the text
>> in the buffer is replaced by the chosen completion.

> Hi Stefan,
> This bug was introduced by your patch:

Yes, I know.

> This sets completion-base-size to 0, which causes the completions buffer
> to delete everything in the Lisp buffer when you make a selection.

The problem is that leaving it nil will revert to the use of
a heuristic.  Fixing it right is a bit more difficult.

> I don't understand why we need the (completely undocumented) hack
> introduced here, where the cdr of the last item on the completions list
> gives completion-base-size.  Does anything else in Emacs depend on this?
> Why not simply add a new optional argument to display-completion-list?

Yes the base-size in the cdr is a hack, and we be able to get rid of it
now that I've added the new `boundaries' action.  But passing it as an
additional argument won't make any difference for the bug at hand.


        Stefan

bug#591: 23.0.60; lisp-complete-symbol erases extra text

[Chong Yidong]

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

>> This sets completion-base-size to 0, which causes the completions buffer
>> to delete everything in the Lisp buffer when you make a selection.
>
> The problem is that leaving it nil will revert to the use of
> a heuristic.  Fixing it right is a bit more difficult.

I don't understand what you mean :-P

>> I don't understand why we need the (completely undocumented) hack
>> introduced here, where the cdr of the last item on the completions list
>> gives completion-base-size.  Does anything else in Emacs depend on this?
>> Why not simply add a new optional argument to display-completion-list?
>
> Yes the base-size in the cdr is a hack, and we be able to get rid of it
> now that I've added the new `boundaries' action.  But passing it as an
> additional argument won't make any difference for the bug at hand.

Why not?  That would allow lisp-complete-symbol to pass the correct
value of completion-base-size, which it has already computed (i.e. the
variable `beg' in lisp-complete-symbol) to display-completion-list.  No?

bug#591: 23.0.60; lisp-complete-symbol erases extra text

[Drew Adams]

> From: Stefan Monnier Sent: Tuesday, July 29, 2008 3:08 PM
> > Hi Stefan, This bug was introduced by your patch:
> 
> Yes, I know.
> 
> > This sets completion-base-size to 0, which causes the completions
> > buffer to delete everything in the Lisp buffer when you make a
> > selection.
> 
> The problem is that leaving it nil will revert to the use of
> a heuristic.  Fixing it right is a bit more difficult.
> 
> > I don't understand why we need the (completely undocumented) hack
> > introduced here, where the cdr of the last item on the 
> > completions list gives completion-base-size.  Does anything else
> > in Emacs depend on this? Why not simply add a new optional
> > argument to display-completion-list?
> 
> Yes the base-size in the cdr is a hack, and we be able to get 
> rid of it now that I've added the new `boundaries' action.  But
> passing it as an additional argument won't make any difference for
> the bug at hand.

This hack still seems to be present in the code from a build today:

In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-10-03 on LENNART-69DE564
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
-fno-crossjumping'

The cdr still seems to be used for the base size.

 From completion-all-completions:

  (let ((completion-all-completions-with-base-size t))

 From completion-table-with-context:

  ;; In case of non-empty all-completions,
  ;; add the prefix size to the base-size.
  ((consp comp)
   (let ((last (last comp)))
     (when completion-all-completions-with-base-size
       (setcdr last (+ (or (cdr last) 0) (length prefix))))
     comp))

 From completion--file-name-table:

  (if (and completion-all-completions-with-base-size (consp all))
      ;; Add base-size, but only if the list is non-empty.
      (nconc all base-size)
    all)

This hack prevents code from treating the list as a true list - e.g. applying
`length' to it. If, as you say, the hack is no longer needed and we are "able to
get rid of it now", could you please remove it?

Also, there is still no doc string for some of the more important functions in
this library - e.g. completion-table-with-context,
completion-table-with-terminator, completion-all-sorted-completions,
completion-hilit-commonality, completion-basic-try-completion,
completion-basic-all-completions, completion-pcm-all-completions,
completion-pcm-try-completion.

Likewise, no doc string for less important functions - e.g.
completion--try-word-completion, minibuffer--bitset,
completion--flush-all-sorted-completions, minibuffer--double-dollars,
completion--make-envvar-table, completion--embedded-envvar-table,
completion-emacs21-try-completion, completion-emacs21-all-completions,
completion-emacs22-try-completion, completion-emacs22-all-completions,
completion-pcm--prepare-delim-re, completion-pcm--pattern-trivial-p,
completion-pcm--pattern->regex, completion-pcm--hilit-commonality,
completion-pcm--pattern->string, completion-pcm--merge-try.

Likewise, no doc string for variables - e.g. completion-all-sorted-completions,
completion-common-substring, completion--embedded-envvar-re,
completion-pcm--delim-wild-regex.

Thx.