[PATCH 00/18] Synaptics patches

[Peter Hutterer]

On Fri, Oct 08, 2010 at 07:22:24PM +0200, Takashi Iwai wrote:
> Hi,
> 
> [dropped linux-* ML since these are pure X patches]
> 
> here is a series of patches for xorg synaptics driver, mainly supporting
> Clickpad devices and multi-touch features.  It also contains some fixes,
> improvements of the current behavior, also the extension for sending key
> events, etc.
> 
> Not all patches are in quite good form, and definitely need discussions
> and/or fixes.  The whole bunch of patches have been pending for so
> long time by some (irritating) reason, and I feel relieved now.
> Everyone knows the same feeling in daily life, I guess, so, let me
> flush the patches from my queue first :)
> 
> (BTW, I'm on vacation until Monday; will reply after that)

Thanks Takashi,

(sorry if this mail addresses some things you've already commented on, I
wrote it while I was reviewing the patches)

Bit of a higher-level comment than the patch-specific ones. I just ran a
test of a few of them and they seem to work. Note that they need fixes to
compile against master, most notable LocalDevicePtr is gone from the server
now and needs to be replaced with InputInfoPtr (identical typedefs though).
So please replace any "LocalDevicePtr local" with "InputInfoPtr pInfo".

There is a random mix of tab/space indentations in your patches. Please
observe spaces only for newly written code, tabs where tabs are already
there, and common sense everywhere else.

The order of the patches is a bit confusing. e.g. in 01 you add the clickpad
support with a hardcoded button area, 02 is the man page, then you add the
configurable button area in 07 (as part of another, unrelated change). Just
split that out and add it to the first patch if you don't want the area to
be hardcoded.
I realise this was a flush of patches but making things easy to review
should generally be of a high priority. especially in terms of commit
messages, I want to know what you intended to do with a patch. 

In terms of property usage:
the interesting ones here are "Synaptics Capabilities" and "Synaptics
Gestures". Things like is_clickpad and the has_led should go into the former
to allow clients to change their UI based on this particular hardware.
The "Synaptics Gesture" is for low-level gestures, currently only
tap-and-drag, but the LED doubletap is an example for such a gesture that
could be added there.

multitouch support: this is too geared on the clickpad. I'd rather see a
generic multitouch support added, then the clickpad-specifics on top.

As for gestures in the driver - no. wacom has it because it plays by
slightly different rules, but that doesn't mean it's a good idea.
the driver is not the place to do gestures.

you send key events from gestures, yet the driver has no way of knowing when
the keymap is updated. so if you do so much as a group change, your ctrl+w
close gesture will suddenly mark everything (french azerty vs. us qwerty).
so each time you even do so much as change layouts, you'd have to reload the
property. which means you have to have a daemon in place that monitors the
current group, etc.

you have a gesture detection engine that's incompatible with every other one
in the system. so what's pinch for the synaptics driver is different to the
wacom driver's pinch, etc.

also, there's some inconsistency in the wording: you mix up pinch and zoom
in the first patch. just because pinch _can_ mean zoom doesn't mean
it always is. especially the way it's configured, you can set up pinch to do
a ctrl+ and ctrl-. except that that won't work in some apps, because they
(depending on kbd layout) need ctrl, shift, +, or other's don't zoom on that
shortcut at all. It's a mess already, no need to increase it and confuse it
further.

I think that about covers this patch set, hope I didn't forget anything.

Cheers,
  Peter