Synaptics patch: orientation

[Peter Hutterer]

On Sun, Nov 23, 2008 at 12:29:29PM +0100, Mildred Ki'Lya wrote:
> Well, I send them manually, and I used mercurial queues, I hope you'll
> forgive me :) I'm more used to them, that's all. Even if git seems very
> similar, it's still a bit different.

> > I like extensive commit messages where appropriate, but these ones
> > are a bit noisy. There's a lot of stuff that is obvious through the
> > code, so shortening it down to what is not immediately obvious would
> > be good. That goes for all three commits.
> 
> Well, probably, but in fact, I often find myself reading and reading
> the code to find out what it is doing. And if I could just read a
> summary of it, it would take me less much time. But I agree that for
> someone who knows the code, much of it may be unnecessary.

A commit should describe the intention of the changes, not the changes to the
code. There's not always a clear distinction, so you'll have to apply common
sense. 

> But, well, with patches, you won't have that kind of problems :)

actually, I do, because I need to write the commit messages myself now. If you
attach them in git format, I just need to run a single command to apply each
of them. So please resend your final patches as git formatted ones.

> > Also, if the orientation is to be changed at runtime, it should have
> > property support. You can provide that as a separate patch if you
> > want to.
> 
> I don't exactly understand what you mean here. If by property support
> you mean the possibility to modify the configuration using the shared
> memory, it's already implemented (there would be no point otherwise).
> If you mean something else ... well, I don't understand what it is.

google for input device property support, or just read the commit logs of
either synaptics, joystick or evdev. Long term, the shared memory will go away
for all but for debugging purposes.

> > > +.
> > > +.TP
> > > +.BI "Option \*qDontReportSize\*q \*q" boolean \*q
> > 
> > less verbose please.
> 
> is that ok?
> 
> 
> .BI "Option \*qReportSize\*q \*q" boolean \*q
> When enabled (default behaviour), this option will report the size of the
> trackpad to Xorg. The server usually uses this information to modify the speed
> of the axis depending on the size of the trackpad. Thus, it can lean to
> unreachable pixels (when the speed is increased too much) and can lead to
> strange behaviours when the trackpad is oriented (the speed is over-amplified in
> a direction). So if you plan to use the orientation feature, you probably want
> to disable this (there is no way to change the trackpad size at runtime).

I'd prefer it to be even simpler but that's getting down to personal taste.

> > s/.../This option always specifies the vertical speed, regardless of
> > the orientation./
> > 
> > Just merge it into the VertSpeed section (see below) and adjust the
> > description accordingly:
> 
> Changed to:
> 
> .TP
> .BI "Option \*qHorizSpeed\*q \*q" float \*q
> Speed scale for the X axis. Modify the speed regardless of the orientation.
> ..
> .TP
> .BI "Option \*qVertSpeed\*q \*q" float \*q
> Speed scale for the Y axis. Modify the speed regardless of the orientation.

s/Modify/Modifies/

> 
 
