Merge lp:~vanvugt/ubuntu/precise/xserver-xorg-input-synaptics/fix-754000 into lp:ubuntu/precise/xserver-xorg-input-synaptics

[Daniel van Vugt]

Fix incorrect counting of touching fingers when grail clients (like Unity) are active. (LP: #754000)

This fix also appears to fix multi-touch bug 798047.

[Daniel van Vugt (vanvugt)]

@Chase.... ping. Any thoughts on this new version?

[Chase Douglas (chasedouglas)]

It looks reasonable to me. The reason I haven't been very active about this is because this will all change in precise (hopefully for the better :).

There's a lot of complexities in this code right now, though, so I'm not sure if it will break anything.

[Daniel van Vugt (vanvugt)]

Could you please test it with your Inspiron (the machine which previous versions of this fix broke)?

If the issue with 2-finger right-click/scrolling occurs again (as it did for you ages ago), then I think I know how to fix that. But only if it occurs again...

[Stéphane Graber (stgraber)]

I'm going to mark the merge proposal as work in progress for now until you and Chase are sure it won't break things.
Just mark it needs review again when the change has been tested.

[Chase Douglas (chasedouglas)]

I'm running your branch now on my mini and I don't see any issues. I don't see any differences at all though. Can you refresh my memory of what this fixes?

[Daniel van Vugt (vanvugt)]

I've clarified the description of bug 754000 which describes the problem. The fix is mostly for click-pad owners right now (ie. Apple devices and some others). I am seeking to test whether the fix continues breaks anything on regular touchpads. Your previous comment suggests it doesn't break anything :)

[Chase Douglas (chasedouglas)]

Ahh. I understand now what this patch is trying to do. My question then is: how does this help? Unity still grabs three touch taps. Using this branch I still can't three-tap middle button click.

[Daniel van Vugt (vanvugt)]

Please refer to the discussion in bug 754000. We are primarily fixing clickpads (Macs/Magic Touchpad) right now, not touch pads. And on a clickpad the patch successfully triggers a middle-click when you 3-finger-click. The fact that the love handles appear is not important. What is important is that you can finally do a middle click again when the patch is applied.

[Chase Douglas (chasedouglas)]

Ok, I think the goal is fine. Unfortunately, I can't get it to work with my magic trackpad. Three finger taps and three finger clicks both bring up with love handles but neither will cause a middle finger click :(.

[Daniel van Vugt (vanvugt)]

I just tested the proposal with a Magic Trackpad on oneiric again. It does work, but if you're testing with the Magic Trackpad then you need to work around bug 862094 first:

synclient ClickFinger3=2
or
xinput set-prop 'Apple Wireless Trackpad' 'Synaptics Click Action' 1 3 2

[Daniel van Vugt (vanvugt)]

Chase, please retest with the above workaround.

[Barry Warsaw (barry)]

Hi Daniel,

I tested your branch on my MBP1,1 running Oneiric, with Magic Trackpad. I tried both your workarounds for bug 862094 in the comment above, but in no cases could I get three finger tap to provide middle button events. I looked at the events with xev, but then also tried to do a middle-button paste in a Terminal. I'm not sure what if anything else I can try.

[Daniel van Vugt (vanvugt)]

Barry, this bug is not about tapping. It's about 3-finger clicking with clickpads. Is that what you tested?
Please refer to bug 754000 for a detailed discussion on that.

Also, did you test the MBP's clickpad or a Magic Trackpad? Only the latter needs a workaround for bug 862094.

Strange that it works for me on two different machines; Macbook Air and a PC with Magic Trackpad.

