On Wed, Mar 30, 2011 at 11:17 PM, Peter Hutterer
<[email protected]>wrote:
> > {
> > - temppriv = (WacomDevicePtr)
> device->private;
> > - tempcommon = temppriv->common;
> > -
> > - if ((tempcommon->tablet_id ==
> common->tablet_id) &&
> > - IsTouch(temppriv) &&
> temppriv->oldProximity)
> > - {
> > - /* Send soft prox-out for touch
> first */
> > - wcmSoftOutEvent(device);
> > - }
> > + /* Send touch out so pen takes control */
> > +
> wcmSoftOutEvent(common->wcmPointerToTouch->pInfo);
> > + return;
> > }
> > }
> > + else if (IsTouch(priv) && common->wcmPenInProx)
> > + /* Ignore touch events when pen is in prox */
> > + return;
>
> we won't ever get to this condition, IsPen(priv) is always true in this
> block
>
Right, it should be outside of the if-statement.
> }
> >
> > if (IsPen(priv))
> > diff --git a/src/wcmConfig.c b/src/wcmConfig.c
> > index 6235d3c..21a124e 100644
> > --- a/src/wcmConfig.c
> > +++ b/src/wcmConfig.c
> > @@ -397,6 +397,45 @@ wcmInitModel(InputInfoPtr pInfo)
> > return TRUE;
> > }
> >
> > +/* Link the touch tool to the pen of the same device
>
> asciidoc /** please
>
Oh, I only remembered there is no need to specify pInfo and common. I forgot
this detail.
> + * so we can arbitrate the events.
> > + */
> > +static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
> > +{
> > + WacomDevicePtr priv = pInfo->private;
> > + WacomCommonPtr common = priv->common;
> > + InputInfoPtr device = xf86FirstLocalDevice();
> > + WacomCommonPtr tmpcommon = NULL;
> > + WacomDevicePtr tmppriv = NULL;
> > +
> > + /* Lookup to find the associated pen and touch */
> > + for (; device != NULL; device = device->next)
> > + {
> > + if (strstr(device->drv->driverName, "wacom"))
> > + {
> > + tmppriv = (WacomDevicePtr) device->private;
> > + tmpcommon = tmppriv->common;
> > +
>
> I'm a big fan of using continues for simple conditions to skip. in this
> case, a
> if (tmppriv == priv)
> continue;
>
> rather than adding this to the next condition that deals with the actual
> comparison of the tools.
>
No problem. Will do.
> > + if ((tmpcommon->tablet_id == common->tablet_id) &&
> > + tmppriv != priv)
> > + {
> > + if (IsTouch(tmppriv) && IsPen(priv))
> > + {
> > + common->wcmPointerToTouch =
> tmppriv;
> > + common->tablet_type |=
> WCM_PENTOUCH;
> > + tmpcommon->tablet_type |=
> WCM_PENTOUCH;
> > + }
> > + else if (IsTouch(priv) && IsPen(tmppriv))
> > + {
> > + tmpcommon->wcmPointerToTouch =
> priv;
> > + common->tablet_type |=
> WCM_PENTOUCH;
> > + tmpcommon->tablet_type |=
> WCM_PENTOUCH;
> > + }
>
>
> this can be simplified with the if/else if condition setting a tmp variable
> for the new priv assignment and then a shared block for the three actual
> assignments.
>
Not really. Although priv can be set to tmppriv, both common and tmpcommon
are needed which can not be used with one name. Plus, since it is a if/else
if, there would be cases outside of those statements that we should not
assign any values except continue.
> + }
> > + }
> > + }
> > +}
> > +
> > /* wcmPreInit - called for each input devices with the driver set to
> > * "wacom" */
> > #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 12
> > @@ -518,6 +557,12 @@ static int wcmPreInit(InputDriverPtr drv,
> InputInfoPtr pInfo, int flags)
> > pInfo->fd = -1;
> > }
> >
> > + /* only link them once per port. We need to try for both pen and
> touch
> > + * since we do not know which tool (touch or pen) will be added
> first.
> > + */
> > + if (IsTouch(priv) || (IsPen(priv) && !common->wcmPointerToTouch))
> > + wcmLinkTouchAndPen(pInfo);
> > +
> > return Success;
> >
> > SetupProc_fail:
> > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
> > index 43348fc..60d6426 100644
> > --- a/src/xf86WacomDefs.h
> > +++ b/src/xf86WacomDefs.h
> > @@ -173,6 +173,7 @@ struct _WacomModel
> > #define WCM_TPC (0x00000200 | WCM_LCD) /* TabletPC
> (special
> > button handling,
> > always an LCD) */
> > +#define WCM_PENTOUCH 0x00000400 /* Tablet supports pen and touch
> */
> > #define TabletHasFeature(common, feature) (((common)->tablet_type &
> (feature)) != 0)
> >
> > #define ABSOLUTE_FLAG 0x00000100
> > @@ -421,6 +422,9 @@ struct _WacomCommonRec
> > int fd; /* file descriptor to tablet */
> > int fd_refs; /* number of references to fd; if =0,
> fd is invalid */
> > unsigned long wcmKeys[NBITS(KEY_MAX)]; /* supported tool types for
> the device */
> > + WacomDevicePtr wcmPointerToTouch; /* The pointer for pen to access
> the
> > + touch tool of the same device id
> */
>
> the term pointer in X is so badly overloaded that I can't recommend using
> it. how about just "wcmTouchDevice"?
>
> keeping this pointer gives us a race condition similar to the hotplugging
> one we had a while ago. if the touch device is removed first and tablet
> still sends event, the new block in commonDispatchDevice may dereference
> the
> already deleted pointer.
> You need a matching wcmUnlinkTouchAndPen() called during DEVICE_OFF.
>
I will think about it a bit more and do some testing to be sure.
Thank you for the review. I'll make a v2 after I hear your reply about the
if/else if statement discussion.
Ping
------------------------------------------------------------------------------
Create and publish websites with WebMatrix
Use the most popular FREE web apps or write code yourself;
WebMatrix provides all the features you need to develop and
publish your website. http://p.sf.net/sfu/ms-webmatrix-sf
_______________________________________________
Linuxwacom-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
