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

[Arkadiusz Hiler]

Both Acquire() and event processing with DirectInput seem to be
asynchronous. In most cases we can just keep hammering GetDeviceData()
until the event gets processed.

Things get pretty racy around Acquire() though. If we fire event right after the
call we can find ourselves in one of the three situations:

1. Event happened after acquiring has completed - the wait will suffice.

2. Event happened before acquiring did any real work - the device will
   pick up the state as if the event was processed, but there's nothing
   in GetDeviceData(). Because of that we cannot fail on wait.

3. Event happened somewhere in the middle of acquiring - we ended up
   both missing the event for GetDeviceData() and we have outdated
   state. Firing the event again will register as if the button was not
   already pressed.

This change covers all three scenarios.

Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com>
---

I've done some more testing and noticed that indeed event processing is
asynchronous - events can arrive a bit late but we never miss them.

The situation gets a bit tricky around Acquire(). After trying many things to
make it behave nicely it turned out there are three possible states we can find
ourselves in.

I've been spamming the testbot and two VMs locally with the tests and it seems
to be reliable now.

If this solution gets accepted I'll give the same treatment to dinput mouse
tests (which are currently testing nothing on Windows) and dinput8 device tests.

Thanks Rémi for the suggestions!

Cc: Rémi Bernon <rbernon(a)codeweavers.com>

 dlls/dinput/tests/device.c | 54 +++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/dlls/dinput/tests/device.c b/dlls/dinput/tests/device.c
index 6307957b6e2..d404c769953 100644
--- a/dlls/dinput/tests/device.c
+++ b/dlls/dinput/tests/device.c
@@ -343,11 +343,32 @@ static void pump_messages(void)
     }
 }
 
+BOOL wait_for_device_data_and_discard_(int line, IDirectInputDeviceA *device)
+{
+    DWORD cnt;
+    HRESULT hr;
+    DWORD start_time = GetTickCount();
+
+    do
+    {
+        cnt = 10;
+        hr = IDirectInputDevice_GetDeviceData(device, sizeof(DIDEVICEOBJECTDATA_DX3), NULL, &cnt, 0);
+        ok_(__FILE__, line)(SUCCEEDED(hr), "IDirectInputDevice_GetDeviceData() failed: %08x\n", hr);
+        ok_(__FILE__, line)(cnt == 0 || cnt == 1, "Unexpected number of events: %d\n", cnt);
+    } while (cnt != 1 && (GetTickCount() - start_time < 500));
+
+    return cnt == 1;
+}
+
+#define wait_for_device_data_and_discard(device) wait_for_device_data_and_discard_(__LINE__, device)
+
 void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
 {
     HRESULT hr;
     struct overlapped_state state;
     IDirectInputDeviceA *keyboard = NULL;
+    DIPROPDWORD dp;
+    DWORD tries;
 
     hr = IDirectInput_CreateDevice(pDI, &GUID_SysKeyboard, &keyboard, NULL);
     ok(SUCCEEDED(hr), "IDirectInput_CreateDevice() failed: %08x\n", hr);
@@ -355,15 +376,28 @@ void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
     /* test overlapped slider - default value 0 */
     hr = IDirectInputDevice_SetDataFormat(keyboard, &overlapped_slider_format);
     ok(SUCCEEDED(hr), "IDirectInputDevice_SetDataFormat() failed: %08x\n", hr);
+
+    dp.diph.dwSize = sizeof(DIPROPDWORD);
+    dp.diph.dwHeaderSize = sizeof(DIPROPHEADER);
+    dp.diph.dwHow = DIPH_DEVICE;
+    dp.diph.dwObj = 0;
+    dp.dwData = 10;
+    hr = IDirectInputDevice_SetProperty(keyboard, DIPROP_BUFFERSIZE, &dp.diph);
+    ok(SUCCEEDED(hr), "IDirectInputDevice_SetProperty() failed: %08x\n", hr);
+
     hr = IDirectInputDevice_Acquire(keyboard);
     ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed: %08x\n", hr);
 
     SetFocus(hwnd);
     pump_messages();
 
-    /* press D */
-    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
-    pump_messages();
+    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);
@@ -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);
@@ -391,9 +427,13 @@ void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
     SetFocus(hwnd);
     pump_messages();
 
-    /* press D */
-    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
-    pump_messages();
+    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);
@@ -408,6 +448,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");
 
     if (keyboard) IUnknown_Release(keyboard);
 }
-- 
2.32.0

[Rémi Bernon]

Hi Arek,