Sadly this fix (the superseded version) will be 6 months old in 3 days, and still under review. :(

[Bryce Harrington (bryce)]

This patch looks straightforward enough but has been sitting here for some time. Given it appears to be blocked on testing and cnd's busy with other matters, what if we just upload the patch for now and see how it goes? It looks trivial enough to revert it if we find it causes regressions.

I've verified the patch builds with current -synaptics.

[Bryce Harrington (bryce)]

Actually, the patch applies but the build fails.

../../src/grail.c: In function 'GetClients':
../../src/grail.c:110:37: error: 'struct _DeviceIntRec' has no member named 'u'
../../src/grail.c:147:9: warning: implicit declaration of function 'wGestureMasks' [-Wimplicit-function-declaration]
../../src/grail.c:149:13: error: unknown type name 'GestureClientsPtr'
../../src/grail.c:152:48: error: invalid type argument of '->' (have 'int')

In chatting with cnd, it seems this affects some moderately complex areas in the code. I gather he'd prefer to incorporate it with his work rather than have the patch go in via sponsors. So I will drop this from the sponsors queue, and hopefully cnd will have time to look into this deeper or give more detailed feedback on how he would like it to work.

review: Disapprove

[Bryce Harrington (bryce)]

Actually, it should be 'Needs Fixing' rather than 'Disapprove'.

review: Needs Fixing (sponsorship)

[Daniel van Vugt (vanvugt)]

I don't think that build failure is related to this patch at all. It's a different C file and no headers have changed.

Though I expected Chase would want to weave it into his work, given the number of patches quilted over xserver-xorg-input-synaptics in natty and oneiric. The branch had almost become unmaintainable with so many patches, so it's a good thing if Chase is cleaning things up all round.

[Chase Douglas (chasedouglas)]

In Precise, we no longer have grail patched into synaptics. The uTouch stack is moving to the client side of X. Thus, this patch is obsolete. I don't think the upstream synaptics code exhibits the problems this patch is meant to address, so I'm disapproving it.

review: Disapprove

[Daniel van Vugt (vanvugt)]

Sounds promising, thanks.

[Chase Douglas (chasedouglas)]

Rejecting since this functionality is back, but disabled by default.

[Chase Douglas (chasedouglas)]

Oh, I was hoping to reject it, but apparently I can't...

Unmerged revisions

43. By Daniel van Vugt on 2011-10-21

Number of active touches can be less than number of active fingers/contacts
so ask grail for the real number of fingers when clicking. This fixes
3-finger clicks (middle click) when there are geis/grail clients active
such as Unity. (LP: #754000)

=== added file 'debian/patches/130_multifinger_clicks_lp754000.patch'
--- debian/patches/130_multifinger_clicks_lp754000.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/130_multifinger_clicks_lp754000.patch	2011-10-21 08:47:25 +0000
@@ -0,0 +1,62 @@
+Index: fix-754000-oneiric/src/eventcomm.c
+===================================================================
+--- fix-754000-oneiric.orig/src/eventcomm.c	2011-10-21 16:04:59.050119550 +0800
++++ fix-754000-oneiric/src/eventcomm.c	2011-10-21 16:11:32.306119503 +0800
+@@ -541,6 +541,38 @@
+     return rc;
+ }
+ 
++static int
++CountFingers(const SynapticsPrivate *priv, const EventcommPrivate *ecpriv)
++{
++    int count = -1;
++    /*
++     * We need to count the fingers manually when grail is active.
++     * This is because if grail matches a gesture (like 3 fingers touching)
++     * then ABS_MT_SLOT is never sent to EventProcessEvent and active_touches
++     * remains zero. So we ask grail how many fingers really are touching,
++     * but only count those that are inside the active area.
++     */
++    if (priv->has_touch) {
++        if (ecpriv->grail && !ecpriv->semi_mt) {
++            struct grail_contact contact[5];
++            int i, contacts;
++            contacts = grail_get_contacts(ecpriv->grail, contact,
++                                          sizeof(contact)/sizeof(contact[0]));
++            count = 0;
++            for (i = 0; i < contacts; i++) {
++                if (is_inside_active_area(priv, contact[i].pos.x,
++                                                contact[i].pos.y)) {
++                    count++;
++                }
++            }
++        } else if (ecpriv->active_touches < 2) {
++            /* Old method, used before CountFingers was added */
++            count = ecpriv->active_touches;
++        }
++    }
++    return count;
++}
++
+ Bool
+ EventProcessEvent(InputInfoPtr pInfo, struct CommData *comm,
+                   struct SynapticsHwState *hwRet, const struct input_event *ev)
+@@ -550,14 +582,16 @@
+     SynapticsParameters *para = &priv->synpara;
+     struct SynapticsHwState *hw = &(comm->hwState);
+     Bool v;
++    int numfingers;
+ 
+     switch (ev->type) {
+     case EV_SYN:
+         switch (ev->code) {
+         case SYN_REPORT:
+             ProcessTouch(pInfo, priv);
+-            if (priv->has_touch && ecpriv->active_touches < 2)
+-                hw->numFingers = ecpriv->active_touches;
++            numfingers = CountFingers(priv, ecpriv);
++            if (numfingers >= 0)
++                hw->numFingers = numfingers;
+             else if (comm->oneFinger)
+                 hw->numFingers = 1;
+             else if (comm->twoFingers)

=== modified file 'debian/patches/series'
--- debian/patches/series	2011-07-05 16:40:31 +0000
+++ debian/patches/series	2011-10-21 08:47:25 +0000
@@ -16,3 +16,4 @@
 122_revert_pressure_finger_default.patch
 123_order_ProcessTouch_for_numFingers.patch
 124_syndaemon_events.patch
+130_multifinger_clicks_lp754000.patch