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

[Mariusz Mazur]

A week has passed and everything's working fine (and I'm using the
overlay keycombos very very heavily). Seems the patch does not have
any unforeseen side effects.

2016-11-07 22:25 GMT+01:00 Mariusz Mazur <mariusz.g.mazur at gmail.com>:
> Applied to my work env. If something starts acting funny I'll let you know.
>
> 2016-11-06 23:42 GMT+01:00 Mihail Konev <k.mvc at ya.ru>:
>> 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];
>> +
>>  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
>>
>>
>> _______________________________________________
>> xorg-devel at lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel