On Sat, Aug 21, 2010 at 07:31:36PM +0200, Lukas Zilka wrote: > I'm sending hopefully a better version of my wheel emulation patch. I > merged the configuration properties into two multivalued, as Peter > advised. Otherwise the functionality remains the same. > > If somebody actually tries this, please give me some feedback and ideas > for improvement ;)
thank you for the patch. I admit I haven't tried it yet, but my comments are in-line. Principle is sound, but there are still some rough bits around the edges. > Signed-off-by: Lukas Zilka <[email protected]> > --- > include/wacom-properties.h | 6 ++ > src/wcmCommon.c | 130 > ++++++++++++++++++++++++++++++++++++++------ > src/wcmConfig.c | 6 ++ > src/wcmXCommand.c | 31 +++++++++++ > src/xf86WacomDefs.h | 10 ++++ > 5 files changed, 167 insertions(+), 16 deletions(-) > > diff --git a/include/wacom-properties.h b/include/wacom-properties.h > index f6633a1..8785e04 100644 > --- a/include/wacom-properties.h > +++ b/include/wacom-properties.h > @@ -96,4 +96,10 @@ > */ > #define WACOM_PROP_DEBUGLEVELS "Wacom Debug Levels" > > +/* 32bit, 32bit - movement tolerance (-> start scrolling after exceeding > this distance), delay in miliseconds between 2 consecutive scrolls sent */ properties can only be of a single type/format, so just "32 bit, two values - ..." is enough > +#define WACOM_PROP_WHEEL_SCROLL_PARAMETERS "Wacom Wheel Emulation Scroll > Parameters" I'd name it #define WACOM_PROP_WHEEL_EMULATION_PARAMETERS "Wacom Wheel Emulation Parameters" to make sure in the #define that it's emulation we're talking about here. > + > +/* bool, 8 bit, bool - on/off, button number, scroll only when floating > above tablet? */ > +#define WACOM_PROP_WHEEL_EMULATION "Wacom Wheel Emulation" > + > #endif > diff --git a/src/wcmCommon.c b/src/wcmCommon.c > index 7e50878..e77bc8e 100644 > --- a/src/wcmCommon.c > +++ b/src/wcmCommon.c > @@ -810,7 +810,7 @@ void wcmSendEvents(InputInfoPtr pInfo, const > WacomDeviceState* ds) > */ > if (!is_absolute(pInfo)) > x *= priv->factorY / priv->factorX; > - else > + else > { > /* Padding virtual values */ > wcmVirtualTabletPadding(pInfo); please skip this hunk, whitespace only. > @@ -822,25 +822,39 @@ void wcmSendEvents(InputInfoPtr pInfo, const > WacomDeviceState* ds) > if ((pInfo->dev->proximity && !priv->oldProximity)) > xf86PostProximityEvent(pInfo->dev, 1, 0, naxes, > x, y, z, v3, v4, v5); > > - /* Move the cursor to where it should be before sending > button events */ > - if(!(priv->flags & BUTTONS_ONLY_FLAG)) > + /* do the usual only if we are not scrolling (scrolling > conditions aren't met) */ > + if( !wcmWheelEmuScrollButtonPressed( ds, priv ) ) no spaces between () and the parameters, (ds, priv) in this case. > { > - xf86PostMotionEvent(pInfo->dev, > is_absolute(pInfo), > - 0, naxes, x, y, z, v3, v4, v5); > - /* For relative events, reset the axes as > - * we've already moved the device by the > - * relative amount. Otherwise, a button > - * event in sendCommonEvents will move the > - * axes again. > - */ > - if (!is_absolute(pInfo)) > + /* Move the cursor to where it should be before > sending button events */ > + if(!(priv->flags & BUTTONS_ONLY_FLAG)) > { > - x = y = z = 0; > - v3 = v4 = v5 = 0; > + xf86PostMotionEvent(pInfo->dev, > is_absolute(pInfo), > + 0, naxes, x, y, z, v3, > v4, v5); > + /* For relative events, reset the axes > as > + * we've already moved the device by the > + * relative amount. Otherwise, a button > + * event in sendCommonEvents will move > the > + * axes again. > + */ > + if (!is_absolute(pInfo)) > + { > + x = y = z = 0; > + v3 = v4 = v5 = 0; > + } > + } > + sendCommonEvents(pInfo, ds, x, y, z, v3, v4, > v5); > + /* the srolling button is not pressed, so make > sure that scrolling is stopped */ > + if( wcmWheelEmuIsScrolling( priv ) ) { > + wcmWheelEmuScrollOff( priv ); > + } > + } else { > + /* the scrolling button is pressed, so start > scrolling if it hasn't already been started */ > + if( !wcmWheelEmuIsScrolling( priv ) ) { > + wcmWheelEmuScrollOn( priv, x, y ); > } > } > - > - sendCommonEvents(pInfo, ds, x, y, z, v3, v4, v5); > + /* do the scrolling stuff */ > + wcmWheelEmuScroll( priv ); > } > else /* not in proximity */ > { > @@ -1951,4 +1965,88 @@ WacomCommonPtr wcmRefCommon(WacomCommonPtr common) > return common; > } > > +int wcmWheelEmuIsScrolling( WacomDevicePtr priv ) { opening { on new line. > + return priv->scrolling; > +} > + > +void wcmWheelEmuScrollOff( WacomDevicePtr priv ) { > + priv->scrolling = 0; > +} indentation, one tab plesae in both cases. > + > +void wcmWheelEmuScrollOn( WacomDevicePtr priv, int x, int y ) { > + priv->scrollX = x; > + priv->scrollY = y; > + priv->scrolling = 1; > +} > + > +int wcmWheelEmuScrollButtonPressed( const WacomDeviceState* ds, > WacomDevicePtr priv ) { > + /* if the scroll-only-when-floating-above-the-tablet option has been > set in parameters, > + * we want to scroll only when the button that is activated when the > stylus touches the > + * tablet is not activated. in the other case > (scroll-only-when-floating-above-the-tablet > + * is not set), we don't care about this button, and all we care about > is the scroll button. */ I do have to wonder - is there anyone that wants to scroll when the button is down but the stylus isn't touching the tablet? that doesn't strike me as a particularly useful interface - too error prone. I think we could just skip this configuration option and assume it true. > + > + /* if the wheel emulation is disabled, return false */ just skip this comment, it's rather obvious from the next line. > + if( ! priv->common->wcmWheelEmuEnabled ) { > + return 0; > + } no {} for single-line blocks. > + > + /* check if the scroll button is pressed */ > + if( ( ds-> buttons & priv->common->wcmScrollButton ) == > priv->common->wcmScrollButton ) { use a WcmCommonPtr variable to avoid the priv->common everywhere. > + /* if it is pressed, and the scroll-only-when-floating property > is not set, return true */ > + if( !priv->common->wcmScrollOnlyFloating ) { > + return 1; > + } else { > + /* else, check if the stylus doesn't touch the tablet by > examining the pressed buttons > + * for the Button1 one. it can't be on. */ have you given any thought towards how wheel emulation would work with finger scrolling on capable tablets? the ds->buttons & 1 check wouldn't work in this case, but I can't really think of anything more useful right now either. > + if( ( ds-> buttons & 1 ) == 0 ) { > + return 1; > + } else { > + return 0; > + } > + } these nested ifs can be compressed into a single one with a return 1, and the default exit path of return 0. > + > + > + } else { > + /* if not, return false */ superfluous comment > + return 0; > + } > + > +} > + > +void wcmWheelEmuScroll( WacomDevicePtr priv, x, y, z, v3, v4, v5 ) { > + /* start the scrolling procedure only if we are supposed to scroll */ > + if( priv->common->wcmWheelEmuEnabled && wcmWheelEmuIsScrolling( priv ) > ) { nicer solved with the negation of this condition and an early return, so the rest of the function doesn't have to be nested. > + /* for scrolling direction */ > + int scroll_where; "scroll_direction" instead? also, since we're inside wcmWheelEmuScroll here anyway, just direction, threshold, etc. would be enough. > + /* some movement tolerance so as not to yet scroll if we > haven't moved enough */ > + int scroll_thresh = priv->common->wcmScrollThresh; > + /* set scrolling speed in steps of threshold */ > + int scroll_speed = abs(y - priv->scrollY)/scroll_thresh; > + /* timeout = delay between 2 consecutive scrolls that are > sent.. so that we don't scroll too fast */ > + int scroll_timeout = > (1.0*priv->common->wcmScrollTimeout)/scroll_speed; all three have standard meanings, with the comments interleaved it's a lot harder to read. I'd say just skip the comments, this isn't tutorial code where every line needs to be explained. > + > + /* determine the scrolling direction (mousebutton 4 scroll up, > 5 scroll down, 0 don't scroll as the mouse > + * hasn't exceeded the threshold value) */ > + if( y < priv->scrollY && abs(y - priv->scrollY) > scroll_thresh > ) > + scroll_where = 4; > + else { > + if( y > priv->scrollY && abs(y - priv->scrollY) > > scroll_thresh ) > + scroll_where = 5; > + else > + scroll_where = 0; > + } could be rearranged neater: if (abs ...) scroll_direction = (y >= priv->scrollY) ? 5 : 4 ; else scroll_direction = 0; horizontal scrolling seems to be missing here. > + > + /* if there is somewhere to scroll, and if the timeout from the > last executed scroll has passed, > + * scroll - send the X events */ > + if( scroll_where != 0 && priv->lastScrolled + scroll_timeout < > GetTimeInMillis() ) { > + xf86PostButtonEvent(pInfo->dev, is_absolute(pInfo), > + scroll_where,1,0,priv->naxes, > priv->scrollX,priv->scrollY,z,v3,v4,v5); > + xf86PostButtonEvent(pInfo->dev, is_absolute(pInfo), > + scroll_where,0,0,priv->naxes, > priv->scrollX,priv->scrollY,z,v3,v4,v5); > + > + priv->lastScrolled = GetTimeInMillis(); > + } > + } > +} I don't quite understand why the timeout is needed. If scrolling is too fast, just increase the threshold, no? btw, the traditional approach for scroll wheel emulation is to have a loop that counts down scroll_thresh from the accumulated value so that fast movements have an immediate effect of multiple scrolls. > + > /* vim: set noexpandtab shiftwidth=8: */ > diff --git a/src/wcmConfig.c b/src/wcmConfig.c > index 2fdacfa..208101a 100644 > --- a/src/wcmConfig.c > +++ b/src/wcmConfig.c > @@ -135,6 +135,12 @@ static int wcmAllocate(InputInfoPtr pInfo) > /* transmit position if increment is superior */ > common->wcmRawSample = DEFAULT_SAMPLES; > /* number of raw data to be used to for filtering */ > + > + common->wcmWheelEmuEnabled = 1; > + common->wcmScrollThresh = 100; > + common->wcmScrollTimeout = 100; > + common->wcmScrollButton = 2; > + common->wcmScrollOnlyFloating = 1; I don't think we should enable this by default, especially not with button 2 as activating button. this would be a rather severe behaviour change that usually results in my inbox filling up with complaints. Unlike the evdev driver we can't easily move it onto a different button, so we have to leave it disabled by default. > /* tool */ > priv->tool = tool; > diff --git a/src/wcmXCommand.c b/src/wcmXCommand.c > index ed14de5..dd45746 100644 > --- a/src/wcmXCommand.c > +++ b/src/wcmXCommand.c > @@ -110,6 +110,8 @@ Atom prop_btnactions; > #ifdef DEBUG > Atom prop_debuglevels; > #endif > +Atom prop_wheel_emu; > +Atom prop_wheel_emu_scroll_params; > > /* Special case: format -32 means type is XA_ATOM */ > static Atom InitWcmAtom(DeviceIntPtr dev, char *name, int format, int > nvalues, int *values) > @@ -255,6 +257,16 @@ void InitWcmDeviceProperties(InputInfoPtr pInfo) > values[1] = common->debugLevel; > prop_debuglevels = InitWcmAtom(pInfo->dev, WACOM_PROP_DEBUGLEVELS, 8, > 2, values); > #endif > + /* wheel emulation settings - on/off, button, scroll-only-when-floating > */ > + values[0] = common->wcmWheelEmuEnabled; > + values[1] = common->wcmScrollButton; > + values[2] = common->wcmScrollOnlyFloating; > + prop_wheel_emu = InitWcmAtom(pInfo->dev, WACOM_PROP_WHEEL_EMULATION, 8, > 3, values); > + > + /* scrolling parameters - movement tolerance, delay between 2 scrolls */ > + values[0] = common->wcmScrollThresh; > + values[1] = common->wcmScrollTimeout; > + prop_scroll_params = InitWcmAtom(pInfo->dev, > WACOM_PROP_SCROLL_PARAMETERS, 32, 1, values); this worries me a bit... did you compile this code? the property name is prop_wheel_emu_scroll_params above, but you're using prop_scroll_params here... > } > > /* Returns the offset of the property in the list given. If the property is > @@ -881,6 +893,25 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, > XIPropertyValuePtr prop, > if (prop->size != WCM_MAX_MOUSE_BUTTONS) > return BadMatch; > wcmSetPropertyButtonActions(dev, property, prop, checkonly); > + } else if (property == prop_wheel_emu) > + { > + CARD8 *values = (CARD8*)prop->data; > + > + if (prop->size != 3 || prop->format != 8) > + return BadValue; > + > + common->wcmWheelEmuEnabled = values[0]; > + common->wcmScrollButton = values[1]; > + common->wcmScrollOnlyFloating = values[2]; > + } else if (property == prop_scroll_params) > + { > + CARD32 *values = (CARD32*)prop->data; > + > + if (prop->size != 2 || prop->format != 32) > + return BadValue; > + > + common->wcmScrollThreshold = values[0]; > + common->wcmScrollTimeout = values[1]; > } else > wcmSetActionProperties(dev, property, prop, checkonly); > > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h > index dcd21fc..45b1766 100644 > --- a/src/xf86WacomDefs.h > +++ b/src/xf86WacomDefs.h > @@ -225,6 +225,10 @@ struct _WacomDeviceRec > int oldX; /* previous X position */ > int oldY; /* previous Y position */ > int oldZ; /* previous pressure */ > + int scrolling; /* are we scrolling now? */ > + int scrollX; /* the mouseX position when the scroll button > was pressed */ > + int scrollY; /* the mouseY position when the scroll button > was pressed */ > + int lastScrolled; /* the time in milliseconds, when the last > scroll event has been sent */ > int oldCapacity; /* previous capacity */ > int oldTiltX; /* previous tilt in x direction */ > int oldTiltY; /* previous tilt in y direction */ > @@ -445,6 +449,12 @@ struct _WacomCommonRec > void *private; /* backend-specific information */ > > WacomToolPtr wcmTool; /* List of unique tools */ > + > + int wcmWheelEmuEnabled; /* enable/disable the wheel emulation */ > + int wcmScrollThresh; /* the distance that the mouse must surpass in > order for us to start scrolling */ > + int wcmScrollTimeout; /* the delay between sending another scroll > event */ > + int wcmScrollButton; /* the button that activates scrolling */ > + int wcmScrollOnlyFloating; /* the > scroll-only-if-the-stylus-is-floating-above-the-tablet option */ > > /* DO NOT TOUCH THIS. use wcmRefCommon() instead */ > int refcnt; /* number of devices sharing this > struct */ > -- > 1.7.0.4 please add the xorg.conf options for this as well so these can be set at runtime. The other thing missing here is the man page update. other than that, thanks for the patch, it looks good and I can see that this feature may be useful (haven't tested it yet though). Please make sure you adhere to the style conventions used in the rest of the code. Cheers, Peter ------------------------------------------------------------------------------ This SF.net email is sponsored by Make an app they can't live without Enter the BlackBerry Developer Challenge http://p.sf.net/sfu/RIM-dev2dev _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
