[PATCH 00/18] Synaptics patches

[Takashi Iwai]

At Wed, 13 Oct 2010 14:29:09 +1000,
Peter Hutterer wrote:
> 
> 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".

Ah, OK.  I had no time to actually do testing with master.
Will check later.

> 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.

Noted.

> 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. 

Sure, the patches are mostly in the historical order.  (Man's activity
isn't always explained logically, no? :)

The reason why the configurable parameter for clickpad wasn't in the
first patch is that the first patch (ab-)uses the bottom_edge as the
button position.  In the later patch, the button area is handled again
as a normal touch area, thus this condition was removed, and a new
parameter was introduced instead.


> 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.

Noted.

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

OK.

> 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.

Heh, this is an expected answer.

> 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.

Hm, can't it be hooked and detected in the driver routine...?

I find sending key events more usable in the current situation.
Of course, we can create a new set of buttons for gesture actions, but
it'll take ages until apps accept new button events.  OTOH, keys are
already defined, and applicable as is.  That is, sending keys was the
only option to make things work at this moment.

> 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.

Hm, a mess over a mess.  I hate it, too.


thanks,

Takashi