On Tue, Jan 28, 2014 at 7:01 PM, Peter Hutterer <peter.hutte...@who-t.net>wrote:
> 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.
>
We both missed part of the logic. It should be:
if (((tmpcommon->tablet_id == common->tablet_id) && !different_id) ||
((tmpcommon->tablet_id != common->tablet_id) && different_id)
Otherwise, we run devices with (tmpcommon->tablet_id == common->tablet_id)
again when different_id is TRUE.
Will update the set soon.
Ping
>
>
> > {
> > - 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
