[PATCH 2/3] Input: Add KeyFocusIn internal event

Peter Hutterer peter.hutterer at who-t.net
Thu Nov 27 22:02:36 PST 2014


On Mon, Nov 24, 2014 at 10:17:33PM +0000, Daniel Stone wrote:
> KeyFocusIn is used to notify the server that it has entered focus with a
> key held down, so should be used to update the internal state (including
> modifiers), but perform any actions such as sending key events to the
> client, switching VTs, etc.

typo: "not perform any actions"

> This is intended to be used for FocusIn/KeymapNotify when running nested
> servers such as Xephyr, as well as a wl_keyboard::enter event from
> XWayland.
> 
> Running the currently-pressed keys through the entire XKB event
> mechanism is quite fragile, and will produce incorrect results in some
> cases, e.g. entering with Shift+AltGr pressed when Shift→AltGr will
> produce different results to AltGr→Shift. In this case, we need to lift
> the _actual_ modifier state from the parent server, however this is a
> much larger job.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Tested-by: Giulio Camuffo <giuliocamuffo at gmail.com>
> ---
>  Xi/exevents.c      |  8 +++++++-
>  dix/events.c       |  5 +++--
>  dix/getevents.c    |  7 ++++++-
>  dix/inpututils.c   |  3 ++-
>  include/eventstr.h |  1 +
>  xkb/xkbActions.c   | 38 ++++++++++++++++++++++++++++++++++----
>  xkb/xkbPrKeyEv.c   |  8 +++++---
>  7 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/Xi/exevents.c b/Xi/exevents.c
> index b0bc47e..cd2924a 100644
> --- a/Xi/exevents.c
> +++ b/Xi/exevents.c
> @@ -810,6 +810,7 @@ UpdateDeviceState(DeviceIntPtr device, DeviceEvent *event)
>      case ET_ButtonPress:
>      case ET_ButtonRelease:
>      case ET_KeyPress:
> +    case ET_KeyFocusIn:

I would honestly prefer some different name. We do real focus in events, I'd
like to avoid potential confusion here. How about ET_ServerFocusIn?

but see my reply to 3/3, an extra field in the ET_KeyPress event should
serve us just fine here too.


