On Fri, Jan 24, 2014 at 05:23:47PM -0800, Ping Cheng wrote:
> Tools defined for devices that support both pen and touch need
> to access the touch tool for arbitration and touch switch state
> updates. Initially searching for the touch tool and storing it
> to common->wcmTouchDevice saves time later when we need to access it.
> 
> TabletFeature is updated with WCM_PENTOUCH for devices that
> support both pen and touch too.
> 
> Reviewed-by: Aaron Armstrong Skomra <sko...@gmail.com>
> Signed-off-by: Ping Chdeng <pi...@wacom.com>
> ---
>  src/wcmConfig.c | 101 
> +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 52 insertions(+), 49 deletions(-)
> 
> diff --git a/src/wcmConfig.c b/src/wcmConfig.c
> index 2d19944..a6d4fdc 100644
> --- a/src/wcmConfig.c
> +++ b/src/wcmConfig.c
> @@ -385,11 +385,17 @@ wcmInitModel(InputInfoPtr pInfo)
>       return TRUE;
>  }
>  
> +
>  /**
> - * Link the touch tool to the pen of the same device
> - * so we can arbitrate the events when posting them.
> + * Lookup to find the associated pen and touch for the same device.
> + * Store touch tool in wcmTouchDevice for pen and touch, respectively,
> + * of the same device. Update TabletFeature to indicate it is a hybrid
> + * of touch and pen.
> + *
> + * @return True if found a touch tool for hybrid device.
> + * false otherwise.
>   */
> -static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
> +static Bool wcmPenAndTouch(InputInfoPtr pInfo, Bool different_id)

Please document the "different_id" parameter as well, so it's obvious from
the outset what it does. Maybe rename it to "allow_different_ids".

>  {
>       WacomDevicePtr priv = pInfo->private;
>       WacomCommonPtr common = priv->common;
> @@ -397,65 +403,62 @@ static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
>       WacomCommonPtr tmpcommon = NULL;
>       WacomDevicePtr tmppriv = NULL;
>  
> -     /* Lookup to find the associated pen and touch with same product id */
>       for (; device != NULL; device = device->next)
>       {
> -             if (!strcmp(device->drv->driverName, "wacom"))
> -             {
> -                     tmppriv = (WacomDevicePtr) device->private;
> -                     tmpcommon = tmppriv->common;
> +             if (strcmp(device->drv->driverName, "wacom"))
> +                     continue;
>  
> -                     /* skip the same tool or already linked devices */
> -                     if ((tmppriv == priv) || tmpcommon->wcmTouchDevice)
> -                             continue;
> +             tmppriv = (WacomDevicePtr) device->private;
> +             tmpcommon = tmppriv->common;
>  
> -                     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);
> -                             }
> -                     }
> +             /* skip the same tool or already linked devices */
> +             if ((tmppriv == priv) ||
> +                 (tmpcommon->wcmTouchDevice && IsTablet(tmppriv)))
> +                     continue;
>  
> -                     if (common->wcmTouchDevice)
> -                             return;
> -             }
> -     }
> -
> -     /* 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) && 
> !different_id)
> +                 || different_id)

  if (((tmpcommon->tablet_id == common->tablet_id) || different_id)

would be enough here for our purposes.


>               {
> -                     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))
> +                     if (IsTouch(tmppriv))
> +                     {
>                               common->wcmTouchDevice = tmppriv;
> -                     else if (IsTouch(priv) && IsTablet(tmppriv))
> +                             tmpcommon->wcmTouchDevice = tmppriv;
> +                     } else if (IsTouch(priv))
> +                     {
>                               tmpcommon->wcmTouchDevice = priv;
> +                             common->wcmTouchDevice = priv;
> +                     }
>  
> -                     if (common->wcmTouchDevice || tmpcommon->wcmTouchDevice)
> +                     if ((common->wcmTouchDevice && IsTablet(priv)) ||
> +                         (tmpcommon->wcmTouchDevice && IsTablet(tmppriv)))

NAK to this. Please split this patch into the cleanup patch and a separate
patch where the actual changes are made. Burying minor changes like this
inside a cleanup patch is a bad idea, it makes it really hard to find what
goes wrong in the future (or even figure out what's different).

What I see here is that there are some checks for IsTablet removed and
others added, but it's not clear if that's a need for this patch or
something else.

>                       {
>                               TabletSetFeature(common, WCM_PENTOUCH);
>                               TabletSetFeature(tmpcommon, WCM_PENTOUCH);
>                       }
> -
> -                     if (common->wcmTouchDevice)
> -                             return;
>               }
> +
> +             if (common->wcmTouchDevice)
> +                     return TRUE;
>       }
> +     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)
> +{
> +     /* pen and touch with same product id */
> +     if (wcmPenAndTouch(pInfo, FALSE))
> +             return;
> +     else
> +             /* pen and touch devices with different product ids */
> +             wcmPenAndTouch(pInfo, TRUE);

why not the folowing?

> +     if (!wcmPenAndTouch(pInfo, FALSE))
> +             wcmPenAndTouch(pInfo, TRUE); /* pen and touch devices with 
> different product ids */


Cheers,
   Peter

>  }
>  
>  /**
> @@ -606,11 +609,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
> 

------------------------------------------------------------------------------
WatchGuard Dimension instantly turns raw network data into actionable 
security intelligence. It gives you real-time visual feedback on key
security issues and trends.  Skip the complicated setup - simply import
a virtual appliance and go from zero to informed in seconds.
http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

Reply via email to