[PATCH 1/2] Add Clickpad support
Takashi Iwai
tiwai at suse.de
Tue Apr 20 23:16:40 PDT 2010
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.
> 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.
> > +.TP
> > +.BI "Option \*qTouchButtonSense\*q \*q" integer \*q
> > +Disable the touch-sensing in the Clickpad button area.
> > +.
> > +Since the touchpad is often quite sensitive, the pointer would move
> > +slightly when you click via Clickpad, and user may miss the aimed
> > +position. When this option is set, the cursor movement in the
> > +Clickpad button area is suppressed, thus the point won't move while
> > +clicking the button. On the other hand, this reduces the available
> > +space on the touchpad. The default is on.
> > +Property: "Synaptics Touch Button Sense"
> > +.
> > .LP
> > A tap event happens when the finger is touched and released in a time
> > interval shorter than MaxTapTime, and the touch and release
> > diff --git a/src/eventcomm.c b/src/eventcomm.c
> > index d00d810..ca99f3a 100644
> > --- a/src/eventcomm.c
> > +++ b/src/eventcomm.c
> > @@ -166,6 +166,19 @@ event_query_info(LocalDevicePtr local)
> > }
> > }
> >
> > +static void event_query_clickpad(LocalDevicePtr local)
> > +{
> > + SynapticsPrivate *priv = (SynapticsPrivate *)local->private;
> > +
> > + priv->is_clickpad = FALSE;
> > +
> > + /* clickpad device reports only the single left button mask */
> > + if (priv->has_left && !priv->has_right && !priv->has_middle) {
> > + priv->is_clickpad = TRUE;
> > + xf86Msg(X_INFO, "%s: is Clickpad device\n", local->name);
>
> please move this check into the part that checks for LMR buttons.
OK.
> > + }
> > +}
> > +
> > /* Query device for axis ranges */
> > static void
> > event_query_axis_ranges(LocalDevicePtr local)
> > @@ -253,6 +266,8 @@ event_query_axis_ranges(LocalDevicePtr local)
> > strcat(buf, " triple");
> > xf86Msg(X_INFO, "%s: buttons:%s\n", local->name, buf);
> > }
> > +
> > + event_query_clickpad(local);
> > }
> >
> > static Bool
> > diff --git a/src/properties.c b/src/properties.c
> > index 4366034..a579479 100644
> > --- a/src/properties.c
> > +++ b/src/properties.c
> > @@ -84,6 +84,8 @@ Atom prop_gestures = 0;
> > Atom prop_capabilities = 0;
> > Atom prop_resolution = 0;
> > Atom prop_area = 0;
> > +Atom prop_touch_button_area = 0;
> > +Atom prop_touch_button_sense = 0;
> >
> > static Atom
> > InitAtom(DeviceIntPtr dev, char *name, int format, int nvalues, int *values)
> > @@ -274,6 +276,10 @@ InitDeviceProperties(LocalDevicePtr local)
> > values[2] = para->area_top_edge;
> > values[3] = para->area_bottom_edge;
> > prop_area = InitAtom(local->dev, SYNAPTICS_PROP_AREA, 32, 4, values);
> > +
> > + prop_touch_button_area = InitAtom(local->dev, SYNAPTICS_PROP_TOUCH_BUTTON_AREA, 8, 1, ¶->touch_button_area);
> > +
> > + prop_touch_button_sense = InitAtom(local->dev, SYNAPTICS_PROP_TOUCH_BUTTON_SENSE, 8, 1, ¶->touch_button_sense);
> > }
> >
> > int
> > @@ -490,6 +496,7 @@ SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
> > return BadValue;
> >
> > para->touchpad_off = off;
> > +
> > } else if (property == prop_guestmouse)
> > {
> > if (prop->size != 1 || prop->format != 8 || prop->type != XA_INTEGER)
> > @@ -642,6 +649,18 @@ SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
> > para->area_right_edge = area[1];
> > para->area_top_edge = area[2];
> > para->area_bottom_edge = area[3];
> > + } else if (property == prop_touch_button_area)
> > + {
> > + if (prop->size != 1 || prop->format != 8 || prop->type != XA_INTEGER)
> > + return BadMatch;
> > +
> > + para->touch_button_area = *(CARD8*)prop->data;
> > + } else if (property == prop_touch_button_sense)
> > + {
> > + if (prop->size != 1 || prop->format != 8 || prop->type != XA_INTEGER)
> > + return BadMatch;
> > +
> > + para->touch_button_sense = *(BOOL*)prop->data;
> > }
> >
> > return Success;
> > diff --git a/src/synaptics.c b/src/synaptics.c
> > index 091dbe1..f701074 100644
> > --- a/src/synaptics.c
> > +++ b/src/synaptics.c
> > @@ -532,6 +532,8 @@ static void set_default_parameters(LocalDevicePtr local)
> > pars->tap_and_drag_gesture = xf86SetBoolOption(opts, "TapAndDragGesture", TRUE);
> > pars->resolution_horiz = xf86SetIntOption(opts, "HorizResolution", horizResolution);
> > pars->resolution_vert = xf86SetIntOption(opts, "VertResolution", vertResolution);
> > + pars->touch_button_area = xf86SetIntOption(opts, "TouchButtonArea", 20);
> > + pars->touch_button_sense = xf86SetBoolOption(opts, "TouchButtonSense", FALSE);
> >
> > /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge parameters */
> > 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..? :)
> > + 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.
> also, you're hardcoding the width as 1/15 from the right - what happens if
> someone changes the edge settings?
That's a left-over code I've used from the old patch. Should be fixed.
thanks,
Takashi
More information about the xorg-devel
mailing list