[PATCH] dinput/tests: Make overlapped format tests more robust.

[Arkadiusz Hiler]

On Wed, Aug 04, 2021 at 11:44:14PM +0200, Rémi Bernon wrote:
> On 8/4/21 11:00 PM, Arkadiusz Hiler wrote:
> > On Wed, Aug 04, 2021 at 02:25:49PM -0500, Zebediah Figura (she/her) wrote:
> > > On 8/4/21 2:20 PM, Rémi Bernon wrote:
<SNIP>
> > > > > +    tries = 2;
> > > > > +    do
> > > > > +    {
> > > > > +        /* press D */
> > > > > +        keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
> > > > > +        pump_messages();
> > > > > +    } while (!wait_for_device_data_and_discard(keyboard) && tries--);
> > > > >        memset(&state, 0xFF, sizeof(state));
> > > > >        hr = IDirectInputDevice_GetDeviceState(keyboard,
> > > > > sizeof(state), &state);
> > > > 
> > > > I think it could be worth having a dedicated helper to acquire and wait,
> > > > instead of inlining these loops. The same helper could then be (copied)
> > > > and used in the other tests.
> > > > 
> > > > The tries counter and its hardcoded value is a bit confusing at first,
> > > > and instead of keeping the key pressed the helper could also make sure
> > > > it's released on function exit (maybe use a dedicated unusual key for
> > > > that?).
> > 
> > "Unusual key" may be hard to pick as AFAIU it has to be present in the
> > data format.
> > 
> > How about something like:
> > 
> > static void flush_acquire_keyboard_(int line, IDirectInputDeviceA *device, DWORD valid_dik)
> > {
> >      int tries = 2;
> >      do
> >      {
> >          keybd_event(0, valid_dik, KEYEVENTF_SCANCODE, 0);
> >      } while (!wait_for_device_data_and_discard(device) && tries--);
> > 
> >      keybd_event(0, valid_dik, KEYEVENTF_SCANCODE|KEYEVENTF_KEYUP, 0);
> >      ok_(__FILE__, line)(wait_for_device_data_and_discard(device),
> >              "Timed out while waiting for injected events to be picked up by DirectInput.\n");
> > }
> > 
> > #define flush_acquire_keyboard(device, valid_dik) flush_acquire_keyboard_(__LINE__, device, valid_dik)
> > 
> > And then we can:
> > 
> > hr = IDirectInputDevice_Acquire(keyboard);
> > ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed: %08x\n", hr);
> > flush_acquire_keyboard(keyboard, DIK_F);
> > 
> 
> Why not even call Acquire inside the helper (and name it acquire_and_wait or
> something like that)?

Doesn't really matter. I have a slight preference for slim helpers that
do only one thing.

> > We would also need a mouse variant.
> > 
> 
> You could inject mouse events too, but if the helper is copied in other test
> sources then it's probably better to use a slightly different version with
> mouse events instead.
> 
> > I wonder if instead of copying it verbatim all over the place it would
> > be okay to create dlls/dinput/tests/event_helpers.h and include it from
> > the dinput8 tests.
> > 
> 
> I'm not sure it's worth a header to avoid coping it to a few files
> (especially considering we may rather have two helpers for mouse /
> keyboard). And I don't think it's a good idea to make dinput8 tests include
> files from another test folder so at least it would be duplicated (also
> because the device interface type is different).

Since we are going to copy it over mouse events here do not make much
sense. I'll introduce them in the copied version when necessary.

> > > > > @@ -378,6 +412,8 @@ void overlapped_format_tests(IDirectInputA *pDI,
> > > > > HWND hwnd)
> > > > >        /* release D */
> > > > >        keybd_event(0, DIK_D, KEYEVENTF_SCANCODE|KEYEVENTF_KEYUP, 0);
> > > > >        pump_messages();
> > > > > +    ok(wait_for_device_data_and_discard(keyboard),
> > > > > +            "Timed out while waiting for injected events to be
> > > > > picked up by DirectInput.\n");
> > > > >        hr = IDirectInputDevice_Unacquire(keyboard);
> > > > >        ok(SUCCEEDED(hr), "IDirectInputDevice_Unacquire() failed:
> > > > > %08x\n", hr);
> > > > 
> > > > NP: Yes, dinput code is heavily inconsistent, but I don't think it uses
> > > > the two level indent on line continuation style anywhere yet. Personally
> > > > I don't find it very readable and I would rather align on the paren.
> > > 
> > > The problem with aligning to the parenthesis (or other semantic unit) is
> > > that it makes things really ugly when the parenthesis falls late in the
> > > line. For whatever my opinion is worth, I'm distinctly in favor of a
> > > constant eight-space indent, including spreading that to more code.
> 
> > I share the same preference as zf. I think I've even set my editor to do
> > that after seeing this convention used a lot in msvcrt and a few other
> > places.
> > 
> As much as I don't like arguing about style variations, this one is actually
> bothering me. Alignment (on the paren, on operators, etc) is a pretty common
> style, and I think it actually makes things more readable.
> 
> I feel like the fixed two level indentation only helps when writing code, by
> making the decision simpler, or possibly diffs by saving re-alignments, but
> it doesn't help reading the code at all, especially in cases with nested
> parentheses.
> 
> On function declaration, and because we often have a lot of declspec (and
> possibly long ones like STDMETHODCALLTYPE) it may be relevant but otherwise
> in the code I don't feel like it's an improvement.
> 
> 
> Then again, although dinput style is inconsistent it's not using that style
> yet anywhere (except for function declarations and even only in a few
> locations), so unless we intend to rewrite it all I don't see reason to add
> another style variation.
> 
> In my recent contributions to dinput and as some of the existing code was
> already like this, I chose a style similar to the user32 / ntdll style, with
> space in parentheses (see ansi.c). And unless it really bothers someone I
> intended to use it for my HID joystick too.

IMO those stylistic nitpicks are just a personal preference and are not
worth mentioning in a review at all unless you break existing
conventions in the file or area being modified, or if it's just
something straight up crazy. Otherwise this is what ensues.

There's a precedent of ok() and 8 space indentation in dinput already.
Both conventions are about equally popular across the codebase. There's
no official style guide AFAIK and no one is going to run statistics on
the whole repo when deciding what variant to go with. People will just
copy something from the immediate neighbourhood.

One could even argue that the style in ansi.c now diverges from the rest
of dinput which creates even more room for stylistic confusion.

/shrug, I'm mostly indifferent, so I am going with the suggestions but
only to get this moving.

-- 
Cheers,
Arek