bug#12638: 24.2.50; FR: Some suggestions for icomplete-mode

[Stefan Monnier]

> It is not that patch is final.  It is worthy of further discussion and
> experimentation.

Than you.  Comments below.

> 1. `minibuffer-summarize-completions' is meant as a replacement for
>    `icomplete-exhibit'.  As the name suggests, it is meant to go in to
>     minibuffer.el.  It's presence in minibuffer.el proved problematic
>    (details in followup mail) and I had to move it to icomplete.el.

Why would you want to put it in minibuffer.el?

>    TODO: Handle key binding hints for sole command matches?  Is it
>    really necessary.  It seems so `old school'.  May be it made sense
>    when Emacs /did not/ provide key binding hints.

Agreed.  I don't like it very much, especially because it is very ad-hoc
(and worse, the code can be triggered in cases where it makes no sense
to provide those shortcuts).

> 2. There is a display overlay that is used in minibuffer (see
>    `minibuffer-message').  There is a counterpart in icomplete.el named
>    `icomplete-overlay'?
>    I am wondering whether `icomplete-overlay' could be thrown away and
>    have it use, yet to be introduced `minibuffer-overlay'.  Should this
>    be buffer-local etc etc. I am not sure of.
>    I can exchange notes if there is some interest around this area.

I haven't had time to look into it.  Let's keep it for later.

> 3. Apropos `minibuffer-force-complete-and-exit' and `confirm' etc.

>>> +(defun icomplete-this-match ()
>>> +  "Input first of the displayed matches to minibuffer prompt.
>>> +See `icomplete-matches'."
>>> +  (interactive)
>>> +  (delete-region (minibuffer-prompt-end) (point))
>>> +  (when icomplete-matches
>>> +    (insert (car icomplete-matches)))
>>> +  (exit-minibuffer))
>> I think it should still call test-completion and obey
>> minibuffer-completion-confirm if that test fails.
>   Can `test-completion' fail if the prompt is filled from valid
>   candidates?

Yes, very much so (think of completing a pdf filename, where the
completion will offer you directories, but those are only intermediate
states and are not valid final states).

> +(defvar icomplete-separator " | "
> +  "String used by icomplete to separate alternatives in the minibuffer.")

Better make it a defcustom.

> -(defcustom icomplete-minibuffer-setup-hook nil
> +(defcustom icomplete-minibuffer-setup-hook
> +  '((lambda ()
> +      (define-key icomplete-minibuffer-map
> +     "\C-j" 'minibuffer-force-complete-and-exit)
> +      (define-key icomplete-minibuffer-map
> +     "\C-s" 'minibuffer-forward-sorted-completions)
> +      (define-key icomplete-minibuffer-map
> +     "\C-r" 'minibuffer-backward-sorted-completions)))

Hooks should default to nil, with only *very* few exceptions.

Those define-key belong directly in the definition of
icomplete-minibuffer-map.

> -    (set (make-local-variable 'completion-show-inline-help) nil)
> +    ;; (set (make-local-variable 'completion-show-inline-help) nil)

Why?

> @@ -227,155 +259,192 @@
>    "Remove completions display \(if any) prior to new user input.
>  Should be run in on the minibuffer `pre-command-hook'.  See `icomplete-mode'
>  and `minibuffer-setup-hook'."
> -  (delete-overlay icomplete-overlay))
> +  (unless (memq this-command '(minibuffer-force-complete-and-exit 
> minibuffer-forward-sorted-completions
> +                                                 
> minibuffer-backward-sorted-completions))
> +    ;; Current command does not belong to icomplete-mode.
> +    ;; Delete the overlay.
> +    (delete-overlay icomplete-overlay)))

Why do you need such ad-hoc special case for those commands?
things like `this-command' are always brittle, so while it's often
unavoidable, I want to make sure it really is so.

> +(defun minibuffer-summarize-completions (&optional separator max-width
> +                                                first-match-face
> +                                                strip-common-substring)

Please move this back to where it was (right after icomplete-exhibit),
so that `diff' can show something a bit more useful.

>  ;;;_ > icomplete-exhibit ()
>  (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))
> -    (save-excursion
> -      (goto-char (point-max))
> +  (if (and icomplete-mode (icomplete-simple-completing-p))
> +      (save-excursion
> +     (goto-char (point-max))

Why turn this `when' into an `if'?

> @@ -1108,12 +1108,68 @@
>              (let ((hist (symbol-value minibuffer-history-variable)))
>                (setq all (sort all (lambda (c1 c2)
>                                      (> (length (member c1 hist))
> -                                       (length (member c2 hist))))))))
> +                                       (length (member c2 hist)))))))
> +         ;; Bring exact (but not unique) match to the front.
> +         (when (member string all)
> +           (push string all))

I see why this is often a good idea, but I'd be interested to hear
concrete use-cases where this was useful/needed.  The reason for it is
that the default ordering (shortest first) already should give you the
"exact but not unique is first".  And the other ordering rule (prefer
elements from the history) sounds just as valid as the one you suggest,
so I'm not sure why yours should take precedence.

Furthermore, the "exact but not unique first" is actually kind of
useless in the sense that the user can already just hit RET to get that
one, so she doesn't need much more help to access it.

> +         ;; Delete duplicates.
> +         (delete-dups all))

Sounds OK.

> +(defun minibuffer-force-complete-and-exit ()
> +  "Complete the minibuffer with first of the matches and exit."
> +  (interactive)
> +  ;; FIXME: Need to deal with the extra-size issue here as well.
> +  ;; FIXME: ~/src/emacs/t<M-TAB>/lisp/minibuffer.el completes to
> +  ;; ~/src/emacs/trunk/ and throws away lisp/minibuffer.el.
> +  (let* ((start (copy-marker (field-beginning)))
> +         (end (field-end))
> +         ;; (md (completion--field-metadata start))
> +         (all (completion-all-sorted-completions))
> +         (base (+ start (or (cdr (last all)) 0))))
> +    (cond
> +     ((not (consp all))
> +      (completion--message
> +       (if all "No more completions" "No completions")))
> +     ((not (consp (cdr all)))
> +      (let ((done (equal (car all) (buffer-substring-no-properties base 
> end))))
> +        (unless done (completion--replace base end (car all)))
> +        (completion--done (buffer-substring-no-properties start (point))
> +                          'finished (when done "Sole completion"))
> +     (exit-minibuffer)))
> +     (t
> +      (completion--replace base end (car all))
> +      (completion--done (buffer-substring-no-properties start (point))
> +                     'sole)
> +      (exit-minibuffer)))))

I'd suggest you simply call minibuffer-force-complete rather than
copying its code.  Also, after that call, just call
minibuffer-complete-and-exit (or rather, extract the body of
minibuffer-complete-and-exit to a new function which takes an addition
argument to know whether it should try to complete or not, and then call
this function from minibuffer-complete-and-exit (asking it to complete
if needed) as well as from minibuffer-force-complete-and-exit (asking
not to complete since we've just done it via minibuffer-force-complete).

> +(defun minibuffer-forward-sorted-completions ()

I think this should be in icomplete.el since this functionality doesn't
make much sense if completion-all-sorted-completions isn't displayed.

> +    (setcdr last (cons (car comps) (cdr last)))

You can use (push (car comps) (cdr last)) in Emacs-24.

> +      (setcdr last-but-one (cdr last))
> +      (push (car last) comps)

I think you can do (push (pop (cdr last-but-one)) comps).


        Stefan