Peter Hutterer wrote:
> thanks, that woke me up. I was struggling this morning :)
always a pleasure.
> I don't think this is necessary. We're nitpicking on the wording here and
> given the number of languages out there there's always going to be some
> ambiguity. with this patch you're breaking user configurations for very
> little benefit - especially since the same could be achieved by having
> more verbose documentation.
Yes and - eh. I've got the gut feeling the term 'constant deceleration'
is actually incorrect. But I'll postpone until I'm sure about that.
>> Next, I added the possibility to change pointer feedback controls
>> through options. The InputClass stuff makes it more sensible to do this.
>> TBH, I never figured why it was limited to command line options.
>
> there's protocol requests to adjust this at runtime and those are the only
> pointer-acceleration related ones supported in GUI configuration tools
> today. so arguably it's not essential since we've gone for years without it.
> also, aren't you trying to get people off the old scheme? not having config
> options is one way to do it :)
Yes, but all the accel stuff is runtime and config and per-device,
except the ptr feedback settings. So this a) fills a gap and b) since
the InputClass stuff came live, it actually makes some sense.
I'd prefer my devices to work in the login manager just as in the
session, except if I explicitly change my session. Distros could
sensibly ship pre-tuned accel or no-accel fragments for greater DE
independence.
Runtime config is nice, but being able to leverage InputClass,
you're avoiding the ID/Name matching problems of today's xinput.
>
> Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>
Thanks!
>> +is accelerated with respect to physical device motion. It also provides an
>> +option to scale down devices, which supersedes workarounds based on tweaking
>> +the acceleration controls. Most of these can be adjusted at runtime using
>> the
>> +pointer feedback and input property mechanisms (see the xinput(1) man page).
>> +Note that desktop environments typically do this for at least the first
>> three
>> +options (which correspond to pointer feedback settings). Only the most
>> +important acceleration options are discussed here.
>
> I think the part form "It also" to "feeback settings)" can be cut out. It's
> quite verbose with little information that matters in a man page. Personal
> opinion here. the xinput reference should come after the options, IMO.
I wonder if the scaling workaround was officially blessed. If yes, I'd
rather keep that stuff.
>> +.BI "Option \*qAccelerationDenominator\*q \*q" integer \*q
>> +Set the denominator of the acceleration factor.
>
> what do the numerator and denominator mean though? Having the formula here
> would be nice.
I'll try to be more helpful here.
>> +.TP 7
>> +.BI "Option \*qAccelerationThreshold\*q \*q" integer \*q
>> +Set the threshold, i.e. the velocity (device units/t) required for
>> acceleration
>> +to kick in.
>
> What's the unit of "t"?
> How about something like this:
> "Set the threshold in device units/second at which acceleration takes effect.
> Device movements below the threshold are unaccelerated."
Sounds fine, except that t=10ms by default, and device units has
deceleration applied. Ah, and not every profile honors threshold as
described :) Thus I aimed for simplicity.
>> +.B "predictable default algorithm (behaving intuitively predictable)"
>> +.B "lightweight old acceleration code"
>> +.B "none no acceleration or deceleration"
>> +
>
> biased, very biased :)
I did my best ;)
> IIRC, the old accel code is specified in the X Protocol, at least in parts.
> So rather than an "old severely constrained" algorithm point to the protocol
> specification and call your new one flexible instead of the old one
> constrained.
>
> also, unless you've got a study to show it, I'm somewhat uncomfortable with
> "intuitively predictable".
If X.Org or redhat would be sponsoring it - no problem ;)
Anyway, I'll try to be more balanced. I admit "intuitively predictable"
is hard to show, but "more predictable" is surely achieved.
>> - ErrorF("-t # mouse threshold (pixels)\n");
>> + ErrorF("-t # default mouse threshold (pixels/t)\n");
>
> separate patch for these two hunks. also - the protocol states that a value
> of -1 restores the default (ChangePointerControl, p72). does this mean it
> restores the default given here or some built-in default? if the latter,
> this patch is obsolete since -a and -t only set the startup values.
The former, actually. The defaults then propagate to new devices. Which
is especially nice since you can set invalid values here.
Anything uncommented is agreed to. I'll be sending an new version the
following days. Thanks for the review!
Cheers,
Simon
_______________________________________________
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
