Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Gregory Heytings <gregory@heytings.org>]

>>> I didn't get a terrible amount of feedback, but I spent the week 
>>> testing that branch, making small adjustments, and verifying that the 
>>> default Icomplete and Fido behaviour are unchanged.  So I've now 
>>> pushed the icomplete-vertical-mode rework to master.
>>
>> Okay, so I guess it's too late to send you feedback...  I asked you to 
>> wait a few days more, and promised to send you feedback this week.
>
> You can still send feedback, of course.  Just because it's in master 
> doesn't mean it's written in stone.
>

That's most often the case, but let's see whether this one is an 
exception.

>
> I did wait a few days more to your request but I didn't think it made 
> sense to wait many more as I need to move on to other developments that 
> kind of depend on it. Anyway, I think you'll like the new mode, but let 
> me know what you think.
>

I've now spent some time to test the feature and to look at your code, and 
here are a few comments:

1. Your code is not modular enough, for example, IMO it would be much 
better if the the annotation function and the "candidate number/total 
number of candidates" indication were (user-configurable) hooks, instead 
of hard-coding these operations in the code.  Not everyone wants to see 
these numbers, not everyone wants to see annotations even if they are 
available, some might want to add annotations to the candidates list with 
their own chosen formatting, and so forth.

2. I don't understand why you used "icomplete-scroll" for the variable 
name, and why it is not a defcustom.  IMO it should have been something 
more explicit like "icomplete-vertical-scroll-candidate-list".  And with a 
hook, this variable would not be necessary.

3. As I feared, the code to scroll the candidates list does not work 
correctly alas.  I've put lots of efforts to make "icomplete-vertical" 
work correctly in virtually every possible case, so I find this very 
regrettable.  For example, with frame-height = 47, 10 candidates are 
displayed, yet the candidates list rotates on the 7th candidate (instead 
of the 11th one).  With frame-height = 30, 6 candidates are displayed, and 
the candidates list rotates on the 6th candidate (instead of the 7th one). 
With frame-height = 22, 4 candidates are displayed, and the candidates 
list correctly rotates on the 5th candidate.  With frame-height = 15, two 
candidates are displayed, yet the candidates list rotates on the 4th 
candidate (instead of the 3th one, and in this case you don't even see the 
current candidate anymore).

If I start working on this (again), I would likely change most of your 
code into something else, and it would take some time, because it's a 
difficult problem.  Would you agree with this?