On Sat, Dec 31, 2011 at 2:16 AM, Alexey Osipov <si...@lerlan.ru> wrote:
> В Птн, 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.
I'm referring to comment Peter made when he first submitting the patch
for wcmTouchFilter.c to xf86-input-wacom. Its not real important but
I just wanted to qualify changes to this file aren't as strict in my
opinion.
>
>> 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?
No, function calls are good in driver to improve readability. We do
this quite often. In this case, there is 99% chance that compiler will
inline the function anyways.
>
>> >
>> > /* 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).
>
I see what you were trying to avoid now.
> 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.
OK. let me describe basic issue I'd like you to solve and then you can
decide best way (there are multiple ways).
RIght now the logic towards top of function is grouped into "What do I
need to consider for each combination of finger touches to clean up
gesture modes?" and towards the bottom they are grouped into "What do
I need to do while in a gesture mode?"
You've added items that reads like "What do I need to do for
DRAG/PREDRAG gesture mode?" and so second class of question but you've
added them at non-standard location.
Can you either:
* Convert if()'s so that outer most if()'s are always finger count
checks (may need to break into multiple if()'s because of that) and
then move to an else {} block that makes most sense along with other
finger combo checks.
* Move your if()'s towards bottom of function and group with other
wcmGestureMode if()'s. You'll need to update finger count check
if()'s as needed like you mention above.
>
>>
>> > /* 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.
Oh yes, my mistake. Same thing here. Please create a "if
(ds[0].proximity && !ds[1].proximity)" block or move this if() lower
down (may need to add that block anyways to be able to move down).
Chris
>
>>
>> > 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