> diff -r 9eb9da460bd8 include/synaptics.h
> --- a/include/synaptics.h	Sun Nov 23 11:17:50 2008 +0100
> +++ b/include/synaptics.h	Sun Nov 23 11:48:02 2008 +0100
> @@ -133,6 +133,7 @@
>      double press_motion_min_factor;	    /* factor applied on speed when finger pressure is at minimum */
>      double press_motion_max_factor; 	    /* factor applied on speed when finger pressure is at minimum */
>      Bool grab_event_device;		    /* grab event device for exclusive use? */
> +    int orientation;			    /* orientation of the trackpad */
>  } SynapticsSHM;
>  
>  /*
> diff -r 9eb9da460bd8 man/synaptics.man
> --- a/man/synaptics.man	Sun Nov 23 11:17:50 2008 +0100
> +++ b/man/synaptics.man	Sun Nov 23 11:48:02 2008 +0100
> @@ -384,6 +384,14 @@
>  .
>  This can be achieved by switching to a text console and then switching
>  back to X.
> +.TP
> +.BI "Option \*qOrientation\*q \*q" integer \*q
> +This option changes the orientation of the trackpad and can
> +takes values from 0 to 3. The default value 0 implies a normal orientation,
> +other values can be used to have respectively an orientation set to the
> +left, an inverted orientation, and an orientation set to the right.
> +This may be useful in combination with the orientation option of the XRandR
> +extension. The values used are the same to the values used by XRandR \-o option.

XRandR is the extension, xrandr the tool, you need to s/XRandR/xrandr/ in the
last line.

from the xrandr man page:
"This specifies the orientation of the screen, and can be one of normal,
inverted, left or right."
This is about as consise as it gets, can we have a similar one?

> @@ -751,6 +753,8 @@
>      SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
>      unsigned char map[SYN_MAX_BUTTONS + 1];
>      int i;
> +    int valuator_minx = 0, valuator_miny = 0;
> +    int valuator_maxx = -1, valuator_maxy = -1;
>  
>      DBG(3, ErrorF("Synaptics DeviceInit called\n"));
>  
> @@ -773,17 +777,34 @@
>  			    GetMotionHistorySize(), 2
>  #endif
>  			    );
> +
> +
> +    /* Handle orientation and size of trackpad */
> +
> +    if(priv->synpara->orientation) {
> +	if(priv->minx < priv->maxx && priv->miny < priv->maxy){
> +	    valuator_minx = priv->minx;
> +	    valuator_miny = priv->miny;
> +	    valuator_maxx = priv->maxx;
> +	    valuator_maxy = priv->maxy;
> +	    HandleOrientation_int(priv->synpara->orientation,
> +				  &valuator_minx, &valuator_miny);
> +	    HandleOrientation_int(priv->synpara->orientation,
> +				  &valuator_maxx, &valuator_maxy);
> +	}
> +    } else if(priv->minx < priv->maxx) {
> +	valuator_minx = priv->minx;
> +	valuator_maxx = priv->maxx;
> +    } else if(priv->miny < priv->maxy) {
> +	valuator_miny = priv->miny;
> +	valuator_maxy = priv->maxy;
> +    }
> +

Please rethink this block. You're duplicating code, and there's a possible
path where you only set min/max x although priv->min/max y is set.

 
>  #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) == 0
> @@ -882,7 +903,7 @@
>  static edge_type
>  edge_detection(SynapticsPrivate *priv, int x, int y)
>  {
> -    edge_type edge = 0;
> +    edge_type edge = 0, oriented_edge = 0;
>  
>      if (priv->synpara->circular_pad)
>  	return circular_edge_detection(priv, x, y);
> @@ -897,7 +918,43 @@
>      else if (y > priv->synpara->bottom_edge)
>  	edge |= BOTTOM_EDGE;
>  
> -    return edge;
> +    /* orient edge -> oriented_edge */

wow, that comment confused me ("wtf? what is an orient edge?"). Just replace
it with "handle touchpad orientation" or so.

bail out early here if (!orientation), seems that'll be the case 99% of the
time.

> +
> +    if(edge & BOTTOM_EDGE) {
> +      switch(priv->synpara->orientation) {
> +	  default:
> +	  case 0:  oriented_edge |= BOTTOM_EDGE;	break;
> +	  case 1:  oriented_edge |= LEFT_EDGE;	break;
> +	  case 2:  oriented_edge |= TOP_EDGE;	break;
> +	  case 3:  oriented_edge |= RIGHT_EDGE;	break;
> +      }
> +    } else if(edge & TOP_EDGE) {
> +      switch(priv->synpara->orientation) {
> +	  default:
> +	  case 0:  oriented_edge |= TOP_EDGE;	break;
> +	  case 1:  oriented_edge |= RIGHT_EDGE;	break;
> +	  case 2:  oriented_edge |= BOTTOM_EDGE;	break;
> +	  case 3:  oriented_edge |= LEFT_EDGE;	break;
> +      }
> +    }
> +    if(edge & LEFT_EDGE) {
> +      switch(priv->synpara->orientation) {
> +	  default:
> +	  case 0:  oriented_edge |= LEFT_EDGE;	break;
> +	  case 1:  oriented_edge |= TOP_EDGE;	break;
> +	  case 2:  oriented_edge |= RIGHT_EDGE;	break;
> +	  case 3:  oriented_edge |= BOTTOM_EDGE;	break;
> +      }
> +    } else if(edge & RIGHT_EDGE) {
> +      switch(priv->synpara->orientation) {
> +	  default:
> +	  case 0:  oriented_edge |= RIGHT_EDGE;	break;
> +	  case 1:  oriented_edge |= BOTTOM_EDGE;	break;
> +	  case 2:  oriented_edge |= LEFT_EDGE;	break;
> +	  case 3:  oriented_edge |= TOP_EDGE;	break;
> +      }
> +    }
> +    return oriented_edge;
>  }
>  
>  static CARD32
> @@ -1397,6 +1454,54 @@
>      return x0 * 0.3 + x1 * 0.1 - x2 * 0.1 - x3 * 0.3;
>  }
>  
> +static void
> +HandleOrientation_double(int orientation, double *dx, double *dy) {
> +    double tmp;
> +    switch(orientation) {
> +	default:
> +	case 0:
> +	    break;
> +	case 1: /* left */
> +	    tmp =  *dx;
> +	    *dx = -*dy;
> +	    *dy =  tmp;
> +	    break;
> +	case 2: /* inverted */
> +	    *dx = -*dx;
> +	    *dy = -*dy;
> +	    break;
> +	case 3: /* right */
> +	    tmp =  *dx;
> +	    *dx =  *dy;
> +	    *dy = -tmp;
> +	    break;
> +    }
> +}
> +

