On Thu, Nov 1, 2018 at 11:57 AM Huw Davies huw@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.