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

[Stefan Monnier]

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

Are there behavior differences?  If so, can you summarize them (pun
intended) before I try to understand the code?

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

Why?

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

I'm not necessarily opposed to a rewrite, but I think such a rewrite
should aim to split it into smaller functions: the main problem with
icomplete-exhibit is that it's too large.

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

Is that intended to explain why you want to move it to minibuffer.el?
If so, I don't understand it.  Let's try to focus on improving icomplete
for now.  If/when some code in minibuffer.el could make use of some code
in icomplete.el it'll be easy enough to move the code.

If you move icomplete-exhibit (or its replacement) to minibuffer.el then
you may as well move the whole of icomplete.el to minibuffer.el, and I'm
not interested in doing that, for now.

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

If the variable doesn't work at all, then just remove it.  "Obsolete" is
used for things that are planned for removal but that still work.

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

I remember this part.  But if we keep such a reorganization for later,
we still need this `set' here, right?

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

Hmm... there shouldn't be any such visible flicker, even if we
delete-overlay and then re-add the overlay in post-command-hook
(because there shouldn't be any redisplay between the two).
Do you have a test case showing this problem?

Also, why include minibuffer-force-complete-and-exit in this list, since
it will exit the minibuffer so the post-command-hook won't be run, so
there won't be flicker anyway.

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

Right, which is why icomplete.el already displays it first, no matter
where it is in the list.

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

Then I'd rather not do it.  If you have a specific use-case where that
solves a misbehavior, then we can discuss it, but otherwise I'd rather
focus on the main task at hand: incremental improvements to bring
icomplete.el a bit closer to ido.el.


        Stefan