It looks almost good, but a few changes would make it even better I 
think (and taking the opportunity to nit-pick a bit).

On 8/4/21 5:26 PM, Arkadiusz Hiler wrote:
> Both Acquire() and event processing with DirectInput seem to be
> asynchronous. In most cases we can just keep hammering GetDeviceData()
> until the event gets processed.
>
> Things get pretty racy around Acquire() though. If we fire event right after the
> call we can find ourselves in one of the three situations:
>
> 1. Event happened after acquiring has completed - the wait will suffice.
>
> 2. Event happened before acquiring did any real work - the device will
>     pick up the state as if the event was processed, but there's nothing
>     in GetDeviceData(). Because of that we cannot fail on wait.
>
> 3. Event happened somewhere in the middle of acquiring - we ended up
>     both missing the event for GetDeviceData() and we have outdated
>     state. Firing the event again will register as if the button was not
>     already pressed.
>
> This change covers all three scenarios.
>
> Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com>
> ---
>
> I've done some more testing and noticed that indeed event processing is
> asynchronous - events can arrive a bit late but we never miss them.
>
> The situation gets a bit tricky around Acquire(). After trying many things to
> make it behave nicely it turned out there are three possible states we can find
> ourselves in.
>
> I've been spamming the testbot and two VMs locally with the tests and it seems
> to be reliable now.
>
> If this solution gets accepted I'll give the same treatment to dinput mouse
> tests (which are currently testing nothing on Windows) and dinput8 device tests.
>
> Thanks Rémi for the suggestions!
>
> Cc: Rémi Bernon <rbernon(a)codeweavers.com>
>
> dlls/dinput/tests/device.c | 54 +++++++++++++++++++++++++++++++++-----
>   1 file changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/dinput/tests/device.c b/dlls/dinput/tests/device.c
> index 6307957b6e2..d404c769953 100644
> --- a/dlls/dinput/tests/device.c
> +++ b/dlls/dinput/tests/device.c
> @@ -343,11 +343,32 @@ static void pump_messages(void)
>       }
>   }
>
> +BOOL wait_for_device_data_and_discard_(int line, IDirectInputDeviceA *device)
> +{
> +    DWORD cnt;
> +    HRESULT hr;
> +    DWORD start_time = GetTickCount();
> +
> +    do
> +    {
> +        cnt = 10;
> +        hr = IDirectInputDevice_GetDeviceData(device, sizeof(DIDEVICEOBJECTDATA_DX3), NULL, &cnt, 0);
> +        ok_(__FILE__, line)(SUCCEEDED(hr), "IDirectInputDevice_GetDeviceData() failed: %08x\n", hr);
> +        ok_(__FILE__, line)(cnt == 0 || cnt == 1, "Unexpected number of events: %d\n", cnt);
> +    } while (cnt != 1 && (GetTickCount() - start_time < 500));
> +
> +    return cnt == 1;
> +}
> +
> +#define wait_for_device_data_and_discard(device) wait_for_device_data_and_discard_(__LINE__, device)
> +

NP: These macros are usually declared right before the function itself.
> void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
>   {
>       HRESULT hr;
>       struct overlapped_state state;
>       IDirectInputDeviceA *keyboard = NULL;
> +    DIPROPDWORD dp;
> +    DWORD tries;
>
> hr = IDirectInput_CreateDevice(pDI, &GUID_SysKeyboard, &keyboard, NULL);
>       ok(SUCCEEDED(hr), "IDirectInput_CreateDevice() failed: %08x\n", hr);
> @@ -355,15 +376,28 @@ void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
>       /* test overlapped slider - default value 0 */
>       hr = IDirectInputDevice_SetDataFormat(keyboard, &overlapped_slider_format);
>       ok(SUCCEEDED(hr), "IDirectInputDevice_SetDataFormat() failed: %08x\n", hr);
> +
> +    dp.diph.dwSize = sizeof(DIPROPDWORD);
> +    dp.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> +    dp.diph.dwHow = DIPH_DEVICE;
> +    dp.diph.dwObj = 0;
> +    dp.dwData = 10;
> +    hr = IDirectInputDevice_SetProperty(keyboard, DIPROP_BUFFERSIZE, &dp.diph);
> +    ok(SUCCEEDED(hr), "IDirectInputDevice_SetProperty() failed: %08x\n", hr);
> +
>       hr = IDirectInputDevice_Acquire(keyboard);
>       ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed: %08x\n", hr);
>
> SetFocus(hwnd);
>       pump_messages();
>
> -    /* press D */
> -    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
> -    pump_messages();
> +    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?).
> @@ -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.

