[PATCH v4 07/10] shell32/autocomplete: Redesign the window proc to trigger on key presses instead of key release

[Huw Davies]

> On 11 Sep 2018, at 18:48, Gabriel Ivăncescu <gabrielopcode at gmail.com> wrote:
> 
> On Tue, Sep 11, 2018 at 8:22 PM, Huw Davies <huw at codeweavers.com> wrote:
>> 
>> Specifically the gotos, but I've already commented on the high
>> identation level in this function (I know that's not your fault).
>> 
>> It seems to me that you want the WM_KEYDOWN handler to call the
>> WM_CHAR handler.  At the risk of repeating myself, having the handlers
>> as a helper functions would make this easier.
>> 
>> Huw.
> 
> I honestly don't think the gotos are too bad (especially the VK_DELETE
> one since it's also very close), and my reasoning for it is because
> all the variables that could be affected are set right before the goto
> (ret, displayall, noautoappend), so there's no state to memorize,
> because every other variable is set before both switch statements
> (including parameters).
> 
> However, that probably won't convince you, so I'll maybe end up having
> to rewrite this, but would a larger patch be acceptable then? I mean,
> to get it in a "workable" state it's pretty hard to split it up.
> 
> That said there are still some difficulties with functions instead of
> goto, since the goto doesn't jump directly into WM_CHAR but a
> sub-block depending on condition. It is easy to make most of WM_CHAR a
> separate function, but not the original if blocks. For example,
> VK_DELETE's wParam is actually not a "control character" in terms of
> WM_CHAR's wParam, yet it needs to use that same code path (that's why
> it jumps inside the if statement, to share the code). Maybe I can give
> an extra bool argument to the helper function, to know whether to go
> the "control character" route or not. Definitely not as elegant in my
> opinion, but if it has to be done heh.
> 
> But yeah, I won't be able to do this without rewriting it...
> 
> 
> BTW just wondering but what is exactly bad about the goto in this
> situation? (just the first one or both?) Of course, I know it's not
> ideal, but what makes it so bad I mean? I'm one of those who dislikes
> littering code with BOOLs and have a lot of checks just to avoid some
> gotos (which make code cleaner in that situation), so it's not obvious
> to me due to that background, that's why I'd like to know :-)

It makes following the flow of the code incredibly difficult.  If you
don't believe me, come back to this patch in a month and see how you feel
then.

You'll probably find it easier to first move the existing WM_KEYUP code to a
helper function as essentially a no-op patch.  That patch will be big, but as
it's a cut-n-paste job that won't matter.  Then you can start pulling it apart.

Huw.