Re: Patch for dinput keyboard

On Sun, 19 May 2002, Ove Kaaven wrote:
> On Sat, 18 May 2002 [e-mail] wrote:
> > This 'beautiful' patch I wrote for fixing dinput was silently ignored.
> > Can someone comment about it?
>
> The x11drv parts are unnecessary. It's done differently (more
> Alexandre-style) in WineX. Since there's no particular need to hold this
> piece back whether or not the code trade Gav wants to do is successful
> (especially since you're working on it in any case), I may as well use it
> to help you get this functionality into Wine. Here's the code merged
> against ReWind, licensed under the X11 license. You may have to merge it
> again against the current Wine code yourself, since some other DirectInput
> patches may not have been applied to ReWind. (Of course, change this in
> other ways as well if you feel like it.)

Thank you, I guess. I don't want to get involved in any politics. I don't
really know anything about other trees.

I think the hook is agood idea.

I think the best thing to do now is to get best out of the two different
implementations. I have some comments about your code, and I can improve
mine with yours. I'll make a new patch from these two, and send it later.
If anybody has any other comments, I would like to hear them.
> - /* SysKeyboardAImpl */
> - BYTE                            keystate[256];

There is not a real reason to get a keystate per keyboard, as there is
only one keyboard. I made this global. Is that OK?
> static HRESULT WINAPI SysKeyboardAImpl_SetProperty(
>    LPDIRECTINPUTDEVICE2A iface,REFGUID rguid,LPCDIPROPHEADER ph
>  )
> @@ -115,13 +169,19 @@
>
> TRACE("(this=%p,%s,%p)\n",This,debugstr_guid(rguid),ph);
>    TRACE("(size=%ld,headersize=%ld,obj=%ld,how=%ld\n",
> -
>        ph->dwSize,ph->dwHeaderSize,ph->dwObj,ph->dwHow);
> -
>    ph->dwSize,ph->dwHeaderSize,ph->dwObj,ph->dwHow);
> if (!HIWORD(rguid)) {
>   switch ((DWORD)rguid) {
>   case (DWORD) DIPROP_BUFFERSIZE: {
> -
> 	LPCDIPROPDWORD	pd = (LPCDIPROPDWORD)ph;
> -
> -
> 	TRACE("(buffersize=%ld)\n",pd->dwData);
> -
> 	LPCDIPROPDWORD    pd = (LPCDIPROPDWORD)ph;
> -
> -
> 	TRACE("buffersize = %ld\n",pd->dwData);
> -
> -
> 	This->data_queue = (LPDIDEVICEOBJECTDATA)HeapAlloc(GetProcessHeap(),0,
> -
> 							   pd->dwData * sizeof(DIDEVICEOBJECTDATA));

How do you know the data_queue is free (NULL)? My patch made sure that:
!Acquired => (data_queue == NULL)
And setproperty can only be invoked when !Acquired.
> static HRESULT WINAPI SysKeyboardAImpl_GetDeviceState(
>    LPDIRECTINPUTDEVICE2A iface,DWORD len,LPVOID ptr
>  )
>  {
> - DWORD i;
> - ICOM_THIS(SysKeyboardAImpl,iface);
> -
> - if (!This->acquired)
> -
>    return DIERR_NOTACQUIRED;

I have this game that does not acquire the device. Guess what? It works
with windows.
> - memset( ptr, 0, len );
> - if (len != 256)
> - {
> -
>    WARN("whoops, got len %ld?\n", len);
> -
>    return DI_OK;
> - }
> - for (i = 0; i < 0x80; i++)
> - {
> -
>    WORD vkey = MapVirtualKeyA( i, 1 );
> -
>    if (vkey && (GetAsyncKeyState( vkey ) & 0x8000))
> -
>    {
> -
>        ((LPBYTE)ptr)[i] = 0x80;
> -
>        ((LPBYTE)ptr)[i | 0x80] = 0x80;
> -
>    }
> - }
> - return DI_OK;
> - memset( ptr, 0, len );
> - if (len != 256)
> - {
> -
> WARN("whoops, got len %ld?\n", len);
> -
> return DI_OK;

DI_OK?
> - }
> - memcpy(ptr, This->keystate, len);
> - return DI_OK;
>
> }
>
> static HRESULT WINAPI SysKeyboardAImpl_GetDeviceData(
> @@ -162,34 +258,53 @@
>  )
>  {
>    ICOM_THIS(SysKeyboardAImpl,iface);
> - int i, n;
> - DWORD len, nqtail;
> -
> - EnterCriticalSection(&(This->crit));
> - TRACE("(%p)->(dods=%ld,entries=%ld,fl=0x%08lx)\n",This,dodsize,*entries,flags);
> - TRACE("(this=%p,%ld,%p,%p(%ld)),0x%08lx)\n",
> -
>      This,dodsize,dod,entries,entries?*entries:0,flags);
> - len = ((This->queue_head < This->queue_tail) ? This->queue_len : 0)
> -
>    + (This->queue_head - This->queue_tail);
> - if (len > *entries) len = *entries;
> -
> - if (dod == NULL) {
> -
>    if (len)
> -
> TRACE("Application discarding %ld event(s).\n", len);
> -
> -
>    *entries = len;
> -
>    nqtail = This->queue_tail + len;
> -
>    while (nqtail >= This->queue_len) nqtail -= This->queue_len;
> - } else {
> -
>    if (dodsize < sizeof(DIDEVICEOBJECTDATA)) {
> -
> ERR("Wrong structure size !\n");
> -
> LeaveCriticalSection(&(This->crit));
> -
> return DIERR_INVALIDPARAM;
> -
>    }
> -
> -
>    if (len)
> -
> TRACE("Application retrieving %ld event(s).\n", len);
> -
> -
>    *entries = 0;
> -
>    nqtail = This->queue_tail;
> -
>    while (len) {
> -
> DWORD span = ((This->queue_head < nqtail) ? This->queue_len : This->queue_head)

If dodsize > 16 then span = 1, but I don't really get the algorithm. Do
you know, that most of the time there is not more than one entry in the
buffer. I once read a peace about UNIX and algorithms and N being small...
Well, never mind.
> -
>     - nqtail;
> -
> if (span > len) span = len;
> -
> /* Copy the buffered data into the application queue */
> -
> memcpy(dod + *entries, This->data_queue + nqtail, span * dodsize);
> -
> /* Advance position */
> -
> nqtail += span;
> -
> if (nqtail >= This->queue_len) nqtail -= This->queue_len;
> -
> *entries += span;
> -
> len -= span;
> -
>    }
> -
>
> }
> -
>
> if (!(flags & DIGDD_PEEK))
> -
>    This->queue_tail = nqtail;
>
> -
>
> LeaveCriticalSection(&(This->crit));
> -
>
> return DI_OK;
>
> }

I think my implementation is cleaner.
> static HRESULT WINAPI SysKeyboardAImpl_Acquire(LPDIRECTINPUTDEVICE2A iface)
> @@ -199,10 +314,27 @@
>    TRACE("(this=%p)\n",This);
>
> if (This->acquired == 0) {
> -
>  This->acquired = 1;
> -
>    DWORD i;
> -
> -
>    /* Store (in a global variable) the current lock */
> -
>    current_lock = (IDirectInputDevice2A*)This;
> -
> -
>    /* Install our keyboard hook */
> -
>    This->hook = SetWindowsHookExW( WH_KEYBOARD_LL, dinput_keyboard_hook, 0, 0 );
> -
> -
>    /* Read current keyboard state */
> -
>    memset(&This->keystate, 0, 256);
> -
>    for (i = 0; i < 0x100; i++)
> -
>    {
> -
> WORD vkey = MapVirtualKeyA( i, 1 ); /* FIXME: use map mode 3 when implemented */
> -
> if (vkey && (GetAsyncKeyState( vkey ) & 0x8000))

This does not work (see earlier mails). but it isn't needed when keystate
is global.
> -
>     This->keystate[i] = 0x80;
> -
>    }
> -
> -
>    This->acquired = 1;
> -
>    return DI_OK;
> }
> -
> - return DI_OK;
> - return S_FALSE;
>
> }

Could you send me the file instead of the patch, as I don't really know
what to do with this patch.

To get involved in politics anyway: you can have the changes I make to
this patch.