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

[Jambunathan K]

Quick summary:

1. `minibuffer-summarize-completions' is a re-write of
   `icomplete-exhibit' and line-by-line diff is not possible.

2. `minibuffer-summarize-completions' belongs in minibuffer.el and not
   icomplete.

I need your feedback on 1 and 2 before I re-work the patch. 

Below, I present "arguments" in favor of (1) and (2).

>> 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?

It converts completion to a string much along the lines of
`minibuffer-completion-help'.  For example, when number of completions
hit the threshold one can arrange a call to summarize function.  I think
it would be useful.

It is also in the same spirit as `completion--done' - a
"completion--not-done" so to speak.

>> +(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.

It wouldn't because it is re-written.  I am not sure whether you like
the re-write but it was definitely begging for re-write.  (Please
confirm your preference here, so that I know what is acceptable.)

You may want to focus on the `compare-strings' part where the
conditional accounts for `ignore-case' logic.

FWIW,

 `minibuffer-summarize-completions' does what `icomplete-exhibit'
does. But it has the "same structure" as `completion--do-completion' and 

- It doesn't try to complete or change the minibuffer.

- It doesn't call `minibuffer-help-message', `completion--done' or
  `completion-message'.  Instead it builds it's own string.

The new re-write would make it easy to extract the summary builder to
it's own "message function" potentially along the lines of
`:exit-function' and `:annotation-function'.

> > +(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.

Consistent if you are willing to accept my argument that
`minibuffer-summarize-completions' belongs and 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).

I will remove it in the next round and obsolete the corresponding
variable.

>
>> 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.
>
>> -    (set (make-local-variable 'completion-show-inline-help) nil)
>> +    ;; (set (make-local-variable 'completion-show-inline-help) nil)
>
> Why?

Just wanted to bring attention to the fact that icomplete uses it's own
overlay.  It is through this variable it shuts down minibuffer's very
own overlay.

>> @@ -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.

Avoids "flicker" in the minibuffer.  More aesthetic than functional, not
strictly necessary.



>> @@ -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.

While in buffer, history takes precedence over the length rule.  It is
possible that the exact match gets stacked deep in the summary.

Also when common substring is stripped, the exact match looks weird - it
becomes an empty string and is easily recognized when it is a first
item.

> 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))

The code merely moves the following condition in icomplete.el to it's
rightful place which is sorted completion itself. I have no opinion
whether the exact element should forcibly brought to the front.


-           (cond ((string-equal comp "") (setq most-is-exact t))
-                 ((member comp prospects))
-                 (t (setq prospects-len
-                           (+ (string-width comp) 1 prospects-len))
-                    (if (< prospects-len prospects-max)
-                        (push comp prospects)
-                      (setq limit t))))))