[PATCH] dinput/tests: Make overlapped format tests more robust.
Arkadiusz Hiler
ahiler at codeweavers.com
Wed Aug 4 16:00:52 CDT 2021
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
More information about the wine-devel
mailing list