[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#12638: 24.2.50; FR: Some suggestions for icomplete-mode
From: |
Stefan Monnier |
Subject: |
bug#12638: 24.2.50; FR: Some suggestions for icomplete-mode |
Date: |
Fri, 09 Nov 2012 09:12:04 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) |
> 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