On Sun, Dec 18, 2011 at 10:06 PM, Peter Hutterer
<peter.hutte...@who-t.net> wrote:
> On Tue, Dec 13, 2011 at 01:59:02PM -0800, Jason Gerecke wrote:
>> Replaces sendWheelStripEvents and getWheelButton with several
>> small functions to reduce unnecessary code duplication.
>>
>> As a side-effect, it is now possible for the driver to handle
>> simultaneous scrolls on multiple axes. Previously, getWheelButton
>> would go through all the axes but was limited to returning a
>> single button/action.
>>
>> Signed-off-by: Jason Gerecke <killert...@gmail.com>
>
> thanks! this patch makes it messy, but at least removes the copy/paste.
It'd be nice if there were a 'tunable' diff that let you adjust how it
matches subsequences... Patience diff is usually pretty good, but even
it coughed up a mess for this particular patch. Its not that bad of a
patch when done by hand, but the algorithms get caught up matching the
wrong bits of "common" text. :(
> aside from the comments below, please reshuffle this one so it applies
> _before_ the other one in the series. short story: this way we can bisect
> down to check if this patch broke anything on its own before the new feature
> was added.
>
That's actually the reason I put it as the final patch instead of
somewhere in the middle. Though, I suppose I can put it at the
beginning of sequence as well.
> coincidentally, writing tests for these so they keep exhibiting the same
> behaviour would be great...
>
Good call -- I always seem to forget that bit when adding new
functions. I'll whip some up during the rebase.
>> ---
>> src/wcmCommon.c | 206
>> ++++++++++++++++++++++++++++++++-----------------------
>> 1 files changed, 121 insertions(+), 85 deletions(-)
>>
>> diff --git a/src/wcmCommon.c b/src/wcmCommon.c
>> index 0e719cf..170d1d0 100644
>> --- a/src/wcmCommon.c
>> +++ b/src/wcmCommon.c
>> @@ -317,98 +317,106 @@ static void sendAButton(InputInfoPtr pInfo, int
>> button, int mask,
>> first_val, num_val, valuators);
>> }
>>
>> -/*****************************************************************************
>> - * getWheelButton --
>> - * Get the wheel button to be sent for the current device state.
>> -
>> ****************************************************************************/
>> -
>> -static int getWheelButton(InputInfoPtr pInfo, const WacomDeviceState* ds,
>> - unsigned int **fakeKey)
>> +/**
>> + * Get the distance an axis was scrolled. This function is aware
>> + * of the different ways different scrolling axes work and strives
>> + * to produce a common representation of relative change.
>> + *
>> + * @param current Current value of the axis
>> + * @param old Previous value of the axis
>> + * @param wrap Value after which wraparound occurs (0 if axis does not
>> wrap)
>> + * @param invert Scroll axis is inverted
>> + * @param bitwise Scroll axis values change in a bitwise manner
>> + * @return Relative change in axis value
>> + */
>> +static int getScrollDelta(int current, int old, int wrap, Bool invert, Bool
>> bitwise)
>
> I'd really like either a usage of flags or enums here so that the code is
> easier to read.
> delta = getScrollDelta(ds->stripx, priv->oldStripX, 0, TRUE, TRUE);
> is not at all obvious, getScrollDelta(..., INVERT|BITWISE) would be.
>
Flags work for me.
>> {
>> - WacomDevicePtr priv = (WacomDevicePtr) pInfo->private;
>> - int fakeButton = 0, value = 0;
>> + int delta;
>>
>> - /* emulate events for relative wheel */
>> - if ( ds->relwheel )
>> + if (bitwise)
>> {
>> - value = ds->relwheel;
>> - fakeButton = (value > 0) ? priv->relup : priv->reldn;
>> - *fakeKey = (value > 0) ? priv->wheel_keys[0+1] :
>> priv->wheel_keys[1+1];
>> + current = (int)log2(current);
>> + old = (int)log2(old);
>> }
>>
>> - /* emulate events for absolute wheel when it is a touch ring (on pad)
>> */
>> - if ( (ds->abswheel != priv->oldWheel) && IsPad(priv) &&
>> - (priv->oldProximity == ds->proximity))
>> - {
>> - int wrap_delta;
>> - value = priv->oldWheel - ds->abswheel;
>> -
>> - /* Wraparound detection. If the distance oldvalue..value is
>> - * larger than the oldvalue..value considering the
>> - * wraparound, assume wraparound and readjust */
>> - if (value < 0)
>> - wrap_delta = ((MAX_PAD_RING + 1) + priv->oldWheel) -
>> ds->abswheel;
>> - else
>> - wrap_delta = priv->oldWheel - ((MAX_PAD_RING + 1) +
>> ds->abswheel);
>> + delta = current - old;
>>
>> - DBG(12, priv, "wrap detection for %d (old %d): %d (wrap %d)\n",
>> - ds->abswheel, priv->oldWheel, value, wrap_delta);
>> + if (invert)
>> + delta = -delta;
>>
>> - if (abs(wrap_delta) < abs(value))
>> - value = wrap_delta;
>> -
>> - fakeButton = (value > 0) ? priv->wheelup : priv->wheeldn;
>> - *fakeKey = (value > 0) ? priv->wheel_keys[2+1] :
>> priv->wheel_keys[3+1];
>> - }
>> -
>> - /* emulate events for 2nd absolute wheel when it is a touch ring (on
>> pad) */
>> - if ( (ds->abswheel2 != priv->oldWheel2) && IsPad(priv) &&
>> - (priv->oldProximity == ds->proximity))
>> + if (wrap != 0)
>> {
>> + /* Wraparound detection. If the distance old..current
>> + * is larger than the old..current considering the
>> + * wraparound, assume wraparound and readjust */
>> int wrap_delta;
>> - value = priv->oldWheel2 - ds->abswheel2;
>>
>> - /* Wraparound detection. If the distance oldvalue..value is
>> - * larger than the oldvalue..value considering the
>> - * wraparound, assume wraparound and readjust */
>> - if (value < 0)
>> - wrap_delta = ((MAX_PAD_RING + 1) + priv->oldWheel2) -
>> ds->abswheel2;
>> + if (delta < 0)
>> + wrap_delta = (wrap + 1) + delta;
>> else
>> - wrap_delta = priv->oldWheel2 - ((MAX_PAD_RING + 1) +
>> ds->abswheel2);
>> + wrap_delta = -((wrap + 1) - delta);
>>
>> - DBG(12, priv, "wrap detection for %d (old %d): %d (wrap %d)\n",
>> - ds->abswheel2, priv->oldWheel2, value, wrap_delta);
>> + if (abs(wrap_delta) < abs(delta))
>> + delta = wrap_delta;
>> + }
>>
>> - if (abs(wrap_delta) < abs(value))
>> - value = wrap_delta;
>> + return delta;
>> +}
>>
>> - fakeButton = (value > 0) ? priv->wheel2up : priv->wheel2dn;
>> - *fakeKey = (value > 0) ? priv->wheel_keys[4+1] :
>> priv->wheel_keys[5+1];
>> - }
>> +/**
>> + * Get the scroll button/action to send given the delta of
>> + * the scrolling axis and the possible events that can be
>> + * sent.
>> + *
>> + * @param delta Amount of change in the scrolling axis
>> + * @param button_up Button event to send on scroll up
>> + * @param button_dn Button event to send on scroll down
>> + * @param action_up Action to send on scroll up
>> + * @param action_dn Action to send on scroll down
>> + * @param[out] action Action that should be performed
>> + * @return Button that should be pressed
>> + */
>> +static int getWheelButton(int delta, int button_up, int button_dn,
>> + unsigned int *action_up, unsigned int *action_dn,
>> + unsigned int **action)
>> +{
>> + int button = 0;
>> + *action = NULL;
>>
>> - /* emulate events for left strip */
>> - if ( ds->stripx != priv->oldStripX )
>> + if (delta)
>> {
>> - value = ds->stripx - priv->oldStripX;
>> -
>> - fakeButton = (value < 0) ? priv->striplup : priv->stripldn;
>> - *fakeKey = (value < 0) ? priv->strip_keys[0+1] :
>> priv->strip_keys[1+1];
>> + button = delta > 0 ? button_up : button_dn;
>> + *action = delta > 0 ? action_up : action_dn;
>> }
>>
>> - /* emulate events for right strip */
>> - if ( ds->stripy != priv->oldStripY )
>> - {
>> - value = ds->stripy - priv->oldStripY;
>> + return button;
>> +}
>>
>> - fakeButton = (value < 0) ? priv->striprup : priv->striprdn;
>> - *fakeKey = (value < 0) ? priv->strip_keys[2+1] :
>> priv->strip_keys[3+1];
>> - }
>> +/**
>> + * Send button or actions for a scrolling axis.
>> + *
>> + * @param button X button number to send if no action is defined
>> + * @param action Action to send
>> + * @param pInfo
>> + * @param first_val
>> + * @param num_vals
>> + * @param valuators
>> + */
>> +static void sendWheelStripEvent(int button, unsigned int *action,
>> InputInfoPtr pInfo,
>> + int first_val, int num_vals, int
>> *valuators)
>> +{
>> + WacomDevicePtr priv = (WacomDevicePtr) pInfo->private;
>>
>> - DBG(10, priv, "send fakeButton %x with value = %d \n",
>> - fakeButton, value);
>> + unsigned int button_action[1] = {button | AC_BUTTON | AC_KEYBTNPRESS};
>> + if (!action || !(*action)) {
>> + DBG(10, priv, "No wheel/strip action set; sending button %d
>> (action %d).\n", button, button_action[0]);
>> + action = &button_action[0];
>> + }
>>
>> - return fakeButton;
>> + sendAction(pInfo, 1, action, ARRAY_SIZE(action), first_val, num_vals,
>> valuators);
>> + sendAction(pInfo, 0, action, ARRAY_SIZE(action), first_val, num_vals,
>> valuators);
>> }
>> +
>> /*****************************************************************************
>> * sendWheelStripEvents --
>> * Send events defined for relative/absolute wheels or strips
>> @@ -418,31 +426,59 @@ static void sendWheelStripEvents(InputInfoPtr pInfo,
>> const WacomDeviceState* ds,
>> int first_val, int num_vals, int *valuators)
>> {
>> WacomDevicePtr priv = (WacomDevicePtr) pInfo->private;
>> - int fakeButton = 0;
>> + int fakeButton = 0, delta = 0;
>> unsigned int *fakeKey = NULL;
>>
>> DBG(10, priv, "\n");
>>
>> - fakeButton = getWheelButton(pInfo, ds, &fakeKey);
>> + /* emulate events for left strip */
>> + delta = getScrollDelta(ds->stripx, priv->oldStripX, 0, TRUE, TRUE);
>> + if (delta && IsPad(priv) && priv->oldProximity == ds->proximity)
>> + {
>> + DBG(10, priv, "Left touch strip scroll delta = %d\n", delta);
>> + fakeButton = getWheelButton(delta, priv->striplup,
>> priv->stripldn,
>> + priv->strip_keys[0+1],
>> priv->strip_keys[1+1], &fakeKey);
>
> urgh, named defines for the indices please. which would prevent...
>
>> + sendWheelStripEvent(fakeButton, fakeKey, pInfo, first_val,
>> num_vals, valuators);
>> + }
>>
>> - if (!fakeButton && (!fakeKey || !(*fakeKey)))
>> - return;
>> + /* emulate events for right strip */
>> + delta = getScrollDelta(ds->stripy, priv->oldStripY, 0, TRUE, TRUE);
>> + if (delta && IsPad(priv) && priv->oldProximity == ds->proximity)
>> + {
>> + DBG(10, priv, "Right touch strip scroll delta = %d\n", delta);
>> + fakeButton = getWheelButton(delta, priv->striprup,
>> priv->striprdn,
>> + priv->strip_keys[2+1],
>> priv->strip_keys[3+1], &fakeKey);
>> + sendWheelStripEvent(fakeButton, fakeKey, pInfo, first_val,
>> num_vals, valuators);
>> + }
>>
>> - if (!fakeKey || !(*fakeKey))
>> + /* emulate events for relative wheel */
>> + delta = getScrollDelta(ds->relwheel, 0, 0, FALSE, FALSE);
>> + if (delta && IsCursor(priv) && priv->oldProximity == ds->proximity)
>> {
>> - /* send both button on/off in the same event for pad */
>> - xf86PostButtonEventP(pInfo->dev, is_absolute(pInfo),
>> fakeButton & AC_CODE,
>> - 1, first_val, num_vals, VCOPY(valuators,
>> num_vals));
>> + DBG(10, priv, "Relative wheel scroll delta = %d\n", delta);
>> + fakeButton = getWheelButton(delta, priv->relup, priv->reldn,
>> + priv->wheel_keys[0+1],
>> priv->wheel_keys[1+1], &fakeKey);
>> + sendWheelStripEvent(fakeButton, fakeKey, pInfo, first_val,
>> num_vals, valuators);
>> + }
>
> confusion here, the relwheel has the same index as the lstrip? and the lring
> the same as the rstrip?
>
> the diff is virtually so this may be correct, but having named defines with
> comments would help greatly here.
>
If you're confused by two different arrays (strip_keys and wheel_keys)
having different data at the same indicies, I think that's a sign of a
more fundamental problem. Personally, I never understood the need for
a distinction between strip_keys and wheel_keys. A single
"scroll_keys" (or, as I'd rather name it, "scroll_actions") array that
houses all the scroll events would make things a *lot* less confusing.
Throw named indicies on top of that, and things get better still.
If you don't mind, I'd like to move this work to a separate patch
which does unification and named indicies.
> Cheers,
> Peter
>
>>
>> - xf86PostButtonEventP(pInfo->dev, is_absolute(pInfo),
>> fakeButton & AC_CODE,
>> - 0, first_val, num_vals, VCOPY(valuators,
>> num_vals));
>> + /* emulate events for left touch ring */
>> + delta = getScrollDelta(ds->abswheel, priv->oldWheel, MAX_PAD_RING,
>> TRUE, FALSE);
>> + if (delta && IsPad(priv) && priv->oldProximity == ds->proximity)
>> + {
>> + DBG(10, priv, "Left touch wheel scroll delta = %d\n", delta);
>> + fakeButton = getWheelButton(delta, priv->wheelup,
>> priv->wheeldn,
>> + priv->wheel_keys[2+1],
>> priv->wheel_keys[3+1], &fakeKey);
>
>> + sendWheelStripEvent(fakeButton, fakeKey, pInfo, first_val,
>> num_vals, valuators);
>> }
>> - else
>> +
>> + /* emulate events for right touch ring */
>> + delta = getScrollDelta(ds->abswheel2, priv->oldWheel2, MAX_PAD_RING,
>> TRUE, FALSE);
>> + if (delta && IsPad(priv) && priv->oldProximity == ds->proximity)
>> {
>> - sendAction(pInfo, 1, fakeKey, ARRAY_SIZE(priv->wheel_keys[0]),
>> - first_val, num_vals, valuators);
>> - sendAction(pInfo, 0, fakeKey, ARRAY_SIZE(priv->wheel_keys[0]),
>> - first_val, num_vals, valuators);
>> + DBG(10, priv, "Right touch wheel scroll delta = %d\n", delta);
>> + fakeButton = getWheelButton(delta, priv->wheel2up,
>> priv->wheel2dn,
>> + priv->wheel_keys[4+1],
>> priv->wheel_keys[5+1], &fakeKey);
>> + sendWheelStripEvent(fakeButton, fakeKey, pInfo, first_val,
>> num_vals, valuators);
>> }
>> }
>>
>> --
>> 1.7.7.3
Jason
---
Day xee-nee-svsh duu-'ushtlh-ts'it;
nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it.
Huu-chan xuu naa~-gha.
------------------------------------------------------------------------------
Learn Windows Azure Live! Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for
developers. It will provide a great way to learn Windows Azure and what it
provides. You can attend the event by watching it streamed LIVE online.
Learn more at http://p.sf.net/sfu/ms-windowsazure
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