>      case ET_KeyRelease:
>      case ET_ProximityIn:
>      case ET_ProximityOut:
> @@ -854,7 +855,7 @@ UpdateDeviceState(DeviceIntPtr device, DeviceEvent *event)
>              v->axisVal[i] = event->valuators.data[i];
>      }
>  
> -    if (event->type == ET_KeyPress) {
> +    if (event->type == ET_KeyPress || event->type == ET_KeyFocusIn) {
>          if (!k)
>              return DONT_PROCESS;
>  
> @@ -1715,6 +1716,7 @@ ProcessDeviceEvent(InternalEvent *ev, DeviceIntPtr device)
>      case ET_ButtonPress:
>      case ET_ButtonRelease:
>      case ET_KeyPress:
> +    case ET_KeyFocusIn:
>      case ET_KeyRelease:
>      case ET_ProximityIn:
>      case ET_ProximityOut:
> @@ -1749,6 +1751,10 @@ ProcessDeviceEvent(InternalEvent *ev, DeviceIntPtr device)
>          if (!grab && CheckDeviceGrabs(device, event, 0))
>              return;
>          break;
> +    case ET_KeyFocusIn:
> +	/* FocusIn events are just for internal state management, and do not
> +	 * get posted to clients. */
> +	return;

you need to run a tab vs space replace on this patch (and maybe the other
two patches as well).

>      case ET_KeyRelease:
>          if (grab && device->deviceGrab.fromPassiveGrab &&
>              (key == device->deviceGrab.activatingKey) &&
> diff --git a/dix/events.c b/dix/events.c
> index b8c67fd..49f21d1 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -4308,10 +4308,11 @@ FixKeyState(DeviceEvent *event, DeviceIntPtr keybd)
>  
>      if (event->type == ET_KeyPress) {
>          DebugF("FixKeyState: Key %d %s\n", key,
> -               ((event->type == ET_KeyPress) ? "down" : "up"));
> +               ((event->type == ET_KeyPress) ? "down" :
> +	        ((event->type == ET_KeyFocusIn) ? "down (focus)" : "up")));
>      }
>  
> -    if (event->type == ET_KeyPress)
> +    if (event->type == ET_KeyPress || event->type == ET_KeyFocusIn)
>          set_key_down(keybd, key, KEY_PROCESSED);
>      else if (event->type == ET_KeyRelease)
>          set_key_up(keybd, key, KEY_PROCESSED);
> diff --git a/dix/getevents.c b/dix/getevents.c
> index dd96265..c2248fb 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -1120,7 +1120,8 @@ GetKeyboardEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
>          UpdateFromMaster(events, pDev, DEVCHANGE_KEYBOARD_EVENT, &num_events);
>  
>      /* Handle core repeating, via press/release/press/release. */
> -    if (type == KeyPress && key_is_down(pDev, key_code, KEY_POSTED)) {
> +    if ((type == KeyPress || type == KeymapNotify) &&
> +        key_is_down(pDev, key_code, KEY_POSTED)) {
>          /* If autorepeating is disabled either globally or just for that key,
>           * or we have a modifier, don't generate a repeat event. */
>          if (!pDev->kbdfeed->ctrl.autoRepeat ||
> @@ -1152,6 +1153,10 @@ GetKeyboardEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
>          event->type = ET_KeyPress;
>          set_key_down(pDev, key_code, KEY_POSTED);
>      }
> +    else if (type == KeymapNotify) {
> +	event->type = ET_KeyFocusIn;
> +	set_key_down(pDev, key_code, KEY_POSTED);
> +    }
>      else if (type == KeyRelease) {
>          event->type = ET_KeyRelease;
>          set_key_up(pDev, key_code, KEY_POSTED);
> diff --git a/dix/inpututils.c b/dix/inpututils.c
> index e5bcc31..f6d781a 100644
> --- a/dix/inpututils.c
> +++ b/dix/inpututils.c
> @@ -694,7 +694,8 @@ event_set_state(DeviceIntPtr mouse, DeviceIntPtr kbd, DeviceEvent *event)
>          XkbStatePtr state;
>  
>          /* we need the state before the event happens */
> -        if (event->type == ET_KeyPress || event->type == ET_KeyRelease)
> +        if (event->type == ET_KeyPress || event->type == ET_KeyRelease ||
> +	    event->type == ET_KeyFocusIn)
>              state = &kbd->key->xkbInfo->prev_state;
>          else
>              state = &kbd->key->xkbInfo->state;
> diff --git a/include/eventstr.h b/include/eventstr.h
> index cce903d..2f85d60 100644
> --- a/include/eventstr.h
> +++ b/include/eventstr.h
> @@ -74,6 +74,7 @@ enum EventType {
>      ET_XQuartz,
>      ET_BarrierHit,
>      ET_BarrierLeave,
> +    ET_KeyFocusIn,
>      ET_Internal = 0xFF          /* First byte */
>  };
>  
> diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
> index c6cbf56..c075115 100644
> --- a/xkb/xkbActions.c
> +++ b/xkb/xkbActions.c
> @@ -1144,9 +1144,10 @@ _XkbEnsureStateChange(XkbSrvInfoPtr xkbi)
>  }
>  
>  static void
> -_XkbApplyState(DeviceIntPtr dev, Bool genStateNotify, int evtype, int key)
> +_XkbApplyState(DeviceIntPtr dev, Bool genStateNotify, int evtype_int, int key)

.. Bool genStateNotify, enum EventType evtype, int key)

we have an enum, use it, it's much more expressive than "int evtype_int".
coincidentally, storage is cheap enough these days that we could, on
average, afford to add "internal" instead of just "int". 

>  {
>      XkbSrvInfoPtr xkbi = dev->key->xkbInfo;
> +    int evtype = (evtype_int == ET_KeyFocusIn) ? KeyPress : evtype_int;
>      int changed;
>  
>      XkbComputeDerivedState(xkbi);
> @@ -1174,10 +1175,11 @@ _XkbApplyState(DeviceIntPtr dev, Bool genStateNotify, int evtype, int key)
>  }
>  
>  void
> -XkbPushLockedStateToSlaves(DeviceIntPtr master, int evtype, int key)
> +XkbPushLockedStateToSlaves(DeviceIntPtr master, int evtype_int, int key)

same here

Cheers,
   Peter

>  {
>      DeviceIntPtr dev;
>      Bool genStateNotify;
> +    int evtype = (evtype_int == ET_KeyFocusIn) ? KeyPress : evtype_int;
>  
>      nt_list_for_each_entry(dev, inputInfo.devices, next) {
>          if (!dev->key || GetMaster(dev, MASTER_KEYBOARD) != master)
> @@ -1199,6 +1201,32 @@ XkbActionGetFilter(DeviceIntPtr dev, DeviceEvent *event, KeyCode key,
>      XkbSrvInfoPtr xkbi = dev->key->xkbInfo;
>      XkbFilterPtr filter;
>  
> +    /* For KeyFocusIn, we only want to run actions which update our state to
> +     * (hopefully vaguely kinda) match that of the host server, rather than
> +     * actually execute anything. For example, if we enter our VT with
> +     * Ctrl+Alt+Backspace held down, we don't want to terminate our server
> +     * immediately, but we _do_ want Ctrl+Alt to be latched down, so if
> +     * Backspace is released and then pressed again, the server will terminate.
> +     *
> +     * This is pretty flaky, and we should in fact inherit the complete state
> +     * from the host server. There are some state combinations that we cannot
> +     * express by running the state machine over every key, e.g. if AltGr+Shift
> +     * generates a different state to Shift+AltGr. */
> +    if (event->type == ET_KeyFocusIn) {
> +        switch (act->type) {
> +        case XkbSA_SetMods:
> +        case XkbSA_SetGroup:
> +        case XkbSA_LatchMods:
> +        case XkbSA_LatchGroup:
> +        case XkbSA_LockMods:
> +        case XkbSA_LockGroup:
> +            break;
> +        default:
> +            *sendEvent = 1;
> +            return;
> +        }
> +    }
> +
>      switch (act->type) {
>      case XkbSA_SetMods:
>      case XkbSA_SetGroup:
> @@ -1291,9 +1319,11 @@ XkbHandleActions(DeviceIntPtr dev, DeviceIntPtr kbd, DeviceEvent *event)
>      xkbi->groupChange = 0;
>  
>      sendEvent = 1;
> -    keyEvent = ((event->type == ET_KeyPress) || (event->type == ET_KeyRelease));
> +    keyEvent = ((event->type == ET_KeyPress) || (event->type == ET_KeyRelease) ||
> +                (event->type == ET_KeyFocusIn));
>      pressEvent = ((event->type == ET_KeyPress) ||
> -                  (event->type == ET_ButtonPress));
> +                  (event->type == ET_ButtonPress) ||
> +                  (event->type == ET_KeyFocusIn));
>  
>      if (pressEvent) {
>          if (keyEvent)
> diff --git a/xkb/xkbPrKeyEv.c b/xkb/xkbPrKeyEv.c
> index b24fd6c..0b15f84 100644
> --- a/xkb/xkbPrKeyEv.c
> +++ b/xkb/xkbPrKeyEv.c
> @@ -56,7 +56,8 @@ XkbProcessKeyboardEvent(DeviceEvent *event, DeviceIntPtr keybd)
>      key = event->detail.key;
>      if (xkbDebugFlags & 0x8)
>          DebugF("[xkb] XkbPKE: Key %d %s\n", key,
> -               (event->type == ET_KeyPress ? "down" : "up"));
> +               (event->type == ET_KeyPress ? "down" :
> +	        (event->type == ET_KeyFocusIn ? "down (focus)" : "up")));
>  
>      if (xkbi->repeatKey == key && event->type == ET_KeyRelease &&
>          !(xkbi->desc->ctrls->enabled_ctrls & XkbRepeatKeysMask))
> @@ -73,14 +74,15 @@ XkbProcessKeyboardEvent(DeviceEvent *event, DeviceIntPtr keybd)
>          case XkbKB_Default:
>              /* Neither of these should happen in practice, but ignore them
>                 anyway. */
> -            if (event->type == ET_KeyPress && !event->key_repeat &&
> -                key_is_down(keybd, key, KEY_PROCESSED))
> +            if ((event->type == ET_KeyPress || event->type == ET_KeyFocusIn) &&
> +                !event->key_repeat && key_is_down(keybd, key, KEY_PROCESSED))
>                  return;
>              else if (event->type == ET_KeyRelease &&
>                       !key_is_down(keybd, key, KEY_PROCESSED))
>                  return;
>              break;
>          case XkbKB_Lock:
> +            /* XXX: Semantics here might be incorrect for KeyFocusIn. */
>              if (event->type == ET_KeyRelease)
>                  return;
>              else if (key_is_down(keybd, key, KEY_PROCESSED))
> -- 
> 2.1.0
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 


More information about the xorg-devel mailing list