Re: AltGr key mostly fires an additional CONTROL key

[Jon TURNEY <jon.turney@dronecode.org.uk>]

On 15/08/2011 13:53, Oliver Schmidt wrote:
> I also had problems with the AltGr key. These could reliably
> be reproduced by holding the AltGr for some seconds (causing
> Windows generating auto repeat events).
>
> Unfortunately the test version at
>
>    ftp://cygwin.com/pub/cygwinx/XWin.20110801-git-2d9f9305cb559907.exe.bz2
>
> doesn't fix this problem for me.
>
> I discovered that the mechanism in winkeybd.c function
> winIsFakeCtrl_L had a problem if PeekMessage cannot obtain
> the next Alt_R message because it is not there.
>
> I prepared a patch that remembers the last Ctrl_L event and
> reacts on a later following Alt_R. It was also necessary to
> alter the order in winWindowProc in winwndproc.c: the invocation
> of winIsFakeCtrl_L had to be done before discarding auto-repeated
> key presses.
>
> The attached patch is against the sources of xserver-cygwin-1.10.3-1.

Thanks for the patch.  The approach of remembering if the last keyevent was a 
Ctrl-L and reversing it if it was fake because it's followed by a AltGr is a 
good one, which hadn't occurred to me, and it clearly better than assuming the 
AltGr keyevent message will be available some fixed time after the Ctrl-L one.

(I had considered buffering the Ctrl-L keyevent until we could determine a 
AltGr wasn't going to be coming after it, but that would be more complex to 
implement)

A few comments on the patch follow.  It could also do with some better 
comments, as what this code is trying to do is slightly obscure, and I assume 
that the old comments about TweakUI being the cause of this are just wrong (as 
you don't mention that you have it installed)

> diff --git a/hw/xwin/winkeybd.c b/hw/xwin/winkeybd.c
> index e807fc5..460c9d6 100644
> --- a/hw/xwin/winkeybd.c
> +++ b/hw/xwin/winkeybd.c
> @@ -356,6 +356,12 @@ winIsFakeCtrl_L (UINT message, WPARAM wParam, LPARAM lParam)
>    MSG		msgNext;
>    LONG		lTime;
>    Bool		fReturn;
> +
> +  static Bool   hasLastControlL = FALSE;
> +  static UINT   lastMessage;
> +  static WPARAM lastWparam;
> +  static LPARAM lastLparam;
> +  static LONG   lastTime;

I was going to suggest using static  MSG lastMsg, but then I noticed that 
lastWparam, lastLparam are completely unused...

>    /*
>     * Fake Ctrl_L presses will be followed by an Alt_R keypress
> @@ -389,9 +395,22 @@ winIsFakeCtrl_L (UINT message, WPARAM wParam, LPARAM lParam)
>  				 WM_KEYDOWN, WM_SYSKEYDOWN,
>  				 PM_NOREMOVE);
>  	}
> -      if (msgNext.message != WM_KEYDOWN && msgNext.message != WM_SYSKEYDOWN)
> +      if (fReturn && msgNext.message != WM_KEYDOWN && msgNext.message != WM_SYSKEYDOWN)
>            fReturn = 0;
> +      if (!fReturn)
> +        {
> +          hasLastControlL = TRUE;
> +          lastMessage = message;
> +          lastWparam  = wParam;
> +          lastLparam  = lParam;
> +          lastTime    = lTime;
> +        }
> +      else
> +        {
> +          hasLastControlL = FALSE;
> +        }
> +
>        /* Is next press an Alt_R with the same timestamp? */
>        if (fReturn && msgNext.wParam == VK_MENU
>  	  && msgNext.time == lTime
> @@ -406,11 +425,33 @@ winIsFakeCtrl_L (UINT message, WPARAM wParam, LPARAM lParam)
>  	}
>      }
>
> +  /*
> +   * Check for Alt_R keypress, that was not ready when the
> +   * last Ctrl_L appeared.
> +   */
> +  else if ((message == WM_KEYDOWN || message == WM_SYSKEYDOWN)
> +      && wParam == VK_MENU
> +      && (HIWORD (lParam) & KF_EXTENDED))
> +    {
> +      if (hasLastControlL)
> +        {
> +          lTime = GetMessageTime ();
> +
> +          if ((lastMessage == WM_KEYDOWN || lastMessage == WM_SYSKEYDOWN)
> +              && lastTime == lTime)

Why is it necessary to check that the last message was WM_(SYS)KEYDOWN here? 
hasLastControlL can't get set unless it was?

> +            {
> +                /* take back the fake ctrl_L key */
> +                winSendKeyEvent (KEY_LCtrl, FALSE);
> +            }
> +          hasLastControlL = FALSE;
> +        }
> +    }
> +
>    /*
>     * Fake Ctrl_L releases will be followed by an Alt_R release
>     * with the same timestamp as the Ctrl_L release.
>     */
> -  if ((message == WM_KEYUP || message == WM_SYSKEYUP)
> +  else if ((message == WM_KEYUP || message == WM_SYSKEYUP)
>        && wParam == VK_CONTROL
>        && (HIWORD (lParam) & KF_EXTENDED) == 0)
>      {
> @@ -439,9 +480,11 @@ winIsFakeCtrl_L (UINT message, WPARAM wParam, LPARAM lParam)
>  				 PM_NOREMOVE);
>  	}
>
> -      if (msgNext.message != WM_KEYUP && msgNext.message != WM_SYSKEYUP)
> +      if (fReturn && msgNext.message != WM_KEYUP && msgNext.message != WM_SYSKEYUP)
>            fReturn = 0;
>
> +    hasLastControlL = FALSE;
> +
>        /* Is next press an Alt_R with the same timestamp? */
>        if (fReturn
>  	  && (msgNext.message == WM_KEYUP
> @@ -458,6 +501,10 @@ winIsFakeCtrl_L (UINT message, WPARAM wParam, LPARAM lParam)
>  	  return TRUE;
>  	}
>      }
> +  else
> +    {
> +      hasLastControlL = FALSE;
> +    }
>
>    /* Not a fake control left press/release */
>    return FALSE;
> diff --git a/hw/xwin/winwndproc.c b/hw/xwin/winwndproc.c
> index 316cf08..7de5a5d 100644
> --- a/hw/xwin/winwndproc.c
> +++ b/hw/xwin/winwndproc.c
> @@ -1060,6 +1060,10 @@ winWindowProc (HWND hwnd, UINT message,
>        if ((wParam == VK_LWIN || wParam == VK_RWIN) && !g_fKeyboardHookLL)
>  	break;
>
> +      /* Discard fake Ctrl_L presses that precede AltGR on non-US keyboards */
> +      if (winIsFakeCtrl_L (message, wParam, lParam))
> +	return 0;
> +
>        /*
>         * Discard presses generated from Windows auto-repeat
>         */
> @@ -1080,10 +1084,6 @@ winWindowProc (HWND hwnd, UINT message,
>          }
>        }
>
> -      /* Discard fake Ctrl_L presses that precede AltGR on non-US keyboards */
> -      if (winIsFakeCtrl_L (message, wParam, lParam))
> -	return 0;
> -

Can you say why it's necessary to change the order here and why this is the 
correct ordering?  A comment here might be a good idea :-)

>        /* Translate Windows key code to X scan code */
>        winTranslateKey (wParam, lParam, &iScanCode);
>
>

-- 
Jon TURNEY
Volunteer Cygwin/X X Server maintainer

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/