[PATCH 1/2] Add Clickpad support

[Peter Hutterer]

On Wed, Apr 21, 2010 at 08:16:40AM +0200, Takashi Iwai wrote:
> At Wed, 21 Apr 2010 14:16:41 +1000,
> Peter Hutterer wrote:
> > 
> > Thank you for the patch. Just as a heads-up, I'd like to wait with this
> > until there's at least one kernel released with this support.
> > My comments to the patch are inline.
> 
> Thanks for review!
> 
> > On Thu, Apr 15, 2010 at 06:12:10PM +0200, Takashi Iwai wrote:
> > > This patch adds the support for Synaptics Clickpad devices.
> > > It requires the change in Linux kernel synaptics input driver, found in
> > > 	https://patchwork.kernel.org/patch/92435/
> > > 
> > > When the kernel driver sets only the left-button bit evbit, Clickpad
> > > mode is activated.  In this mode, the bottom touch area is used as
> > > button emulations.  Clicking at the bottom-left, bottom-center and
> > > bottom-right zone corresponds to a left, center and right click.
> > > 
> > > There are two new parameters added.  One is a parameter to specify
> > > the size of the bottom button-area, and another is to toggle the touch-
> > > sensing in the button area.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > ---
> > >  include/synaptics-properties.h |    6 ++++
> > >  man/synaptics.man              |   21 +++++++++++++++
> > >  src/eventcomm.c                |   15 ++++++++++
> > >  src/properties.c               |   19 +++++++++++++
> > >  src/synaptics.c                |   56 ++++++++++++++++++++++++++++++++++++++++
> > >  src/synapticsstr.h             |    4 +++
> > >  tools/synclient.c              |    2 +
> > >  7 files changed, 123 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> > > index cf330d8..c77afd3 100644
> > > --- a/include/synaptics-properties.h
> > > +++ b/include/synaptics-properties.h
> > > @@ -155,4 +155,10 @@
> > >  /* 32 bit, 4 values, left, right, top, bottom */
> > >  #define SYNAPTICS_PROP_AREA "Synaptics Area"
> > >  
> > > +/* 8 bit (0-100) */
> > > +#define SYNAPTICS_PROP_TOUCH_BUTTON_AREA "Synaptics Touch Button Area"
> > 
> > > +
> > > +/* 8 bit (BOOL) */
> > > +#define SYNAPTICS_PROP_TOUCH_BUTTON_SENSE "Synaptics Touch Button Sense"
> > > +
> > >  #endif /* _SYNAPTICS_PROPERTIES_H_ */
> > > diff --git a/man/synaptics.man b/man/synaptics.man
> > > index 59fbaac..f92ea0c 100644
> > > --- a/man/synaptics.man
> > > +++ b/man/synaptics.man
> > > @@ -509,6 +509,27 @@ Ignore movements, scrolling and tapping which take place below this edge.
> > >  The option is disabled by default and can be enabled by setting the
> > >  AreaBottomEdge option to any integer value other than zero. Property: "Synaptics Area"
> > >  .
> > > +.TP
> > > +.BI "Option \*qTouchButtonArea\*q \*q" integer \*q
> > > +The percentage of touch-button area of Clickpad device.
> > > +.
> > > +The synaptics driver supports "ClickZone" mode, which emulates the buttons
> > > +to click at the bottom of touchpad area.  This option specifies how large
> > > +is the area to be used as the button area.  Passing zero to this disables
> > > +the Clickpad behavior.  The default value is 20.
> > 
> > where does the 0-100 come from? what unit is this? it's answered in the
> > header file but this should be in the man page as well.
> 
> Right.
> 
> > also, I'd rather have this stored in device units and leave things like
> > "this is 20% of the area" to higher-level tools.
> 
> I don't mind in any way.  Maybe this parameter is rarely changed.
> 
> > > +Property: "Synaptics Touch Button Area"
> > 
> > please make this so that the Synaptics Area is re-used instead of
> > introducing another area property. This should remove the need for this
> > property altogether and just need the enable/disable value in the other one.
> 
> Do you mean to extend "Synaptics Area" property?
> 
> I find it might be better to split from Synaptics Area, because this
> is pretty specific to Clickpad mode while Synaptics Area is a more
> generic one.  But, mixing them up is technically no problem, of
> course.

