В Птн, 30/12/2011 в 13:54 -0600, Chris Bagwell пишет:
> I'm glad someone decided to add this! I wanted to but been to busy.
>
> Similar to Peter's past comments on the file wcmTouchFilter.c, this
> code is almost obsolete before being published and so I'm not
> reviewing very strict.
Could you please point me to that Perer's comment? I'm not quite
understand what you are meaning.
> Comments below.
>
> On Sat, Dec 24, 2011 at 12:18 AM, Alexey Osipov <si...@lerlan.ru> wrote:
> > First, we define two new GESTURE_ modes:
> > - GESTURE_PREDRAG_MODE - when first tap happen and we wait for second touch.
> > - GESTURE_DRAG_MODE - when actual drag happening (left button pressed).
> >
> > Second, we define tap timeout function wcmSingleFingerTapTimer(), which
> > simulate single click if no drag operation started within timeout.
> >
> > Third, we make an exception for GESTURE_DRAG_MODE in wcmCommon.c, because
> > we actually want cursor movements while dragging.
> >
> > Now, to do a single tap you just tap (touch and untouch). Actual click
> > happens after TapTime period.
> > To drag something you make a tap (touch and untouch) and then quickly
> > (in TapTime period) touch device again. Then drag.
> >
> > Signed-off-by: Alexey Osipov <si...@lerlan.ru>
> > ---
> > src/common.mk | 3 +-
> > src/wcmCommon.c | 8 +++---
> > src/wcmTouchFilter.c | 54
> > ++++++++++++++++++++++++++++++++++++++++---------
> > src/wcmTouchFilter.h | 38 +++++++++++++++++++++++++++++++++++
> > src/xf86Wacom.h | 1 -
> > 5 files changed, 88 insertions(+), 16 deletions(-)
> > create mode 100644 src/wcmTouchFilter.h
> >
> > diff --git a/src/common.mk b/src/common.mk
> > index fae8d9f..7f1eadd 100644
> > --- a/src/common.mk
> > +++ b/src/common.mk
> > @@ -12,4 +12,5 @@ DRIVER_SOURCES= \
> > $(top_srcdir)/src/wcmUSB.c \
> > $(top_srcdir)/src/wcmXCommand.c \
> > $(top_srcdir)/src/wcmValidateDevice.c \
> > - $(top_srcdir)/src/wcmTouchFilter.c
> > + $(top_srcdir)/src/wcmTouchFilter.c \
> > + $(top_srcdir)/src/wcmTouchFilter.h
> > diff --git a/src/wcmCommon.c b/src/wcmCommon.c
> > index d0bff63..0cab803 100644
> > --- a/src/wcmCommon.c
> > +++ b/src/wcmCommon.c
> > @@ -24,6 +24,7 @@
> > #include "xf86Wacom.h"
> > #include "Xwacom.h"
> > #include "wcmFilter.h"
> > +#include "wcmTouchFilter.h"
> > #include <xkbsrv.h>
> > #include <xf86_OSproc.h>
> >
> > @@ -743,8 +744,8 @@ void wcmSendEvents(InputInfoPtr pInfo, const
> > WacomDeviceState* ds)
> > valuators[4] = v4;
> > valuators[5] = v5;
> >
> > - /* don't move the cursor if in gesture mode */
> > - if (!(!is_absolute(pInfo) && priv->common->wcmGestureMode)) {
> > + /* don't move the cursor if in gesture mode (except drag mode) */
> > + if (!(!is_absolute(pInfo) && priv->common->wcmGestureMode &
> > ~GESTURE_DRAG_MODE)) {
> > if (type == PAD_ID)
> > wcmSendPadEvents(pInfo, ds, 3, 3, &valuators[3]); /*
> > pad doesn't post x/y/z */
> > else
> > @@ -945,8 +946,7 @@ void wcmEvent(WacomCommonPtr common, unsigned int
> > channel,
> > if ((ds.device_type == TOUCH_ID) && common->wcmTouch)
> > wcmGestureFilter(priv, channel);
> >
> > - /* don't move the cursor if in gesture mode */
> > - if ((priv->flags & ABSOLUTE_FLAG) && (common->wcmGestureMode))
> > + if ((priv->flags & ABSOLUTE_FLAG) && (common->wcmGestureMode &
> > ~GESTURE_DRAG_MODE))
> > return;
>
> These would need to align with comment on previous patch.
Agreed.
> And more a personal opinion, I think the wcmGestureMode values are
> meant to be more internal to the touch filter engine and shouldn't be
> exposed to wcmCommon.c. I'd prefer if you exported a new function
> with name like wcmTouchNeedsFilter() that returns yes/no answer and
> then you add exact values to check inside wcmTouchFilter.c.
Yes, this can be done that way. But isn't calling extra function will
slow down overall events processing? I don't think that extra CPU usage
in driver is a good idea. What you think?
> >
> > /* For touch, only first finger moves the cursor */
> > diff --git a/src/wcmTouchFilter.c b/src/wcmTouchFilter.c
> > index 047490b..c1a6071 100644
> > --- a/src/wcmTouchFilter.c
> > +++ b/src/wcmTouchFilter.c
> > @@ -1,5 +1,6 @@
> > /*
> > * Copyright 2009 - 2010 by Ping Cheng, Wacom. <pi...@wacom.com>
> > + * Copyright 2011 by Alexey Osipov. <si...@lerlan.ru>
> > *
> > * This program is free software; you can redistribute it and/or
> > * modify it under the terms of the GNU General Public License
> > @@ -21,6 +22,7 @@
> > #endif
> >
> > #include "xf86Wacom.h"
> > +#include "wcmTouchFilter.h"
> > #include <math.h>
> >
> > /* Defines for 2FC Gesture */
> > @@ -28,12 +30,6 @@
> > #define WACOM_VERT_ALLOWED 2
> > #define WACOM_GESTURE_LAG_TIME 10
> >
> > -#define GESTURE_NONE_MODE 0
> > -#define GESTURE_TAP_MODE 1
> > -#define GESTURE_SCROLL_MODE 2
> > -#define GESTURE_ZOOM_MODE 4
> > -#define GESTURE_LAG_MODE 8
> > -
> > #define WCM_SCROLL_UP 5 /* vertical up */
> > #define WCM_SCROLL_DOWN 4 /* vertical down */
> > #define WCM_SCROLL_LEFT 6 /* horizontal left */
> > @@ -147,6 +143,23 @@ static void wcmFingerTapToClick(WacomDevicePtr priv)
> > }
> > }
> >
> > +static CARD32 wcmSingleFingerTapTimer(OsTimerPtr timer, CARD32 time,
> > pointer arg)
> > +{
> > + WacomDevicePtr priv = (WacomDevicePtr)arg;
> > + WacomCommonPtr common = priv->common;
> > +
> > + if (common->wcmGestureMode == GESTURE_PREDRAG_MODE)
> > + {
> > + /* left button down */
> > + wcmSendButtonClick(priv, 1, 1);
> > +
> > + /* left button up */
> > + wcmSendButtonClick(priv, 1, 0);
> > + common->wcmGestureMode = GESTURE_NONE_MODE;
> > + }
> > +
> > + return 0;
> > +}
> >
> > /* A single finger tap is defined as 1 finger tap that lasts less than
> > * wcmTapTime. It results in a left button press.
> > @@ -185,11 +198,10 @@ static void wcmSingleFingerTap(WacomDevicePtr priv)
> > common->wcmGestureParameters.wcmTapTime &&
> > ds[1].sample < dsLast[0].sample)
> > {
> > - /* left button down */
> > - wcmSendButtonClick(priv, 1, 1);
> > + common->wcmGestureMode = GESTURE_PREDRAG_MODE;
> >
> > - /* left button up */
> > - wcmSendButtonClick(priv, 1, 0);
> > + /* Delay to detect possible drag operation */
> > + TimerSet(NULL, 0,
> > common->wcmGestureParameters.wcmTapTime, wcmSingleFingerTapTimer, priv);
> > }
> > }
> > }
> > @@ -244,6 +256,17 @@ void wcmGestureFilter(WacomDevicePtr priv, int channel)
> > if (!common->wcmGesture)
> > goto ret;
> >
> > + if (common->wcmGestureMode == GESTURE_DRAG_MODE)
> > + {
> > + if (!ds[0].proximity)
> > + {
> > + /* left button up */
> > + wcmSendButtonClick(priv, 1, 0);
> > + common->wcmGestureMode = GESTURE_NONE_MODE;
> > + }
> > + goto ret;
> > + }
> > +
>
> This feels out of place at the top of function. I'm pretty sure it
> would live better inside the "if (!ds[0].proximity &&
> !ds[1].proximity)" were we are already doing cleanup gesture logic
> related to two fingers not touching.
Without other changes it can't, actually. If I move this code below,
then
-------------------------------------------------------------------------
else if (dsLast[0].proximity)
{
...
else
{
/* Been in LAG mode long enough. Force to NONE mode. */
common->wcmGestureMode = GESTURE_NONE_MODE;
}
}
-------------------------------------------------------------------------
will fire, setting wcmGestureMode to NONE too early (while dragging is still
active).
We can, of course change this to
-------------------------------------------------------------------------
else if (dsLast[0].proximity)
{
...
else
{
/* Been in LAG mode long enough. Force to NONE mode. */
if (common->wcmGesureMode == GESTURE_LAG_MODE)
common->wcmGestureMode = GESTURE_NONE_MODE;
}
}
-------------------------------------------------------------------------
but again, this will add an extra comparison to events processing code.
>
> > /* When 2 fingers are in proximity, it must always be in one of
> > * the valid 2 fingers modes: LAG, SCROLL, or ZOOM.
> > * LAG mode is used while deciding between SCROLL and ZOOM and
> > @@ -306,6 +329,17 @@ void wcmGestureFilter(WacomDevicePtr priv, int channel)
> > goto ret;
> > }
> >
> > + if (common->wcmGestureMode == GESTURE_PREDRAG_MODE)
> > + {
> > + if (ds[0].proximity && !ds[1].proximity)
> > + {
> > + /* left button down */
> > + wcmSendButtonClick(priv, 1, 1);
> > + common->wcmGestureMode = GESTURE_DRAG_MODE;
> > + goto ret;
> > + }
> > + }
> > +
>
> There is already a "if (ds[0].proximity && !ds[1].proximity)" right
> above this. This logic should be moved inside that if().
There is a "if (ds[0].proximity && !dsLast[0].proximity)" right above,
which obviously different from what I needed. I've checked the code once
again and didn't find such "if()" in this function.
>
> > if (!ds[0].proximity && !ds[1].proximity)
> > {
> > /* first finger was out-prox when GestureMode was still on */
> > diff --git a/src/wcmTouchFilter.h b/src/wcmTouchFilter.h
> > new file mode 100644
> > index 0000000..453ef41
> > --- /dev/null
> > +++ b/src/wcmTouchFilter.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Copyright 2009 - 2010 by Ping Cheng, Wacom. <pi...@wacom.com>
> > + * Copyright 2011 by Alexey Osipov. <si...@lerlan.ru>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
> > USA.
> > + */
> > +
> > +#ifndef __XF86_WCMTOUCHFILTER_H
> > +#define __XF86_WCMTOUCHFILTER_H
> > +
> > +#include "xf86Wacom.h"
> > +
> > +/****************************************************************************/
> > +
> > +#define GESTURE_NONE_MODE 0
> > +#define GESTURE_TAP_MODE 1
> > +#define GESTURE_SCROLL_MODE 2
> > +#define GESTURE_ZOOM_MODE 4
> > +#define GESTURE_LAG_MODE 8
> > +#define GESTURE_PREDRAG_MODE 16
> > +#define GESTURE_DRAG_MODE 32
> > +
> > +void wcmGestureFilter(WacomDevicePtr priv, int channel);
>
> Generally, this type stuff should be broken into a cleanup patch
> (moving existing external defines to a better header file) and a new
> feature patch. That way in case gesture patch needs to reverted for
> some reason, we do not lose the header cleanup with it.
Agreed.
> Overall, pretty trivial comments. Thanks for patch!
>
> Chris
>
> > +
> > +/****************************************************************************/
> > +#endif /* __XF86_WCMTOUCHFILTER_H */
> > diff --git a/src/xf86Wacom.h b/src/xf86Wacom.h
> > index e58e0b0..fc1b4f4 100644
> > --- a/src/xf86Wacom.h
> > +++ b/src/xf86Wacom.h
> > @@ -152,7 +152,6 @@ extern int wcmDevSwitchMode(ClientPtr client,
> > DeviceIntPtr dev, int mode);
> > /* run-time modifications */
> > extern void wcmChangeScreen(InputInfoPtr pInfo, int value);
> > extern int wcmTilt2R(int x, int y, double offset);
> > -extern void wcmGestureFilter(WacomDevicePtr priv, int channel);
> > extern void wcmEmitKeycode(DeviceIntPtr keydev, int keycode, int state);
> > extern void wcmSoftOutEvent(InputInfoPtr pInfo);
> >
> > --
> > 1.7.0.4
> >
> >
> >
> >
> > ------------------------------------------------------------------------------
> > Write once. Port to many.
> > Get the SDK and tools to simplify cross-platform app development. Create
> > new or port existing apps to sell to consumers worldwide. Explore the
> > Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
> > http://p.sf.net/sfu/intel-appdev
> > _______________________________________________
> > Linuxwacom-devel mailing list
> > Linuxwacom-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
------------------------------------------------------------------------------
Ridiculously easy VDI. With Citrix VDI-in-a-Box, you don't need a complex
infrastructure or vast IT resources to deliver seamless, secure access to
virtual desktops. With this all-in-one solution, easily deploy virtual
desktops for less than the cost of PCs and save 60% on VDI infrastructure
costs. Try it free! http://p.sf.net/sfu/Citrix-VDIinabox
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
