On Fri, Jan 31, 2014 at 04:42:20PM -0800, Ping Cheng wrote:
> 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.
true, but we only call it if we've been unsucessful linking it the first
time, so this would have no effect. Feel free to change it though, it makes
it more obvious.
Cheers,
Peter
>
> 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
> > >
> >
------------------------------------------------------------------------------
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
