Synaptics patch: orientation

[Peter Hutterer]

On Tue, Nov 11, 2008 at 01:27:34PM +0100, Mildred Ki'Lya wrote:
> Well, I don't really know how to creates patch series using git. It was
> easier for me to create a repository I made available on github:
> 
> git://github.com/mildred/xorg-synaptics-orientation-patch.git
> http://github.com/mildred/xorg-synaptics-orientation-patch/
 
git-format-patch HEAD~4 (for the last 4 patches)
git-format-patch -n HEAD~4 (to create a [PATCH 1/4] series)

Then send these patches to the list (e.g. with git-send-email). Reason being
is that it's easier to quote and point out things to change.

Now, for the suggested changes below: the easiest way is to go
edit
git-commit 
edit
git-commit
edit
git-commit

git rebase -i HEAD~8 (for the last 8 commits)
git-format-patch HEAD~4

then rearrange the commits, and squash them appropriately that you end up with
the semantically correct ones again. Just play around with git rebase -i a
bit, you'll get used to it quite quickly.
Once you're done, please resend the patches.


> From 3a4cb5a4e3b2856d3aba4f9abfb9732b555494bf Mon Sep 17 00:00:00 2001
> From: Mildred Ki'Lya <mildred593(at)online.fr>
> Date: Tue, 11 Nov 2008 13:02:36 +0100
> Subject: [PATCH] Added Orientation option

> Changed bits of code all around to handle that properly. Especially in the
> function ComputeDeltas() and HandleScrolling(). Those two functions convert
> (using helper functions that I've added) the hw->x and hw->y coordinates to
> oriented_x and oriented_y coordinates. The angle used for circular scrolling
> do not use oriented coordinates as i've taught it was not necessary, but I
> haven't tested it though (i don't use circular scrolling).

> Moreover, in the function HandleState(), the variable edge contains how the
> oriented edge. So the functions that use the edge are automatically oriented.
> This is useful for HandleTap(). The buttons in the corners of the pads are
> oriented without effort.

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.

> +.TP
> +.BI "Option \*qOrientation\*q \*q" integer \*q
> +This option can be used to change the orientation of the trackpad and can

... This option changes the orientation ...

> +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 combinaison with the orientation option of the XRandR
                        ^^ typo
> +extension. You may notice that the values used are the same to the values
> +used by XRandR.

Again, please keep it short and to-the point. e.g. the second-to-last sentence
is superfluous, last one can be shortened (no "You may notice").


> @@ -1593,6 +1649,9 @@ HandleScrolling(SynapticsPrivate *priv, struct SynapticsHwState *hw,
>  {
>      SynapticsSHM *para = priv->synpara;
>      int delay = 1000000000;
> +    int oriented_x = hw->x, oriented_y = hw->y;

Just use x, y instead of oriented_x, oriented_y please.
 
>      edge = edge_detection(priv, hw->x, hw->y);
> +    edge = HandleEdgeOrientation(para->orientation, edge);

can we merge that into edge_detection so we don't have separate calls?


Missing from this patch is the code to readjust the valuators if the
orientation was specified at startup.
i.e. if option Orientation is set, change the xf86InitValuatorAxisStruct
around accordingly so the ranges are correct.
we can't do anything about run-time changes, but at least it should be correct
for the startup setting.

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.

> From 3c16621ca139ea6c4ef121da9a245c5456fe59ec Mon Sep 17 00:00:00 2001
> From: Mildred Ki'Lya <mildred593(at)online.fr>
> Date: Tue, 11 Nov 2008 13:11:58 +0100
> Subject: [PATCH] Added DontReportSize option

Replace as ReportSize option, set to TRUE by default.

> Perhaps there is a way to change the trackpad dimentions when we change
> orientation, but I don't know enough of the Worg internals to do that.

there isn't. not at runtime anyway.

> +Along with this option, you might be interested to enable the DontReportSize
> +option. Read its documentation to know why.

replace with "See also: ReportSize"

> +.
> +.TP
> +.BI "Option \*qDontReportSize\*q \*q" boolean \*q
> +This option prevent the synaptics driver from reporting the size of the trackpad
> +to Xorg. Xorg can use this information to amplify the movements in one
> +direction. For example, if your trackpad is wider than higher, Xorg will speed
> +up your vertical movements. For example, moving the mouse cursor every two
> +pixels when synaptics told Xorg that there was a movement on one unit along the
> +y axis.
> +.
> +This is particularly useful with the Orientation option which effectively swaps
> +the axis inside the synaptics driver. At that time, Xorg no longer have relevant
> +information about the size of the x and y axis and might amplifying the
> +movements of the wrong axis. Causing an unusable trackpad, over sensitive
> +horizontally and very slow vertically for example.

less verbose please.


> +This option isn't affected by the orientation, so when the screen is rotated
> +using XRandR, the movements you have with your oriented trackpad will be similar
> +to those you have with a non oriented trackpad and a non rotated screen.

s/.../This option always specifies the vertical speed, regardless of the
orientation./

> +.
> +.TP
> +.BI "Option \*qHorizSpeed\*q \*q" float \*q
> +Changes the horizontal speed similarly to VertSpeed.

Just merge it into the VertSpeed section (see below) and adjust the description
accordingly:
>> +.TP
>> +.BI "Option \*qVertSpeed\*q \*q" float \*q
>> +.TP
>> +.BI "Option \*qHorizSpeed\*q \*q" float \*q

> 	    if (priv->moving_state == MS_TRACKSTICK) {
> -		dx = (hw->x - priv->trackstick_neutral_x);
> -		dy = (hw->y - priv->trackstick_neutral_y);
> +		dx = (hw->x - priv->trackstick_neutral_x) * para->horiz_speed;
> +		dy = (hw->y - priv->trackstick_neutral_y) * para->vert_speed;

different horiz/vert speed for the trackstick? That doesn't seem like a good idea.

>  	    } 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;
> 
> 		HandleOrientation_double(para->orientation, &dx, &dy);


Now we have 
    MinSpeed
    MaxSpeed
    TrackstickSpeed
    VertSpeed
    HorizSpeed

Min/Max is applied on top of Vert/Horiz, which is confusing at best. I'd be
good to re-think the in-driver speed settings and see if we can simplify them
a bit.


> From 90c87f36f3a6940d39759588e48c6104b12fb6f2 Mon Sep 17 00:00:00 2001
> From: Mildred Ki'Lya <mildred593(at)online.fr>
> Date: Tue, 11 Nov 2008 13:20:38 +0100
> Subject: [PATCH] Unimportant changes

Please split into two patches, not titled "Unimportant changes"

> I changed somewhere a priv->synpara->... in para->...

"Cosmetic fix. Change a priv->synpara->.. to para->... in ComputeDeltas"

is a good enough commit msg. for this one-liner.


Cheers,
  Peter