Cheers,
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[Zebediah Figura (she/her)]

On 8/4/21 2:20 PM, Rémi Bernon wrote:
> Hi Arek,
>
> It looks almost good, but a few changes would make it even better I 
> think (and taking the opportunity to nit-pick a bit).
>
> On 8/4/21 5:26 PM, Arkadiusz Hiler wrote:
> > Both Acquire() and event processing with DirectInput seem to be
> > asynchronous. In most cases we can just keep hammering GetDeviceData()
> > until the event gets processed.
> >
> > Things get pretty racy around Acquire() though. If we fire event right 
> > after the
> > call we can find ourselves in one of the three situations:
> >
> > 1. Event happened after acquiring has completed - the wait will suffice.
> >
> > 2. Event happened before acquiring did any real work - the device will
> >     pick up the state as if the event was processed, but there's nothing
> >     in GetDeviceData(). Because of that we cannot fail on wait.
> >
> > 3. Event happened somewhere in the middle of acquiring - we ended up
> >     both missing the event for GetDeviceData() and we have outdated
> >     state. Firing the event again will register as if the button was not
> >     already pressed.
> >
> > This change covers all three scenarios.
> >
> > Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com>
> > ---
> >
> > I've done some more testing and noticed that indeed event processing is
> > asynchronous - events can arrive a bit late but we never miss them.
> >
> > The situation gets a bit tricky around Acquire(). After trying many 
> > things to
> > make it behave nicely it turned out there are three possible states we 
> > can find
> > ourselves in.
> >
> > I've been spamming the testbot and two VMs locally with the tests and 
> > it seems
> > to be reliable now.
> >
> > If this solution gets accepted I'll give the same treatment to dinput 
> > mouse
> > tests (which are currently testing nothing on Windows) and dinput8 
> > device tests.
> >
> > Thanks Rémi for the suggestions!
> >
> > Cc: Rémi Bernon <rbernon(a)codeweavers.com>
> >
> >   dlls/dinput/tests/device.c | 54 +++++++++++++++++++++++++++++++++-----
> >   1 file changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/dlls/dinput/tests/device.c b/dlls/dinput/tests/device.c
> > index 6307957b6e2..d404c769953 100644
> > --- a/dlls/dinput/tests/device.c
> > +++ b/dlls/dinput/tests/device.c
> > @@ -343,11 +343,32 @@ static void pump_messages(void)
> >       }
> >   }
> > +BOOL wait_for_device_data_and_discard_(int line, IDirectInputDeviceA 
> > *device)
> > +{
> > +    DWORD cnt;
> > +    HRESULT hr;
> > +    DWORD start_time = GetTickCount();
> > +
> > +    do
> > +    {
> > +        cnt = 10;
> > +        hr = IDirectInputDevice_GetDeviceData(device, 
> > sizeof(DIDEVICEOBJECTDATA_DX3), NULL, &cnt, 0);
> > +        ok_(__FILE__, line)(SUCCEEDED(hr), 
> > "IDirectInputDevice_GetDeviceData() failed: %08x\n", hr);
> > +        ok_(__FILE__, line)(cnt == 0 || cnt == 1, "Unexpected number 
> > of events: %d\n", cnt);
> > +    } while (cnt != 1 && (GetTickCount() - start_time < 500));
> > +
> > +    return cnt == 1;
> > +}
> > +
> > +#define wait_for_device_data_and_discard(device) 
> > wait_for_device_data_and_discard_(__LINE__, device)
> > +
>
> NP: These macros are usually declared right before the function itself.

I don't think it's that consistent; I've seen both organizations.
> >   void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
> >   {
> >       HRESULT hr;
> >       struct overlapped_state state;
> >       IDirectInputDeviceA *keyboard = NULL;
> > +    DIPROPDWORD dp;
> > +    DWORD tries;
> >       hr = IDirectInput_CreateDevice(pDI, &GUID_SysKeyboard, 
> > &keyboard, NULL);
> >       ok(SUCCEEDED(hr), "IDirectInput_CreateDevice() failed: %08x\n", 
> > hr);
> > @@ -355,15 +376,28 @@ void overlapped_format_tests(IDirectInputA *pDI, 
> > HWND hwnd)
> >       /* test overlapped slider - default value 0 */
> >       hr = IDirectInputDevice_SetDataFormat(keyboard, 
> > &overlapped_slider_format);
> >       ok(SUCCEEDED(hr), "IDirectInputDevice_SetDataFormat() failed: 
> > %08x\n", hr);
> > +
> > +    dp.diph.dwSize = sizeof(DIPROPDWORD);
> > +    dp.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> > +    dp.diph.dwHow = DIPH_DEVICE;
> > +    dp.diph.dwObj = 0;
> > +    dp.dwData = 10;
> > +    hr = IDirectInputDevice_SetProperty(keyboard, DIPROP_BUFFERSIZE, 
> > &dp.diph);
> > +    ok(SUCCEEDED(hr), "IDirectInputDevice_SetProperty() failed: 
> > %08x\n", hr);
> > +
> >       hr = IDirectInputDevice_Acquire(keyboard);
> >       ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed: %08x\n", 
> > hr);
> >       SetFocus(hwnd);
> >       pump_messages();
> > -    /* press D */
> > -    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
> > -    pump_messages();
> > +    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?).
> > @@ -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.

[Arkadiusz Hiler]

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>
> > > +BOOL wait_for_device_data_and_discard_(int line,
> > > IDirectInputDeviceA *device)
> > > +{
> > > +    DWORD cnt;
> > > +    HRESULT hr;
> > > +    DWORD start_time = GetTickCount();
> > > +
> > > +    do
> > > +    {
> > > +        cnt = 10;
> > > +        hr = IDirectInputDevice_GetDeviceData(device,
> > > sizeof(DIDEVICEOBJECTDATA_DX3), NULL, &cnt, 0);
> > > +        ok_(__FILE__, line)(SUCCEEDED(hr),
> > > "IDirectInputDevice_GetDeviceData() failed: %08x\n", hr);
> > > +        ok_(__FILE__, line)(cnt == 0 || cnt == 1, "Unexpected
> > > number of events: %d\n", cnt);
> > > +    } while (cnt != 1 && (GetTickCount() - start_time < 500));
> > > +
> > > +    return cnt == 1;
> > > +}
> > > +
> > > +#define wait_for_device_data_and_discard(device)
> > > wait_for_device_data_and_discard_(__LINE__, device)
> > > +
> >
> > NP: These macros are usually declared right before the function itself.
>
> I don't think it's that consistent; I've seen both organizations.

Indeed. I've checked 3 instances that I've found by looking for `ok_`.
All three were declared this way, so I went with that.
> > >   void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
> > >   {
> > >       HRESULT hr;
> > >       struct overlapped_state state;
> > >       IDirectInputDeviceA *keyboard = NULL;
> > > +    DIPROPDWORD dp;
> > > +    DWORD tries;
> > >       hr = IDirectInput_CreateDevice(pDI, &GUID_SysKeyboard,
> > > &keyboard, NULL);
> > >       ok(SUCCEEDED(hr), "IDirectInput_CreateDevice() failed:
> > > %08x\n", hr);
> > > @@ -355,15 +376,28 @@ void overlapped_format_tests(IDirectInputA
> > > *pDI, HWND hwnd)
> > >       /* test overlapped slider - default value 0 */
> > >       hr = IDirectInputDevice_SetDataFormat(keyboard,
> > > &overlapped_slider_format);
> > >       ok(SUCCEEDED(hr), "IDirectInputDevice_SetDataFormat() failed:
> > > %08x\n", hr);
> > > +
> > > +    dp.diph.dwSize = sizeof(DIPROPDWORD);
> > > +    dp.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> > > +    dp.diph.dwHow = DIPH_DEVICE;
> > > +    dp.diph.dwObj = 0;
> > > +    dp.dwData = 10;
> > > +    hr = IDirectInputDevice_SetProperty(keyboard,
> > > DIPROP_BUFFERSIZE, &dp.diph);
> > > +    ok(SUCCEEDED(hr), "IDirectInputDevice_SetProperty() failed:
> > > %08x\n", hr);
> > > +
> > >       hr = IDirectInputDevice_Acquire(keyboard);
> > >       ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed:
> > > %08x\n", hr);
> > >       SetFocus(hwnd);
> > >       pump_messages();
> > > -    /* press D */
> > > -    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
> > > -    pump_messages();
> > > +    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);

We would also need a mouse variant.

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.
> > > @@ -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.

-- 
Cheers,
Arek

[Rémi Bernon]

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>
> > > > +BOOL wait_for_device_data_and_discard_(int line,
> > > > IDirectInputDeviceA *device)
> > > > +{
> > > > +    DWORD cnt;
> > > > +    HRESULT hr;
> > > > +    DWORD start_time = GetTickCount();
> > > > +
> > > > +    do
> > > > +    {
> > > > +        cnt = 10;
> > > > +        hr = IDirectInputDevice_GetDeviceData(device,
> > > > sizeof(DIDEVICEOBJECTDATA_DX3), NULL, &cnt, 0);
> > > > +        ok_(__FILE__, line)(SUCCEEDED(hr),
> > > > "IDirectInputDevice_GetDeviceData() failed: %08x\n", hr);
> > > > +        ok_(__FILE__, line)(cnt == 0 || cnt == 1, "Unexpected
> > > > number of events: %d\n", cnt);
> > > > +    } while (cnt != 1 && (GetTickCount() - start_time < 500));
> > > > +
> > > > +    return cnt == 1;
> > > > +}
> > > > +
> > > > +#define wait_for_device_data_and_discard(device)
> > > > wait_for_device_data_and_discard_(__LINE__, device)
> > > > +
> > >
> > > NP: These macros are usually declared right before the function itself.
> >
> > I don't think it's that consistent; I've seen both organizations.
>
> Indeed. I've checked 3 instances that I've found by looking for `ok_`.
> All three were declared this way, so I went with that.

It's not completely consistent yes, but I think it quite stands out that 
most of the time it's the case.

$ egrep 'define.*\W[a-z]\w+_\(' dlls/*/tests/* -A1
> > > >   void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
> > > >    {
> > > >        HRESULT hr;
> > > >        struct overlapped_state state;
> > > >        IDirectInputDeviceA *keyboard = NULL;
> > > > +    DIPROPDWORD dp;
> > > > +    DWORD tries;
> > > >        hr = IDirectInput_CreateDevice(pDI, &GUID_SysKeyboard,
> > > > &keyboard, NULL);
> > > >        ok(SUCCEEDED(hr), "IDirectInput_CreateDevice() failed:
> > > > %08x\n", hr);
> > > > @@ -355,15 +376,28 @@ void overlapped_format_tests(IDirectInputA
> > > > *pDI, HWND hwnd)
> > > >        /* test overlapped slider - default value 0 */
> > > >        hr = IDirectInputDevice_SetDataFormat(keyboard,
> > > > &overlapped_slider_format);
> > > >        ok(SUCCEEDED(hr), "IDirectInputDevice_SetDataFormat() failed:
> > > > %08x\n", hr);
> > > > +
> > > > +    dp.diph.dwSize = sizeof(DIPROPDWORD);
> > > > +    dp.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> > > > +    dp.diph.dwHow = DIPH_DEVICE;
> > > > +    dp.diph.dwObj = 0;
> > > > +    dp.dwData = 10;
> > > > +    hr = IDirectInputDevice_SetProperty(keyboard,
> > > > DIPROP_BUFFERSIZE, &dp.diph);
> > > > +    ok(SUCCEEDED(hr), "IDirectInputDevice_SetProperty() failed:
> > > > %08x\n", hr);
> > > > +
> > > >        hr = IDirectInputDevice_Acquire(keyboard);
> > > >        ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed:
> > > > %08x\n", hr);
> > > >        SetFocus(hwnd);
> > > >        pump_messages();
> > > > -    /* press D */
> > > > -    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
> > > > -    pump_messages();
> > > > +    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)?
> 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).
> > > > @@ -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.

Cheers,
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[Zebediah Figura (she/her)]

On 8/4/21 4:44 PM, Rémi Bernon wrote:
> > > > 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.

Well, this is already more discussion than is warranted, but anyway...

I don't disagree that aligning to parentheses (or whatever) is prettier, 
and if it were some other code base I might even prefer it. Problem is, 
the win32 API has really long names. Some parts of wine (looking at you, 
wined3d) do as well. That shows up not just in function declarations, 
but invocations as well (and not just for function names, but everything 
else as well).

Granted, I'm used to eight-space indentations, but I don't find it much 
harder to read.

But in general I don't think it's worth arguing about, not even in 
review. At most I'll fix up and resend a DirectShow patch that uses an 
alternate indentation style ;-)

Of course, if you end up having nested conditionals that wrap multiple 
lines, there's a good chance you should use a helper function anyway; 
those are hard to read regardless of code style.

[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

[Rémi Bernon]

On 8/5/21 9:57 AM, Arkadiusz Hiler wrote:
> 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.

Sure, but doing Acquire in the same helper still makes sense IMHO, as 
some kind of atomic "blocking acquire" operation that only returns when 
the device has really been acquired.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>