pointer acceleration property rename and docs
Peter Hutterer
peter.hutterer at who-t.net
Tue Jan 5 16:02:56 PST 2010
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
More information about the xorg
mailing list