[PATCH v4 xserver] xkb: fix releasing overlay while keydown

[Peter Hutterer]

On Mon, Nov 07, 2016 at 03:42:41AM +0500, Mihail Konev wrote:
> Testcase:
> 
> In ~/.xbindkeysrc:
> 
>   "xterm &"
>        XF86LaunchA
> 
> In ~/ov.xkb:
> 
>   xkb_keymap {
>           xkb_keycodes { include "evdev" };
>           xkb_types    { include "complete" };
> 
>           xkb_compat   { include "complete"
>                interpret Overlay1_Enable+AnyOfOrNone(all) {
>                   action= SetControls(controls=Overlay1);
>                };
>           };
> 
>           xkb_symbols  { include "pc+inet(evdev)+us"
>                   key <INS> { [ Overlay1_Enable ] };
>                   key <AE01> { overlay1 = <AE02> }; // Insert+1 => 2
>                   key <TLDE> { overlay1 = <I128> }; // Insert+~ => XF86LaunchA
>           };
> 
>           xkb_geometry { include "pc(pc104)" };
>   };
> 
> Apply this layout: 'xkbcomp ~/ov.xkb $DISPLAY'.
> Run "xbindkeys -n -v"
> In the exact order:
> - press Insert
> - press Tilde
> - release Insert
> - wait
> - release Tilde
> Keyboard input in the new terminal window(s) would be locked
> until another Insert+Tilde .
> 
> Reported-by: Mariusz Mazur <mariusz.g.mazur at gmail.com>
> Signed-off-by: Mihail Konev <k.mvc at ya.ru>
> ---
> v3 was still incorrect and did not done what it was supposed to.
> This version is specifically tested to properly enable and disable
> overlay, i.e. allow "`"-s to be sent both before and after Insert being
> down.
> Debugging version attached.
> 
> Without (keywas_overlaid - 1) trickery it does not address the issue
> (i.e. input stays locked until Insert+Tilde)
> (but does not happen without open-new-window being triggered by xbindkeys,
> i.e. when the latter is not running).
> Maybe overlay_perkey_state description comment should better reflect this.
> 
> Also commit description missed Reported-by.
> 
> The "where-overlay1,2-is-in-xkb" is resolved in this patch.
> 
> As for "applicability of overlays", they are per-keycode, and layout-independent.
> This differs from RedirectKey that are per-keysym, and, therefore,
> also per-shift-level and per-layout (per-group).
> 
> There should be no need to use overlays instead of RedirectKey,
> especially given that overlay is a "behavior", which
> could be only one per keycode.
> 
>  xkb/xkbPrKeyEv.c | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/xkb/xkbPrKeyEv.c b/xkb/xkbPrKeyEv.c
> index f7a6b4b14306..35bb1e9f405a 100644
> --- a/xkb/xkbPrKeyEv.c
> +++ b/xkb/xkbPrKeyEv.c
> @@ -43,6 +43,14 @@ THE USE OR PERFORMANCE OF THIS SOFTWARE.
>  
>  /***====================================================================***/
>  
> +/* Keeps track of overlay in effect for a given key,
> + * so that if an overlay is released while key is down,
> + * the key retains overlaid until its release.
> + * Cannot be a bitmask, as needs at least three values
> + * (as overlaid keys need to generate two releases).
> + * */
> +static unsigned char overlay_perkey_state[256];

I haven't really wrapped my head around the rest of the patch, but one thing
I noticed: this can't work across multiple devices, it would have to be a
per-device property.

Cheers,
   Peter

> +
>  void
>  XkbProcessKeyboardEvent(DeviceEvent *event, DeviceIntPtr keybd)
>  {
> @@ -121,20 +129,38 @@ XkbProcessKeyboardEvent(DeviceEvent *event, DeviceIntPtr keybd)
>          case XkbKB_Overlay2:
>          {
>              unsigned which;
> +            unsigned overlay_active_now;
> +            unsigned is_keyrelease = (event->type == ET_KeyRelease) ? 1 : 0;
> +            unsigned key_was_overlaid = 0;
>  
>              if (behavior.type == XkbKB_Overlay1)
>                  which = XkbOverlay1Mask;
>              else
>                  which = XkbOverlay2Mask;
> -            if ((xkbi->desc->ctrls->enabled_ctrls & which) == 0)
> +            overlay_active_now = (xkbi->desc->ctrls->enabled_ctrls & which) ? 1 : 0;
> +
> +            if ((unsigned char)key == key) {
> +                key_was_overlaid = overlay_perkey_state[key];
> +                if (overlay_active_now && !is_keyrelease)
> +                    overlay_perkey_state[key] = 2;
> +                else if (is_keyrelease && (overlay_active_now || key_was_overlaid))
> +                    overlay_perkey_state[key] = key_was_overlaid - 1;
> +                else if (key_was_overlaid && !overlay_active_now && !is_keyrelease) {
> +                    /* ignore key presses after overlay is released,
> +                     * as their release would have been overridden in prev branch,
> +                     * and key would need another key-and-release to recover from overlay
> +                     * */
> +                    return;
> +                } else {
> +                    break;
> +                }
> +            }
> +
> +            if (!overlay_active_now && !key_was_overlaid)
>                  break;
>              if ((behavior.data >= xkbi->desc->min_key_code) &&
>                  (behavior.data <= xkbi->desc->max_key_code)) {
>                  event->detail.key = behavior.data;
> -                /* 9/11/94 (ef) -- XXX! need to match release with */
> -                /*                 press even if the state of the  */
> -                /*                 corresponding overlay control   */
> -                /*                 changes while the key is down   */
>              }
>          }
>              break;
> -- 
> 2.9.2
>