João Távora <joaotavora@gmail.com> writes:

> I've pushed the patch with the fix and followed up with a simpler one
> that simply renames internal functions to be shorter and in-line with
> our convention for internal function names (it's icomplete--, not
> icomplete-vertical-- since icomplete-vertical.el doesn't exist).
>

Thanks! Much easiear to read.

> I've also noticed that you didn't update the hand-holding comments of
> icomplete--render-vertical, and that there's a suspicious double call to
> a new icomplete--ensure-visible-lines-inside-buffer function.  I'm
> fairly sure this is only relevant for `icomplete--in-region-buffer' but
> I think the code should look like this.  Can you try this patch after my
> sig?
>

Yeah it looks like it is duplicated, but there's a silly reason for
it. The first call could be removed with not much trouble. I'll explain
below.


--


I'm attaching another diff where I'm putting togheter some more stuff.

- Removed unecessary ':group' from the newer defcustoms and deffaces

- Made use of 'string-width' instead of lenght, so charts that might
  vary on 'length' are proper counted, like "⇒é".


These, together with a proper comenting of
`icomplete--ensure-vertical-completion-list-visible` was also suggested
by Stefan, so I changed your changes a bit trying to explain what I
meant by 'real lines' on
'icomplete--ensure-vertical-completion-list-visible' doc. Stefan told me
there are references to 'screen lines' and 'logical lines' elsewhere, I
tried to adapt this doc to follow this:

```
(defun icomplete--ensure-vertical-completion-list-visible ()
  "Ensure the vertical completion list is visible.
Scroll the window so that there are at least `icomplete-prospects-height'
screen lines (i.e., visual lines, including wrapped lines) available
below point.  Wrapped lines are counted individually."
````


Now for the good part. I already apologies for the headache.

> I'm
> fairly sure this is only relevant for `icomplete--in-region-buffer' but


Yes, you're right, the use of
`icomplete--ensure-vertical-completion-list-visible` is exclusively for
`icomplete--in-region-buffer`, thanks for removing the `minibufferp`
clause and adding `icomplete--in-region-buffer` where necessary.


In theory we could simply make use of a single entry like:

```
  (when (and icomplete-scroll
             icomplete--scrolled-completions
             (null icomplete--scrolled-past))

     ;; Here, it should work with only this
     (when icomplete--in-region-buffer
       (icomplete--ensure-vertical-completion-list-visible))
```


Problem is: there is a small inconsistency here, IF the cursor is
somewhere on the bottom of the buffer, where the list won't fit, and you
try to complete, this will only make it fit if we've scrolled icomplete
before. So, the inconsitence: this will not ajust if needed on the first
time you complete, but from the second forward. Could I live with a
simple C-g, C-M-i once every time I fire Emacs and my first complete
happened to be on the last line (thus needing scrolling)? Sure, but I
also would love not have to do it, so, keep following :)


Ok, so, what about putting it as you suggested, as the first step of
Loopapalooza?


There's some racing condition if we wanna do it here, it will work, but
it will always fire-up twice, moving the buffer way more than needed. I
couldn't figure it out a mean to make this work properly.


That's the reason we're calling it "twice", the first condition will
only fire up if it is the first (non scrolled, non icomplete used)
state.


The second entry is there 'from the second completion' forward.


I tried a plethora of combinations with
`icomplete--scrolled-completions` and friends, the one that covers more
cases without refactoring too much code was this one. This is now
explained on commentaries you can find on the diff.


Also, I fixed some minor bug on your last diff where you called
`icomplete--ensure-vertical-completion-list-visible` without checking
for `icomplete--in-region-buffer` first and icomplete-vertical was
crashing on minibuffer.


Please feel free to edit my diff, we could:

1. keep both calls, like I just did in the patch, that are complementary

2. keep only the second call, and 'live with a very small inconsistency'

3. try to patch something else and figure out what is causing the race
condition and how to tackle this.


Phew, that's it. Thanks again for taking the time to review these
changes :)