On Thu, Jun 18, 2009 at 7:49 PM, Peter Hutterer<peter.hutte...@who-t.net> wrote:
> Thanks for the patch. A few comments though:
>
> On Thu, Jun 18, 2009 at 01:56:49AM -0400, Ben Gamari wrote:
>> diff --git a/src/synaptics.c b/src/synaptics.c
>> index ff66517..f917d8b 100644
>> --- a/src/synaptics.c
>> +++ b/src/synaptics.c
>> @@ -69,6 +69,11 @@
>> #include <xf86Xinput.h>
>> #include <exevents.h>
>>
>> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 3
>> +#include <X11/Xatom.h>
>> +#include <xserver-properties.h>
>> +#endif
>> +
>
> this should strictly be >= 7, since we don't need these parts in earlier
> versions.
Good call.
>
>> #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) == 0
>> #include "mipointer.h"
>> #endif
>> @@ -795,6 +800,20 @@ DeviceInit(DeviceIntPtr dev)
>> SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
>> unsigned char map[SYN_MAX_BUTTONS + 1];
>> int i;
>> + Atom axis_labels[] = {
>> + XIGetKnownProperty(AXIS_LABEL_PROP_ABS_X),
>> + XIGetKnownProperty(AXIS_LABEL_PROP_ABS_Y),
>> + };
>> + Atom btn_labels[] = {
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_LEFT),
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_MIDDLE),
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_RIGHT),
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_UP),
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_DOWN),
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_LEFT),
>> + XIGetKnownProperty(BTN_LABEL_PROP_BTN_WHEEL_RIGHT),
>> + // FIXME: I don't know what the other buttons might be
>> + };
>
> no // comments please.
> also, this part is incorrect. SYN_MAX_BUTTONS is 12, so when you pass this
> array into InitPointerDeviceStruct, it'll run over the bounds and produce
> bogus data.
> fwiw, label of 0 ("None") is permitted.
Yeah, this is true. Perhaps my introduction message didn't come across
the list, but this patch was introduced as an RFC for this exact
reason. Strictly speaking I'm not even completely sure of the button
labels I've given here. Someone with greater knowledge of the driver
will need to comment here.
>
>> @@ -805,6 +824,9 @@ DeviceInit(DeviceIntPtr dev)
>>
>> InitPointerDeviceStruct((DevicePtr)dev, map,
>> SYN_MAX_BUTTONS,
>> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>> + btn_labels,
>> +#endif
>> #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) == 0
>> miPointerGetMotionEvents,
>> #elif GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 3
>> @@ -813,21 +835,40 @@ DeviceInit(DeviceIntPtr dev)
>> SynapticsCtrl,
>> #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) == 0
>> miPointerGetMotionBufferSize()
>> -#else
>> +#elif GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 7
>> GetMotionHistorySize(), 2
>> +#else
>> + GetMotionHistorySize(), 2, axis_labels
>> #endif
>> );
>> /* X valuator */
>> if (priv->minx < priv->maxx)
>> - xf86InitValuatorAxisStruct(dev, 0, priv->minx, priv->maxx, 1, 0, 1);
>> + xf86InitValuatorAxisStruct(dev, 0,
>> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>> + axis_labels[0],
>> +#endif
>> + priv->minx, priv->maxx, 1, 0, 1);
>> else
>> - xf86InitValuatorAxisStruct(dev, 0, 0, -1, 1, 0, 1);
>> + xf86InitValuatorAxisStruct(dev, 0,
>> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>> + axis_labels[0],
>> +#endif
>> + 0, -1, 1, 0, 1);
>> xf86InitValuatorDefaults(dev, 0);
>> +
>> /* Y valuator */
>> if (priv->miny < priv->maxy)
>> - xf86InitValuatorAxisStruct(dev, 1, priv->miny, priv->maxy, 1, 0, 1);
>> + xf86InitValuatorAxisStruct(dev, 1,
>> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>> + axis_labels[1],
>> +#endif
>> + priv->miny, priv->maxy, 1, 0, 1);
>> else
>> - xf86InitValuatorAxisStruct(dev, 1, 0, -1, 1, 0, 1);
>> + xf86InitValuatorAxisStruct(dev, 1,
>> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>> + axis_labels[1],
>> +#endif
>> + 0, -1, 1, 0, 1);
>> xf86InitValuatorDefaults(dev, 1);
>>
>> #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) == 0
>
> messy, hence the need for fd939a37d7df320f76fc772eb1f18eb6ba1d54b9.
Yeah, I absolutely agree. This is #ifdef hell.
> How about this one (obviously on top of fd939)?
Looks good (once someone figures out the button label situation).
_______________________________________________
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
