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 <[email protected]>
> Signed-off-by: Ping Chdeng <[email protected]>
> ---
> 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
