Synaptics patch: orientation
Mildred Ki'Lya
ml.mildred593 at online.fr
Sun Nov 23 03:29:29 PST 2008
Hi,
Excuse me for the delay, but I had many other things to do. So I just
had to wait until I had some time to spend on synaptics. So here are
the modified patches.
Le Thu 13/11/2008 à 12:09 Peter Hutterer à écrit:
> 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.
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.
> > 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.
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.
But, well, with patches, you won't have that kind of problems :)
> > ... man page ...
>
> ... This option changes the orientation ...
>
> ^^ typo
>
> 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").
fixed
> > @@ -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.
fixed
> > 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?
merged
> 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.
done
> 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.
> > 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.
done
>
> > +Along with this option, you might be interested to enable the
> > DontReportSize +option. Read its documentation to know why.
>
> replace with "See also: ReportSize"
done
> > +.
> > +.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).
> 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.
I think this is better, isn't it? I just took inspiration from
TrackstickSpeed. I also moved it just below TrackstickSpeed.
> > 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.
Seems like an error, thanks. When i did this, I probably didn't
knew what was the trackstick :/
Fixed
>
> 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.
Well, MinSpeed and MaxSpeed are used to change the speed according to
the finger pressure (or do I miss something?). HorizSpeed, VertSpeed
and TrackstickSpeed are used to globally change the speed. I don't see
any problem in all that.
>
>
> > 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"
>
Just split it.
Thanks for your review, and I hope I didn't made you wait too long.
:)
Mildred
--
Mildred Ki'Lya
╭───────── ｍｉｌｄｒｅｄ５９３＠ｏｎｌｉｎｅ．ｆｒ ──────────
│ Jabber, GoogleTalk: <mildred at jabber.fr>
│ Site: <http://ki.lya.online.fr> GPG ID: 9A7D 2E2B
│ Fingerprint: 197C A7E6 645B 4299 6D37 684B 6F9D A8D6 9A7D 2E2B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 001-orientation.patch
Type: text/x-patch
Size: 12328 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081123/2802f2fe/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 002-report-size.patch
Type: text/x-patch
Size: 4048 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081123/2802f2fe/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 003-speed-x-y.patch
Type: text/x-patch
Size: 3105 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081123/2802f2fe/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 004-one-line-simplification.patch
Type: text/x-patch
Size: 438 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081123/2802f2fe/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 005-emulate-clicks.patch
Type: text/x-patch
Size: 639 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081123/2802f2fe/attachment-0004.bin>
More information about the xorg
mailing list