<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - Lenovo P50 - Slow fine touchpad movement makes it jump (bug 2)" href="https://bugs.freedesktop.org/show_bug.cgi?id=105963#c10">Comment # 10</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - Lenovo P50 - Slow fine touchpad movement makes it jump (bug 2)" href="https://bugs.freedesktop.org/show_bug.cgi?id=105963">bug 105963</a> from <a class="email" href="mailto:peter.hutterer@who-t.net" title="Peter Hutterer <peter.hutterer@who-t.net>"> Peter Hutterer</a> <pre>Comment on <a href="attachment.cgi?id=138744" name="attach_138744" title="patch for P50">attachment 138744</a> <a href="attachment.cgi?id=138744&action=edit" title="patch for P50">[details]</a> <a href='page.cgi?id=splinter.html&bug=105963&attachment=138744'>[review]</a> patch for P50 Review of <a href="attachment.cgi?id=138744" name="attach_138744" title="patch for P50">attachment 138744</a> <a href="attachment.cgi?id=138744&action=edit" title="patch for P50">[details]</a> <a href='page.cgi?id=splinter.html&bug=105963&attachment=138744'>[review]</a>: ----------------------------------------------------------------- Thanks, overall - I don't know why this patch works but the previous one didn't. Functionally they are (or should be) exactly the same on your touchpad. The (1 << 28) is a definite bug that needs to be fixed, please submit a separate patch for that. ::: src/evdev-mt-touchpad.c @@ +106,4 @@ > * is reset whenever a new finger is down, so we'd be resetting the > * speed and failing. > */ > + if (t->history.count < HISTORY_COUNT_THRESHOLD) { fwiw, I haven't used a const here because we only use it in one place. So having the number right here makes it more obvious what the value is. There's no strict rule for this, there is some gut feeling involved :) Same goes for the NON_MOTION_EVENT_COUNT @@ +435,4 @@ > > if (t->history.count <= 1) > return zero; > + this and a whole bunch of others look like detritus and erroneous whitespace being introduced. I recommend looking into git add -p, this saves you from adding spurious hunks. @@ +1411,4 @@ > reset that touch to non-dirty effectively swallowing that event > and restarting with the next event again. > */ > + if (tp->device->model_flags & EVDEV_MODEL_LENOVO_P50_TOUCHPAD) { this would break all the touchpads that currently rely on this setting. But I don't quite get why this is even needed, the flag name itself just doesn't matter. ::: src/evdev.h @@ -126,4 @@ > EVDEV_MODEL_LOGITECH_MARBLE_MOUSE = (1 << 26), > EVDEV_MODEL_TABLET_NO_PROXIMITY_OUT = (1 << 27), > EVDEV_MODEL_MS_NANO_TRANSCEIVER = (1 << 28), > - EVDEV_MODEL_TABLET_NO_TILT = (1 << 28), oh crap, that's a real bug (both flags having 1<<28). Can you split this into a separate commit please and I'll get it merged. ::: udev/90-libinput-model-quirks.hwdb @@ +220,5 @@ > + > +# Lenovo P50 > +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadP50* > + LIBINPUT_MODEL_LENOVO_P50_TOUCHPAD=1 > + LIBINPUT_ATTR_PALM_PRESSURE_THRESHOLD=150 I really don't understand why this could be needed. The udev properties you set here are the same as the ones with the previous patch (compare udevadm info /sys/class/input/eventX output to be sure). Even the palm threshold is the same, so all that happens here is a renaming of flags but the functional effect should be the same before and after and this patch is effectively a noop (for your touchpad, it breaks all the ones below :) ::: .gitignore @@ +16,5 @@ > +/help/ > +/.cproject > +/.project > +/tim_notes.txt > +.externalToolBuilders can't merge those, too specific. Please remove from the patch, thanks.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>