[PATCH xf86-input-synaptics] syndaemon: enable touchpad when pressing a modifier combo

[Peter Hutterer]

On Thu, Aug 04, 2016 at 01:06:37PM +0200, Anton Lindqvist wrote:
> Thanks for the feedback. Here's a revised patch trying to resolve the
> issues. Should the new patch be re-submitted in a separate thread?

sticking it in the same thread makes them easier to associate, but for next
time please add a v2, v3, etc. right after PATCH

> 
> ------------------------ >8 ------------------------
> 
> When ignoring modifiers, ensure the touchpad is enabled once a modifier
> key is pressed disregarding any previous key press that caused the
> touchpad to be disabled.
> 
> Signed-off-by: Anton Lindqvist <anton.lindqvist at gmail.com>
> ---

fwiw, if you write the general comments here, after the --- then git am will
automatically remove it and there's no need to get the scissors out.

>  tools/syndaemon.c | 48 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/syndaemon.c b/tools/syndaemon.c
> index 29e75f5..1d5ce1d 100644
> --- a/tools/syndaemon.c
> +++ b/tools/syndaemon.c
> @@ -47,6 +47,12 @@
>  
>  #include "synaptics-properties.h"
>  
> +enum KeyboardActivity {
> +    ActivityNew,
> +    ActivityNone,
> +    ActivityReset
> +};
> +
>  enum TouchpadState {
>      TouchpadOn = 0,
>      TouchpadOff = 1,
> @@ -181,29 +187,29 @@ install_signal_handler(void)
>      }
>  }
>  
> -/**
> - * Return non-zero if the keyboard state has changed since the last call.
> - */
> -static int
> +static enum KeyboardActivity
>  keyboard_activity(Display * display)
>  {
>      static unsigned char old_key_state[KEYMAP_SIZE];
>      unsigned char key_state[KEYMAP_SIZE];
>      int i;
> -    int ret = 0;
> +    int ret = ActivityNone;
>  
>      XQueryKeymap(display, (char *) key_state);
>  
>      for (i = 0; i < KEYMAP_SIZE; i++) {
>          if ((key_state[i] & ~old_key_state[i]) & keyboard_mask[i]) {
> -            ret = 1;
> +            ret = ActivityNew;
>              break;
>          }
>      }
>      if (ignore_modifier_combos) {
>          for (i = 0; i < KEYMAP_SIZE; i++) {
>              if (key_state[i] & ~keyboard_mask[i]) {
> -                ret = 0;
> +                if (old_key_state[i] & ~keyboard_mask[i])
> +                    ret = ActivityNone;
> +                else
> +                    ret = ActivityReset;
>                  break;
>              }
>          }
> @@ -232,8 +238,17 @@ main_loop(Display * display, double idle_time, int poll_delay)
>  
>      for (;;) {
>          current_time = get_time();
> -        if (keyboard_activity(display))
> -            last_activity = current_time;
> +        switch (keyboard_activity(display)) {
> +            case ActivityNew:
> +                last_activity = current_time;
> +                break;
> +            case ActivityNone:
> +                /* NOP */;
> +                break;
> +            case ActivityReset:
> +                last_activity = 0.0;
> +                break;
> +        }
>  
>          /* If system times goes backwards, touchpad can get locked. Make
>           * sure our last activity wasn't in the future and reset if it was. */
> @@ -423,6 +438,7 @@ record_main_loop(Display * display, double idle_time)
>          fd_set read_fds;
>          int ret;
>          int disable_event = 0;
> +	int modifier_event = 0;

all the record bits have tabs instead of spaces. I fixed that up locally,
but...

>          struct timeval timeout;
>  
>          FD_ZERO(&read_fds);
> @@ -454,9 +470,12 @@ record_main_loop(Display * display, double idle_time)
>                  disable_event = 1;
>              }
>  
> -            if (cbres.non_modifier_event &&
> -                !(ignore_modifier_combos && is_modifier_pressed(&cbres))) {
> -                disable_event = 1;
> +            if (cbres.non_modifier_event) {
> +		if (ignore_modifier_combos && is_modifier_pressed(&cbres)) {
> +		    modifier_event = 1;

this doesn't work the same way. in poll mode, any modifier will re-enable
the touchpad immediately. but this path is only hit for the non-modifier
keys while a modifer is down. 

- press a
- touchpad is disabled
- press shift
    poll mode: touchpad re-enables
    record mode: no change
- press s
    poll mode: no change
    record mode: touchpad re-enables
- release keys, touchpad remains enabled

I can't quite decide which one is the more sensible behaviour but I think
more compatible with normal -k mode would be the your poll mode one so let's
stick with that. Please make sure both behave the same.

Cheers,
   Peter




> +		} else {
> +		    disable_event = 1;
> +		}
>              }
>          }
>  
> @@ -468,10 +487,13 @@ record_main_loop(Display * display, double idle_time)
>              toggle_touchpad(False);
>          }
>  
> +	if (modifier_event && pad_disabled) {
> +	    toggle_touchpad(True);
> +	}
> +
>          if (ret == 0 && pad_disabled) { /* timeout => enable event */
>              toggle_touchpad(True);
>          }
> -
>      }                           /* end while(1) */
>  
>      XFreeModifiermap(cbres.modifiers);
> -- 
> 2.7.0