[PATCH v4 1/4] shell32/autocomplete: Implement a cache and sort the enumerated strings for proper behavior

[Gabriel Ivăncescu]

On Thu, Nov 1, 2018 at 11:57 AM Huw Davies <huw at codeweavers.com> wrote:
>
> Hopefully you agree that this looks much nicer than the previous
> versions.  There's a slight issue though in that you should check that
> the return value from IEnumString_Next() == S_OK.  You could simply
> then set n = 0 if that condition isn't met, to break out of the loops.
> Also, let's change 'n' -> 'read'.
>
Yeah, I actually thought about how to simplify it with a do...while,
when you gave me the idea to check for n != 0. :-)

>
> 'i' is superfluous, you can directly decrement 'cur'.
>
Sorry, just a habit of mine. I'm used to do iterations with such
variables rather than changing the "original" (in this case it doesn't
matter obviously).

>
> So this search loop looks like the code in find_first_matching...().
> I'd suggest making that function more general, and having a take a
> parameter 'BOOL direction' parameter that would find the last in the
> list if set.  You could also pass the start and end indicies to avoid
> unnecessary searching in this case.
>
Well if it's alright, I'll only add a parameter for the 'start' index,
because the 'end' is always the same in both cases; I want to avoid
adding too many parameters, because in a later pending patch it will
have to find based on potential prefix filtering (so I'll add another
parameter), and I want to avoid cluttering the call-site with too many
parameters which makes it ugly.

I can also simplify the function and just set "cmp" to "direction"
when they compare equal, so I don't have to duplicate the logic (since
that's the only situation in which the direction matters).

>
> There's still a lot of code shuffling in this patch in
> dispatch_matching_strs().  Can you try to reduce that by more patch
> splitting?  At the moment there's just too much going on to clearly
> see what this patch is doing.
>
I'm afraid not. Try to look at the whole function to get an idea why
it's done this way. Maybe git's diff made it seem like there's more
changes than a total rewrite of the function, but it's actually a
rewrite.

The previous code added items to the listbox as soon as they got
enumerated. In this one we add them at the end after we find the first
and last items (since they are sorted).

If you are referring to the else {} block, that just handles the case
where the len is zero and we display *all* the items. It has to be
done in this patch because it's specific to the enumeration, and it is
also needed to not break the behavior compared to before when len ==
0.

>
> So you moved this to a helper as I suggested and then moved it back in
> the next patch??  You could add a BOOL flag to hide_listbox that
> performs the reset if it's set.
>
> Huw.

If you are referring to the previous 2 patches, I didn't touch
hide_listbox at all. I can add a BOOL flag to reset, but I didn't want
to change every call of hide_listbox just for one or two cases that
need it set to FALSE.

That said there's still a problem with this: since the reset doesn't
exist before this patch, should I add a normal hide_listbox (with
"broken" behavior that always resets) in this patch, and then in the
next patch convert it to hide_listbox with a BOOL flag? Is that
alright, or every patch needs to be correct by itself?

If that's not acceptable (i.e. "temporary" slightly broken behavior),
I don't know how else to split it up; prior to this patch, there's no
"reset" at all. So every hide_listbox call-site would have to be
changed in this patch, which makes it larger... and I wanted to avoid
that.