On Mon, Feb 3, 2014 at 5:43 PM, Peter Hutterer <peter.hutte...@who-t.net> wrote:
> On Mon, Feb 03, 2014 at 04:20:26PM -0800, Ping Cheng wrote:
>> Reuse the routine that links tools with the same product ID devices
>> for different product ID devices to avoid duplicated code.
>>
>> Signed-off-by: Ping Cheng <pi...@wacom.com>
>> ---
>> src/wcmConfig.c | 79
>> +++++++++++++++++++++++++--------------------------------
>> 1 file changed, 35 insertions(+), 44 deletions(-)
>>
>> diff --git a/src/wcmConfig.c b/src/wcmConfig.c
>> index 2d19944..103fc15 100644
>> --- a/src/wcmConfig.c
>> +++ b/src/wcmConfig.c
>> @@ -388,8 +388,13 @@ wcmInitModel(InputInfoPtr pInfo)
>> /**
>> * Link the touch tool to the pen of the same device
>> * so we can arbitrate the events when posting them.
>> + *
>> + * @param allow_different_id TRUE to check different product IDs
>> + *
>> + * @return True if found a touch tool for hybrid devices.
>> + * false otherwise.
>> */
>> -static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
>> +static Bool wcmLinkTouchAndPen(InputInfoPtr pInfo, Bool allow_different_id)
>> {
>> WacomDevicePtr priv = pInfo->private;
>> WacomCommonPtr common = priv->common;
>> @@ -397,50 +402,22 @@ static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
>> WacomCommonPtr tmpcommon = NULL;
>> WacomDevicePtr tmppriv = NULL;
>>
>> - /* Lookup to find the associated pen and touch with same product id */
>> + /* Lookup to find the associated pen and touch */
>> for (; device != NULL; device = device->next)
>> {
>> - if (!strcmp(device->drv->driverName, "wacom"))
>> - {
>> - tmppriv = (WacomDevicePtr) device->private;
>> - tmpcommon = tmppriv->common;
>> -
>> - /* skip the same tool or already linked devices */
>> - if ((tmppriv == priv) || tmpcommon->wcmTouchDevice)
>> - continue;
>> + if (strcmp(device->drv->driverName, "wacom"))
>> + continue;
>>
>> - if (tmpcommon->tablet_id == common->tablet_id)
>> - {
>> - if (IsTouch(tmppriv) && IsTablet(priv))
>> - common->wcmTouchDevice = tmppriv;
>> - else if (IsTouch(priv) && IsTablet(tmppriv))
>> - tmpcommon->wcmTouchDevice = priv;
>> -
>> - if (common->wcmTouchDevice ||
>> - tmpcommon->wcmTouchDevice)
>> - {
>> - TabletSetFeature(common, WCM_PENTOUCH);
>> - TabletSetFeature(tmpcommon,
>> WCM_PENTOUCH);
>> - }
>> - }
>> + tmppriv = (WacomDevicePtr) device->private;
>> + tmpcommon = tmppriv->common;
>>
>> - if (common->wcmTouchDevice)
>> - return;
>> - }
>> - }
>> + /* skip the same tool or already linked devices */
>> + if ((tmppriv == priv) || tmpcommon->wcmTouchDevice)
>> + continue;
>>
>> - /* Lookup for pen and touch devices with different product ids */
>> - for (; device != NULL; device = device->next)
>> - {
>> - if (!strcmp(device->drv->driverName, "wacom"))
>> + if (((tmpcommon->tablet_id == common->tablet_id) &&
>> !allow_different_id) ||
>> + ((tmpcommon->tablet_id == common->tablet_id) &&
>> allow_different_id))
>
> shouldn't the second line be "!=" ? (
>
> with that change, Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>
>
> Cheers,
> Peter
>
>
>> {
>> - tmppriv = (WacomDevicePtr) device->private;
>> - tmpcommon = tmppriv->common;
>> -
>> - /* skip the same tool or already linked devices */
>> - if ((tmppriv == priv) || tmpcommon->wcmTouchDevice)
>> - continue;
>> -
>> if (IsTouch(tmppriv) && IsTablet(priv))
>> common->wcmTouchDevice = tmppriv;
>> else if (IsTouch(priv) && IsTablet(tmppriv))
>> @@ -451,11 +428,25 @@ static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
>> TabletSetFeature(common, WCM_PENTOUCH);
>> TabletSetFeature(tmpcommon, WCM_PENTOUCH);
>> }
>> -
>> - if (common->wcmTouchDevice)
>> - return;
>> }
>> + if (common->wcmTouchDevice)
>> + return TRUE;
The `allow_different_id` case is a recipe for this to always succeed
and result in bogus connections between unrelated devices. The problem
existed prior to this patch, but was impotent due to the effective
short-circuit in the original second for-loop. Now, any pen-only
device or device with pen and touch on separate PIDs will find
themselves linked to the first available touch device!
The "skip the same tool or already linked devices" test above might
have been intended to filter these out, but because wcmTouchDevice
isn't set on the touch side of the connection, it doesn't actually
help. Since the point of your next patch is to set the variable on
both the pen *and* touch side, it would appear to be solved in patch
2, but... Well, I'll put the comment on that patch :)
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
>> }
>> + return FALSE;
>> +}
>> +
>> +/**
>> + * Update festures for tablets that support both pen and touch.
>> + * Pen and touch of the same device can have same product id,
>> + * such as Intuos and most Cintiq series; or different product
>> + * ids, such as Cintiq 24HD. We look for pen and touch with same
>> + * product id first. If we do not find a touch tool, devices
>> + * with different product ids will be searched.
>> + */
>> +static void wcmUpdatePenAndTouchInfo(InputInfoPtr pInfo)
>> +{
>> + if (!wcmLinkTouchAndPen(pInfo, FALSE))
>> + wcmLinkTouchAndPen(pInfo, TRUE);
>> }
>>
>> /**
>> @@ -606,11 +597,11 @@ static int wcmPreInit(InputDriverPtr drv, InputInfoPtr
>> pInfo, int flags)
>> pInfo->fd = -1;
>> }
>>
>> - /* only link them once per port. We need to try for both tablet tool
>> + /* only update once per port. We need to try for both tablet tool
>> * and touch since we do not know which tool will be added first.
>> */
>> if (IsTouch(priv) || (IsTablet(priv) && !common->wcmTouchDevice))
>> - wcmLinkTouchAndPen(pInfo);
>> + wcmUpdatePenAndTouchInfo(pInfo);
>>
>> free(type);
>> free(oldname);
>> --
>> 1.8.3.2
>>
>
> ------------------------------------------------------------------------------
> Managing the Performance of Cloud-Based Applications
> Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
> Read the Whitepaper.
> http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.clktrk
> _______________________________________________
> Linuxwacom-devel mailing list
> Linuxwacom-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.clktrk
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