you might want to consider an enum for the orientations, it'll make the code
much easier to read. Oh, and please don't mix caps and underscore coding
style. DontDo_this().
patch 2:

>  The LeftEdge, RightEdge, TopEdge and BottomEdge parameters are used to
> diff -r 2a2d09e8b175 src/synaptics.c
> --- a/src/synaptics.c	Sun Nov 23 11:48:02 2008 +0100
> +++ b/src/synaptics.c	Sun Nov 23 12:04:14 2008 +0100
> @@ -477,6 +477,7 @@
>      pars->press_motion_min_z = xf86SetIntOption(opts, "PressureMotionMinZ", pressureMotionMinZ);
>      pars->press_motion_max_z = xf86SetIntOption(opts, "PressureMotionMaxZ", pressureMotionMaxZ);
>      pars->orientation        = xf86SetIntOption(opts, "Orientation", 0);
> +    pars->report_size   = xf86SetBoolOption(opts, "ReportSize", TRUE);
>  
>      pars->min_speed = synSetFloatOption(opts, "MinSpeed", 0.4);
>      pars->max_speed = synSetFloatOption(opts, "MaxSpeed", 0.7);
> @@ -781,23 +782,25 @@
>  
>      /* Handle orientation and size of trackpad */
>  
> -    if(priv->synpara->orientation) {
> -	if(priv->minx < priv->maxx && priv->miny < priv->maxy){
> +    if(priv->synpara->report_size == TRUE) {

The convention is not to check == TRUE, or == FALSE.

> +	if(priv->synpara->orientation) {
> +	    if(priv->minx < priv->maxx && priv->miny < priv->maxy){
> +		valuator_minx = priv->minx;
> +		valuator_miny = priv->miny;
> +		valuator_maxx = priv->maxx;
> +		valuator_maxy = priv->maxy;
> +		HandleOrientation_int(priv->synpara->orientation,
> +				      &valuator_minx, &valuator_miny);
> +		HandleOrientation_int(priv->synpara->orientation,
> +				      &valuator_maxx, &valuator_maxy);
> +	    }
> +	} else if(priv->minx < priv->maxx) {
>  	    valuator_minx = priv->minx;
> +	    valuator_maxx = priv->maxx;
> +	} else if(priv->miny < priv->maxy) {
>  	    valuator_miny = priv->miny;
> -	    valuator_maxx = priv->maxx;
>  	    valuator_maxy = priv->maxy;
> -	    HandleOrientation_int(priv->synpara->orientation,
> -				  &valuator_minx, &valuator_miny);
> -	    HandleOrientation_int(priv->synpara->orientation,
> -				  &valuator_maxx, &valuator_maxy);
>  	}
> -    } else if(priv->minx < priv->maxx) {
> -	valuator_minx = priv->minx;
> -	valuator_maxx = priv->maxx;
> -    } else if(priv->miny < priv->maxy) {
> -	valuator_miny = priv->miny;
> -	valuator_maxy = priv->maxy;
>      }

This block suffers from the same issue as the similar block in the first
patch.



> diff -r 7a734f5e122b include/synaptics.h
> --- a/include/synaptics.h	Sun Nov 23 12:04:14 2008 +0100
> +++ b/include/synaptics.h	Sun Nov 23 12:21:26 2008 +0100
> @@ -135,6 +135,8 @@
>      Bool grab_event_device;		    /* grab event device for exclusive use? */
>      int orientation;			    /* orientation of the trackpad */
>      Bool report_size;			    /* Report the size of the trackpad to Xorg */
> +    double horiz_speed;			    /* Horizontal speed for mouse movements */
> +    double vert_speed;			    /* Vertical speed for mouse movements */
>  } SynapticsSHM;
>  
>  /*
> diff -r 7a734f5e122b man/synaptics.man
> --- a/man/synaptics.man	Sun Nov 23 12:04:14 2008 +0100
> +++ b/man/synaptics.man	Sun Nov 23 12:21:26 2008 +0100
> @@ -196,6 +196,12 @@
>  .TP
>  .BI "Option \*qTrackstickSpeed\fR (\*q \*q" float \*q)
>  Speed scale when in trackstick emulation mode.
> +.TP
> +.BI "Option \*qHorizSpeed\*q \*q" float \*q
> +Speed scale for the X axis. Modify the speed regardless of the orientation.
> +.TP
> +.BI "Option \*qVertSpeed\*q \*q" float \*q
> +Speed scale for the Y axis. Modify the speed regardless of the orientation.
>  .TP
>  .BI "Option \*qPressureMotionMinZ\*q \*q" integer \*q
>  Finger pressure at which minimum pressure motion factor is applied.
> diff -r 7a734f5e122b src/synaptics.c
> --- a/src/synaptics.c	Sun Nov 23 12:04:14 2008 +0100
> +++ b/src/synaptics.c	Sun Nov 23 12:21:26 2008 +0100
> @@ -487,6 +487,8 @@
>      pars->coasting_speed = synSetFloatOption(opts, "CoastingSpeed", 0.0);
>      pars->press_motion_min_factor = synSetFloatOption(opts, "PressureMotionMinFactor", 1.0);
>      pars->press_motion_max_factor = synSetFloatOption(opts, "PressureMotionMaxFactor", 1.0);
> +    pars->horiz_speed = synSetFloatOption(opts, "HorizSpeed", 1.0);
> +    pars->vert_speed = synSetFloatOption(opts, "VertSpeed", 1.0);
>      pars->grab_event_device = xf86SetBoolOption(opts, "GrabEventDevice", TRUE);
>  
>      /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge parameters */
> @@ -1555,8 +1557,10 @@
>  		dx = dx * dtime * para->trackstick_speed;
>  		dy = dy * dtime * para->trackstick_speed;
>  	    } else if (moving_state == MS_TOUCHPAD_RELATIVE) {
> -		dx = estimate_delta(hw->x, HIST(0).x, HIST(1).x, HIST(2).x);
> -		dy = estimate_delta(hw->y, HIST(0).y, HIST(1).y, HIST(2).y);
> +		dx = estimate_delta(hw->x, HIST(0).x, HIST(1).x, HIST(2).x)
> +		   * para->horiz_speed;
> +		dy = estimate_delta(hw->y, HIST(0).y, HIST(1).y, HIST(2).y)
> +		   * para->vert_speed;

indentation.
>  
>  		HandleOrientation_double(para->orientation, &dx, &dy);
>  
> diff -r 7a734f5e122b tools/synclient.c
> --- a/tools/synclient.c	Sun Nov 23 12:04:14 2008 +0100
> +++ b/tools/synclient.c	Sun Nov 23 12:21:26 2008 +0100
> @@ -125,6 +125,8 @@
>      DEFINE_PAR("GrabEventDevice",      grab_event_device,       PT_BOOL,   0, 1),
>      DEFINE_PAR("Orientation",          orientation,             PT_INT,    0, 3),
>      DEFINE_PAR("DontReportSize",       dont_report_size,        PT_BOOL,   0, 1),
> +    DEFINE_PAR("HorizSpeed",           horiz_speed,             PT_DOUBLE, 0, 10),
> +    DEFINE_PAR("VertSpeed",            vert_speed,              PT_DOUBLE, 0, 10),
>      { NULL, 0, 0, 0, 0 }
>  };

Thanks for amending your patch, don't forget to CC me on the next lot.
btw, you might want to start reading the man page for git-send-email :)

Cheers,
  Peter