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,