right now, the area detection code won't move the cursor if it's outside of
the applicable area. this was essentially added for the touchpads the dell
minis had. so as long as you're inside that area the pointer moves and
outside nothing happens.

with this feature, you could modulate this option with a clickpad feature,
i.e. if ClickPad is on, then the area outside is used as the button
calculation area (or the bottom area anyway).
I think this makes sense enough, given that the hardware is so different I
don't think there's the need for having an inactive area and a separate
clickpad area.

or is this too confusing for the user - I'm not sure myself here.

> > how do you get the default of 20? isn't this something that should be
> > auto-scaled from the device in case future devices have a different
> > resolution?
> 
> It looks same regardless of the device size.  I checked several
> clickpad devices in different sizes, and all of them show mostly same
> button area, ca 20% of the touchpad size at the bottom.

is this area hardware-dependent though or can you click anywhere in the
touchpad? from the option I gather that it is flexible,, which would suggest
that we could reconfigure the whole thing to be much more flexible than just
the bottom button approach.
think of left edge is left button, right edge is right button.

> > >      if (pars->top_edge > pars->bottom_edge) {
> > > @@ -2052,6 +2054,56 @@ HandleClickWithFingers(SynapticsParameters *para, struct SynapticsHwState *hw)
> > >      }
> > >  }
> > >  
> > > +/* clickpad event handling */
> > > +static void
> > > +HandleClickpad(LocalDevicePtr local, struct SynapticsHwState *hw)
> > > +{
> > > +    SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> > > +    SynapticsParameters *para = &priv->synpara;
> > > +    int width = priv->maxx - priv->minx;
> > > +    int height = priv->maxy - priv->miny;
> > > +    int button_area_y, x_vscroll;
> > > +    int left_button_x, right_button_x;
> > > +
> > > +    /* left and right clickpad button ranges;
> > > +     * the gap between them is interpreted as a middle-button click
> > > +     */
> > > +    left_button_x = width * 2/ 5 + priv->minx;
> >                                 ^ space missing
> 
> Good catch.  (Don't we have a tool like checkpatch.pl for kernel..? :)

unfortunately not, because the coding style is nowhere near standardized.

> > > +    right_button_x = width * 3 / 5 + priv->minx;
> > > +    /* button area */
> > > +    button_area_y = priv->maxy - height * para->touch_button_area / 100;
> > > +    /* allow position change for the v-scroll area exceptionally */
> > > +    x_vscroll = priv->maxx - width / 15;
> > 
> > huh? what does this do? why don't you just use the right edge for this?
> 
> The problem is that the current synaptics driver is way too sensitive
> for the initial move.  On Windows, the pointer doesn't move quickly at
> first when you slide your finger.  It sticks at the position for a
> while, then starts moving.  On Linux, it reacts very fast from the
> very first moment.
> 
> This is basically fine, and I like better.  But, for clickpad, this is
> a disadvantage because the pointer often moves together at clicking.
> The parameter to disable touch-sensing at button area was introduced
> to avoid it.  But, at the same time, you do want sensing for the
> scrolling area even in the button area.  The above is to achieve such
> an exception.

ah, now I get it. should still be handled through the right edge setting
(and documented in the code so the next person knows what's going on).
though combined with the TouchpadArea that might be possible to fix this
otherwise - haven't looked at that in detail though.

can you also rename TouchButtonSense to TouchButtonDetect? 
that seems to be more self-explanatory (to me anyway :)

Cheers,
  Peter