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

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.

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'