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

[Mariusz Mazur]

I've been using this heavily (at least a few hundred overlay1
keystrokes per day) for the past almost two months and had zero
issues. Any chance this could get merged?

2016-11-15 10:31 GMT+01:00 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 :
>> 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 :
>>> 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  { [ Overlay1_Enable ] };
>>>   key  { overlay1 =  }; // Insert+1 => 2
>>>   key  { overlay1 =  }; // 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 
>>> Signed-off-by: Mihail Konev 
>>> ---
>>> 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) {
>>> +   

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

[Peter Hutterer]

On Sun, Nov 27, 2016 at 01:14:35AM +0500, Mihail Konev wrote:
> On Sun, Nov 27, 2016 at 12:55:40AM +0500, Mihail Konev wrote:
> > Is adding an Xinput property the only way?
> 
> Rather,  should  it be an Xi property, or static arrays in xkb are enough?

sorry, I meant a per-device field, not a full XI property.

side-note: if you reply to list only, I may not see your email for days :)

Cheers,
   Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Mihail Konev]

On Sun, Nov 27, 2016 at 12:55:40AM +0500, Mihail Konev wrote:
> Is adding an Xinput property the only way?

Rather,  should  it be an Xi property, or static arrays in xkb are enough?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [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  { [ Overlay1_Enable ] };
>   key  { overlay1 =  }; // Insert+1 => 2
>   key  { overlay1 =  }; // 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 
> Signed-off-by: Mihail Konev 
> ---
> 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 <= 

Re: [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 :
> 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 :
>> 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  { [ Overlay1_Enable ] };
>>   key  { overlay1 =  }; // Insert+1 => 2
>>   key  { overlay1 =  }; // 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 
>> Signed-off-by: Mihail Konev 
>> ---
>> 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;
>> + 

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

[Mariusz Mazur]

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 :
> 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  { [ Overlay1_Enable ] };
>   key  { overlay1 =  }; // Insert+1 => 2
>   key  { overlay1 =  }; // 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 
> Signed-off-by: Mihail Konev 
> ---
> 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 

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

[Mihail Konev]

There is the log produced with debugging patch to illustrate
the logic in v4.
I'm not sure if doubled key events are guaranteed behaviour,
and not machine-specific one.


Tilde down
There the "`" is sent

[ 12894.309] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12894.309] (II) XkbProcessKeyboardEvent: brk
[ 12894.309] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12894.309] (II) XkbProcessKeyboardEvent: brk
...
[ 12896.268] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12896.268] (II) XkbProcessKeyboardEvent: brk

Insert down
Overlay becomes active

[ 12896.303] (II) XkbProcessKeyboardEvent: rel:0 ov_a:1 k_ov:0
[ 12896.303] (II) XkbProcessKeyboardEvent:165
[ 12896.303] (II) XkbProcessKeyboardEvent: rel:0 ov_a:1 k_ov:2
[ 12896.303] (II) XkbProcessKeyboardEvent:165
[ 12896.338] (II) XkbProcessKeyboardEvent: rel:0 ov_a:1 k_ov:2
[ 12896.338] (II) XkbProcessKeyboardEvent:165
...
[ 12900.265] (II) XkbProcessKeyboardEvent: rel:0 ov_a:1 k_ov:2
[ 12900.265] (II) XkbProcessKeyboardEvent:165

Insert up
Overlay becomes inactive
Now ignoring Tilde keypresses

[ 12900.300] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:2
[ 12900.300] (II) XkbProcessKeyboardEvent: ign
[ 12900.300] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:2
[ 12900.300] (II) XkbProcessKeyboardEvent: ign
[ 12900.334] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:2
...
[ 12902.455] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:2
[ 12902.455] (II) XkbProcessKeyboardEvent: ign

Tilde up
Treat it as overlaid-tilde-keyrelease

[ 12902.486] (II) XkbProcessKeyboardEvent: rel:1 ov_a:0 k_ov:2
[ 12902.486] (II) XkbProcessKeyboardEvent:165
[ 12902.486] (II) XkbProcessKeyboardEvent: rel:1 ov_a:0 k_ov:1
[ 12902.486] (II) XkbProcessKeyboardEvent:165

Tilde down
Then "ign"-s before were (and "brk"-s here are)
for "`" to be sent now.

[ 12907.136] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12907.136] (II) XkbProcessKeyboardEvent: brk
[ 12907.136] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12907.136] (II) XkbProcessKeyboardEvent: brk

Tilde up
Without "ign"-s before, only this would recover Tilde from overlay.

[ 12907.298] (II) XkbProcessKeyboardEvent: rel:1 ov_a:0 k_ov:0
[ 12907.298] (II) XkbProcessKeyboardEvent: brk
[ 12907.298] (II) XkbProcessKeyboardEvent: rel:1 ov_a:0 k_ov:0
[ 12907.298] (II) XkbProcessKeyboardEvent: brk

Tilde down
There and afterwards the "`"-s are sent regardless of (key_was_overlaid - 1) or 
just 0

[ 12914.156] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12914.156] (II) XkbProcessKeyboardEvent: brk
[ 12914.156] (II) XkbProcessKeyboardEvent: rel:0 ov_a:0 k_ov:0
[ 12914.156] (II) XkbProcessKeyboardEvent: brk

Tilde up

[ 12914.278] (II) XkbProcessKeyboardEvent: rel:1 ov_a:0 k_ov:0
[ 12914.278] (II) XkbProcessKeyboardEvent: brk
[ 12914.278] (II) XkbProcessKeyboardEvent: rel:1 ov_a:0 k_ov:0
[ 12914.278] (II) XkbProcessKeyboardEvent: brk
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Mihail Konev]

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  { [ Overlay1_Enable ] };
  key  { overlay1 =  }; // Insert+1 => 2
  key  { overlay1 =  }; // 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 
Signed-off-by: Mihail Konev 
---
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

>From 2c51bf28ef13dbb6229c60e662f6fedc553d3978 Mon Sep 17 00:00:00 2001
From: Mihail Konev