[PATCH v5 3/7] shell32/autocomplete: Redesign the window proc to trigger on key presses instead of key release

[Huw Davies]

> On 13 Sep 2018, at 20:12, Gabriel Ivăncescu <gabrielopcode at gmail.com> wrote:
> 
> On Thu, Sep 13, 2018 at 6:47 PM, Gabriel Ivăncescu
> <gabrielopcode at gmail.com> wrote:
>> On Thu, Sep 13, 2018 at 1:37 PM, Huw Davies <huw at codeweavers.com> wrote:
>>> 
>>> Not that it really matters, because this isn't going to go in as it is, but
>>> having noautoappend as a tri-state is fine, except you can't declare it as a
>>> BOOL and use FALSE, TRUE and 2 as its states.  You'd want an enum.
>>> 
>>> Huw.
>>> 
>> 
>> I have two questions about the enum. First, is there some sort of
>> naming convention that should be used to avoid future clashes with
>> public definitions (headers)? For example, I know that using all-caps
>> for enum values is common, but that can easily conflict with future
>> public macros or the like, so I personally dislike it. Of course, the
>> Windows API doesn't really use underscores like this_is_an_enum_value
>> for its public definitions, so maybe I should go with that approach?
>> (e.g. noautoappend_displayall as enum value? or some other naming
>> scheme?). Basically I'm asking how to best make the enum "private" for
>> internal linkage purposes in terms of naming convention.
>> 
>> Second question is, how should I tell the enum that any value other
>> than zero is "no auto append", including the "displayall" part? (to
>> simplify the code checks since it implies it) For example currently i
>> just check if it's FALSE, because both TRUE and 2 means "don't auto
>> append" (while 2 further means displayall at the beginning). How to
>> best proceed with an enum there? Maybe I should use a short helper
>> function with the enum as parameter that returns BOOL whether it
>> should auto-append or not?
> 
> Here's what I have so far:
> 
> enum autoappend_flag
> {
>    autoappend_flag_yes = 0,
>    autoappend_flag_no = 1,
>    autoappend_flag_displayempty = -1
> };

Since the actual values are irrelevant don't assign them, just
let the compiler pick the defaults.

> static BOOL autoappend_flag_enabled(enum autoappend_flag flag)
> {
>    /* "no" and "displayempty" don't autoappend, only "yes" does */
>    return flag == autoappend_flag_yes;
> }
> 
> Is this fine like this? I find it natural with accessor but of course
> that's just me :-)

You're making this more complicated than it needs be; testing for
the enabled case is as simple as:

if (flag == autoappend_flag_yes)

there's no need to wrap this in a helper.

Huw.