pointer acceleration property rename and docs
Simon Thum
simon.thum at gmx.de
Wed Jan 6 11:27:34 PST 2010
Those patches should address the issues. The rename is skipped for now.
Peter Hutterer wrote:
> thanks, that woke me up. I was struggling this morning :)
>
> On Tue, Jan 05, 2010 at 09:13:47PM +0100, Simon Thum wrote:
>> another round of ptr accel patches. I didn't send to the dev list to
>> maybe gather more input on the rename.
>>
>> First, I renamed 'constant deceleration' to 'downscaling', and adjusted
>> the property and option names. I hope that it is a good name; I'm still
>> open for suggestions. Many suggested 'sensitivity' but I think that
>> isn't it.
>>
>> Only the option rename is backwards-compatible though, to avoid
>> redundant input properties. Peter, is that one OK with you?
>
> 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.
>
>> 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 :)
>
> see for more comments in-line though.
>
>> Third, some whitespace style fixes. Nothing functional.
>
> meh, as usual with whitespace patches. since you're the one working most of
> that code - if you're happy with that go for it.
>
>> Last but not least, the documentation should now reflect reality wrt
>> pointer acceleration. Affects the xorg.conf.man and --help output.
>>
>> It's perfectly possible the man changes are bogus, I just copied around
>> and I seem not to have broken things.
>
>> From da0b0bb6bda12eaa7f3cece3ab72056e73afa765 Mon Sep 17 00:00:00 2001
>> From: Simon Thum <simon.thum at gmx.de>
>> Date: Tue, 5 Jan 2010 14:46:35 +0100
>> Subject: [PATCH 2/4] xfree86: set accelaration controls from options
>>
>> Since config backends become better integrated, we may as well
>> cover all the acceleration-related settings. Previously, the only
>> way to set acceleration controls (aka pointer feedback) upfront
>> was command-line options to the Xorg binary.
>> ---
>> hw/xfree86/common/xf86Xinput.c | 33 +++++++++++++++++++++++++++++++--
>> 1 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
>> index 979b265..90ed1bc 100644
>> --- a/hw/xfree86/common/xf86Xinput.c
>> +++ b/hw/xfree86/common/xf86Xinput.c
>> @@ -199,12 +199,12 @@ ProcessVelocityConfiguration(DeviceIntPtr pDev, char* devname, pointer list,
>>
>> static void
>> ApplyAccelerationSettings(DeviceIntPtr dev){
>> - int scheme;
>> + int scheme, i;
>> DeviceVelocityPtr pVel;
>> LocalDevicePtr local = (LocalDevicePtr)dev->public.devicePrivate;
>> char* schemeStr;
>>
>> - if(dev->valuator){
>> + if(dev->valuator && dev->ptrfeed){
>
> this whitespace should be fixed as part of this patch.
>
>> schemeStr = xf86SetStrOption(local->options, "AccelerationScheme", "");
>>
>> scheme = dev->valuator->accelScheme.number;
>> @@ -247,6 +247,35 @@ ApplyAccelerationSettings(DeviceIntPtr dev){
>> pVel);
>> break;
>> }
>> +
>> + /*
>> + * process acceleration controls (or 'pointer feedback') as device
>> + * options, to allow people to set acceleration preferences more
>> + * flexibly than command-line trickery.
>> + */
>
> I don't think this comment is necessary since the stuff below is pretty
> standard code.
>
>> + i = xf86SetIntOption(local->options, "AccelerationNumerator",
>> + dev->ptrfeed->ctrl.num);
>> + if (i >= 0)
>> + dev->ptrfeed->ctrl.num = i;
>> +
>> + i = xf86SetIntOption(local->options, "AccelerationDenominator",
>> + dev->ptrfeed->ctrl.den);
>> + if (i > 0)
>> + dev->ptrfeed->ctrl.den = i;
>> +
>> + i = xf86SetIntOption(local->options, "AccelerationThreshold",
>> + dev->ptrfeed->ctrl.threshold);
>> + if (i >= 0)
>> + dev->ptrfeed->ctrl.threshold = i;
>> +
>> + /* mostly a no-op anyway */
>> + (*dev->ptrfeed->CtrlProc)(dev, &dev->ptrfeed->ctrl);
>> +
>> + xf86Msg(X_CONFIG, "%s: (accel) acceleration factor: %.3f\n",
>> + local->name, ((float)dev->ptrfeed->ctrl.num)/
>> + ((float)dev->ptrfeed->ctrl.den));
>> + xf86Msg(X_CONFIG, "%s: (accel) acceleration threshold: %i\n",
>> + local->name, dev->ptrfeed->ctrl.threshold);
>> }
>> }
>>
>> --
>> 1.6.4.4
>
> the man page additions need to be part of this patch.
>
>
>> From 3ef79eb47a2011cd3f64ad48a42cba6483ea367b Mon Sep 17 00:00:00 2001
>> From: Simon Thum <simon.thum at gmx.de>
>> Date: Tue, 5 Jan 2010 15:31:08 +0100
>> Subject: [PATCH 3/4] whitespace fixes for accel setup
>>
>> ---
>> hw/xfree86/common/xf86Xinput.c | 36 +++++++++++++++++-------------------
>> 1 files changed, 17 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
>> index 90ed1bc..e225294 100644
>> --- a/hw/xfree86/common/xf86Xinput.c
>> +++ b/hw/xfree86/common/xf86Xinput.c
>> @@ -122,7 +122,7 @@ ProcessVelocityConfiguration(DeviceIntPtr pDev, char* devname, pointer list,
>> /* common settings (available via device properties) */
>> tempf = xf86SetRealOption(list, "ConstantDeceleration", 1.0);
>> tempf = xf86SetRealOption(list, "Downscaling", tempf);
>> - if(tempf > 1.0){
>> + if (tempf > 1.0) {
>> xf86Msg(X_CONFIG, "%s: (accel) scaling down by %.1f\n",
>> devname, tempf);
>> prop = XIGetKnownProperty(ACCEL_PROP_DOWNSCALING);
>> @@ -131,7 +131,7 @@ ProcessVelocityConfiguration(DeviceIntPtr pDev, char* devname, pointer list,
>> }
>>
>> tempf = xf86SetRealOption(list, "AdaptiveDeceleration", 1.0);
>> - if(tempf > 1.0){
>> + if (tempf > 1.0) {
>> xf86Msg(X_CONFIG, "%s: (accel) adaptive deceleration by %.1f\n",
>> devname, tempf);
>> prop = XIGetKnownProperty(ACCEL_PROP_ADAPTIVE_DECELERATION);
>> @@ -145,8 +145,7 @@ ProcessVelocityConfiguration(DeviceIntPtr pDev, char* devname, pointer list,
>>
>> prop = XIGetKnownProperty(ACCEL_PROP_PROFILE_NUMBER);
>> if (XIChangeDeviceProperty(pDev, prop, XA_INTEGER, 32,
>> - PropModeReplace, 1, &tempi, FALSE) == Success)
>> - {
>> + PropModeReplace, 1, &tempi, FALSE) == Success) {
>> xf86Msg(X_CONFIG, "%s: (accel) acceleration profile %i\n", devname,
>> tempi);
>> } else {
>> @@ -157,20 +156,19 @@ ProcessVelocityConfiguration(DeviceIntPtr pDev, char* devname, pointer list,
>> /* set velocity scaling */
>> tempf = xf86SetRealOption(list, "ExpectedRate", 0);
>> prop = XIGetKnownProperty(ACCEL_PROP_VELOCITY_SCALING);
>> - if(tempf > 0){
>> + if (tempf > 0) {
>> tempf = 1000.0 / tempf;
>> XIChangeDeviceProperty(pDev, prop, float_prop, 32,
>> PropModeReplace, 1, &tempf, FALSE);
>> - }else{
>> + } else {
>> tempf = xf86SetRealOption(list, "VelocityScale", s->corr_mul);
>> XIChangeDeviceProperty(pDev, prop, float_prop, 32,
>> PropModeReplace, 1, &tempf, FALSE);
>> }
>>
>> tempi = xf86SetIntOption(list, "VelocityTrackerCount", -1);
>> - if(tempi > 1){
>> + if (tempi > 1)
>> InitTrackers(s, tempi);
>> - }
>>
>> s->initial_range = xf86SetIntOption(list, "VelocityInitialRange",
>> s->initial_range);
>> @@ -178,7 +176,7 @@ ProcessVelocityConfiguration(DeviceIntPtr pDev, char* devname, pointer list,
>> s->max_diff = xf86SetRealOption(list, "VelocityAbsDiff", s->max_diff);
>>
>> tempf = xf86SetRealOption(list, "VelocityRelDiff", -1);
>> - if(tempf >= 0){
>> + if (tempf >= 0) {
>> xf86Msg(X_CONFIG, "%s: (accel) max rel. velocity difference: %.1f%%\n",
>> devname, tempf*100.0);
>> s->max_rel_diff = tempf;
>> @@ -204,35 +202,35 @@ ApplyAccelerationSettings(DeviceIntPtr dev){
>> LocalDevicePtr local = (LocalDevicePtr)dev->public.devicePrivate;
>> char* schemeStr;
>>
>> - if(dev->valuator && dev->ptrfeed){
>> + if (dev->valuator && dev->ptrfeed) {
>> schemeStr = xf86SetStrOption(local->options, "AccelerationScheme", "");
>>
>> scheme = dev->valuator->accelScheme.number;
>>
>> - if(!xf86NameCmp(schemeStr, "predictable"))
>> + if (!xf86NameCmp(schemeStr, "predictable"))
>> scheme = PtrAccelPredictable;
>>
>> - if(!xf86NameCmp(schemeStr, "lightweight"))
>> + if (!xf86NameCmp(schemeStr, "lightweight"))
>> scheme = PtrAccelLightweight;
>>
>> - if(!xf86NameCmp(schemeStr, "none"))
>> + if (!xf86NameCmp(schemeStr, "none"))
>> scheme = PtrAccelNoOp;
>>
>> /* reinit scheme if needed */
>> - if(dev->valuator->accelScheme.number != scheme){
>> - if(dev->valuator->accelScheme.AccelCleanupProc){
>> + if (dev->valuator->accelScheme.number != scheme) {
>> + if (dev->valuator->accelScheme.AccelCleanupProc) {
>> dev->valuator->accelScheme.AccelCleanupProc(dev);
>> }
>>
>> - if(InitPointerAccelerationScheme(dev, scheme)){
>> + if (InitPointerAccelerationScheme(dev, scheme)) {
>> xf86Msg(X_CONFIG, "%s: (accel) selected scheme %s/%i\n",
>> local->name, schemeStr, scheme);
>> - }else{
>> + } else {
>> xf86Msg(X_CONFIG, "%s: (accel) could not init scheme %s\n",
>> local->name, schemeStr);
>> scheme = dev->valuator->accelScheme.number;
>> }
>> - }else{
>> + } else {
>> xf86Msg(X_CONFIG, "%s: (accel) keeping acceleration scheme %i\n",
>> local->name, scheme);
>> }
>> @@ -240,7 +238,7 @@ ApplyAccelerationSettings(DeviceIntPtr dev){
>> xfree(schemeStr);
>>
>> /* process special configuration */
>> - switch(scheme){
>> + switch (scheme) {
>> case PtrAccelPredictable:
>> pVel = GetDevicePredictableAccelData(dev);
>> ProcessVelocityConfiguration (dev, local->name, local->options,
>> --
>> 1.6.4.4
>
>
> Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
>
>
>> From 34b91933b977e73749ec934c78249cb93533966e Mon Sep 17 00:00:00 2001
>> From: Simon Thum <simon.thum at gmx.de>
>> Date: Tue, 5 Jan 2010 16:23:57 +0100
>> Subject: [PATCH 4/4] xfree86: add pointer acceleration info to xorg.conf.man
>>
>> Just try to describe what's important, still looking for a place
>> put the whole sermon.
>>
>> Also, be more precise about what -a and -t actually do
>> on --help output.
>> ---
>> hw/xfree86/doc/man/xorg.conf.man.pre | 62 +++++++++++++++++++++++++++++++++-
>> os/utils.c | 4 +-
>> 2 files changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/xfree86/doc/man/xorg.conf.man.pre b/hw/xfree86/doc/man/xorg.conf.man.pre
>> index 5b98bda..63526e0 100644
>> --- a/hw/xfree86/doc/man/xorg.conf.man.pre
>> +++ b/hw/xfree86/doc/man/xorg.conf.man.pre
>> @@ -914,7 +914,67 @@ X Input extension. This option controls the startup behavior only, a device
>> may be reattached or set floating at runtime.
>> .TP 7
>> .BI "Option \*qSendDragEvents\*q \*q" boolean \*q
>> -???
>> +Send core events while dragging. Default: Enabled.
>
> separate patch please.
>
>> +.PP
>> +.TP 7
>> +.B Pointer Acceleration
>
> judging by the rest of the InputDevice section, this shouldn't be bold, it's
> not a config option. just skip this line.
>
>> +For pointing devices, the following options control if and how the pointer
>
> s/if and//
>
>> +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.
>
>> +.TP 7
>> +.BI "Option \*qAccelerationNumerator\*q \*q" integer \*q
>> +Set the numerator of the acceleration factor.
>> +.TP 7
>> +.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.
>
>> +.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."
>
>> +.TP 7
>> +.BI "Option \*qAccelerationProfile\*q \*q" integer \*q
>> +Select the profile, which determines actual acceleration as a function of
>> +device velocity and other acceleration settings.
>> +.PP
>> +.RS 6
>> +.nf
>> +.B " 0 classic (mostly compatible)"
>> +.B "-1 none (just DownScale is applied)"
>> +.B " 1 device-dependent"
>> +.B " 2 polynomial"
>> +.B " 3 smooth linear"
>> +.B " 4 simple"
>> +.B " 5 power"
>> +.B " 6 linear"
>> +.B " 7 limited"
>> +.fi
>> +.RE
>> +.TP 7
>> +.BI "Option \*qDownScale\*q \*q" real \*q
>> +Makes the pointer go
>> +.B DownScale
>> +times slower than without this option. Most useful for high-resolution devices.
>
> deceleration/downscaling wording as outlined at the top of the email, please
> adjust as necessary.
>
>> +.TP 7
>> +.BI "Option \*qAdaptiveDeceleration\*q \*q" real \*q
>> +Most profiles allow to actually decelerate the pointer when going slow. Use this
>> +for very accurate hit-the-pixel work.
>
> what does this option set? what value would I set? high ones or low ones?
> also, "allow to decelerate" or simply "decelerated"? in which profile is
> this option not applied?
>
>
>> +.TP 7
>> +.BI "Option \*qAccelerationScheme\*q \*q" string \*q
>> +Selects the scheme, which is the underlying algorithm. Basically one can
>> +resurrect the old (severely constrained) algorithm, choose none or the current
>> +default.
>> +.PP
>> +.RS 7
>> +.nf
>> +.B "predictable default algorithm (behaving intuitively predictable)"
>> +.B "lightweight old acceleration code"
>> +.B "none no acceleration or deceleration"
>> +
>
> biased, very biased :)
> 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".
>
>> .SH "INPUTCLASS SECTION"
>> The config file may have multiple
>> .B InputClass
>> diff --git a/os/utils.c b/os/utils.c
>> index 3718b17..449460b 100644
>> --- a/os/utils.c
>> +++ b/os/utils.c
>> @@ -472,7 +472,7 @@ AdjustWaitForDelay (pointer waitTime, unsigned long newdelay)
>> void UseMsg(void)
>> {
>> ErrorF("use: X [:<display>] [option]\n");
>> - ErrorF("-a # mouse acceleration (pixels)\n");
>> + ErrorF("-a # default mouse acceleration (factor)\n");
>> ErrorF("-ac disable access control restrictions\n");
>> ErrorF("-audit int set audit trail level\n");
>> ErrorF("-auth file select authorization file\n");
>> @@ -524,7 +524,7 @@ void UseMsg(void)
>> #endif
>> ErrorF("-retro start with classic stipple and cursor\n");
>> ErrorF("-s # screen-saver timeout (minutes)\n");
>> - ErrorF("-t # mouse threshold (pixels)\n");
>> + ErrorF("-t # default mouse threshold (pixels/t)\n");
>> ErrorF("-terminate terminate at server reset\n");
>> ErrorF("-to # connection time out\n");
>> ErrorF("-tst disable testing extensions\n");
>> --
>> 1.6.4.4
>
> 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.
>
> Cheers,
> Peter
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-os-state-effect-of-a-and-t-options-more-precisely.patch
URL: <http://lists.x.org/archives/xorg/attachments/20100106/be05018c/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-doc-actually-document-SendDragEvents.patch
URL: <http://lists.x.org/archives/xorg/attachments/20100106/be05018c/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-xfree86-document-pointer-acceleration-in-xorg.conf.m.patch
URL: <http://lists.x.org/archives/xorg/attachments/20100106/be05018c/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0004-whitespace-fixes.patch
URL: <http://lists.x.org/archives/xorg/attachments/20100106/be05018c/attachment-0003.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0005-xfree86-init-pointer-feedback-controls-from-options.patch
URL: <http://lists.x.org/archives/xorg/attachments/20100106/be05018c/attachment-0004.ksh>
More information about the xorg
mailing list