Attachment 138745 Details for Bug 105963 – email garbage

email garbage

attachment-23212-0.html (text/html), 6.82 KB, created by Tim Richardson on 2018-04-11 01:20:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tim Richardson

Created: 2018-04-11 01:20:49 UTC

Size: 6.82 KB

patch

obsolete

><div dir="ltr">Thanks, I will take those actions, including learning a bit more about git. I will also see if I over-reacted by making a separate udev rule for the P50. Before I split it, the touchpad became unusable after a reboot; I assumed that other quirks somewhere were being applied which were causing the trouble (so my hypothesis is not that the udev properties need to be different, but that somewhere other quirks were being activated because a bunch of different ThinkPads were grouped together). However, these problems may have been due to some of my other exploratory changes (since backed out since none of them helped). You&#39;re right: the only active change is the rejection of non-touch events which you already coded, plus the use of a dedicated P50 rule (I thought you created that for the P50 bug report which is why I wasn&#39;t concerned about a specific P50 rule excluding other hardware from the non-touch event rejection).<div><br></div><div>The touchpad is still a poor experience at slow acceleration settings (&lt;-0.7) but it is considerably improved at xinput acceleration settings &gt;0.5 (previously close to unusable, but not any more, it&#39;s actually pleasant to use at last).Â </div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 11 April 2018 at 10:42,  <span dir="ltr">&lt;<a href="mailto:bugzilla-daemon@freedesktop.org" target="_blank">bugzilla-daemon@freedesktop.org</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>    
>      
>    
>    <div>
>      <p>
>        </p><div>
>            <b><a class="m_-4496730228206097942bz_bug_link m_-4496730228206097942bz_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" target="_blank">Comment # 10</a>
>              on <a class="m_-4496730228206097942bz_bug_link m_-4496730228206097942bz_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" target="_blank">bug 105963</a>
>              from <span class="m_-4496730228206097942vcard"><a class="m_-4496730228206097942email" href="mailto:peter.hutterer@who-t.net" title="Peter Hutterer &lt;peter.hutterer@who-t.net&gt;" target="_blank"> <span class="m_-4496730228206097942fn">Peter Hutterer</span></a>
></span></b>
>        <pre>Comment on <span><a href="https://bugs.freedesktop.org/attachment.cgi?id=138744" name="m_-4496730228206097942_attach_138744" title="patch for P50" target="_blank">attachment 138744</a> <a href="https://bugs.freedesktop.org/attachment.cgi?id=138744&amp;action=edit" title="patch for P50" target="_blank">[details]</a></span> <a href="https://bugs.freedesktop.org/page.cgi?id=splinter.html&amp;bug=105963&amp;attachment=138744" target="_blank">[review]</a>
>patch for P50
>
>Review of <span><a href="https://bugs.freedesktop.org/attachment.cgi?id=138744" name="m_-4496730228206097942_attach_138744" title="patch for P50" target="_blank">attachment 138744</a> <a href="https://bugs.freedesktop.org/attachment.cgi?id=138744&amp;action=edit" title="patch for P50" target="_blank">[details]</a></span> <a href="https://bugs.freedesktop.org/page.cgi?id=splinter.html&amp;bug=105963&amp;attachment=138744" target="_blank">[review]</a>:
>------------------------------<wbr>------------------------------<wbr>-----
>
>Thanks, overall - I don&#39;t know why this patch works but the previous one
>didn&#39;t. Functionally they are (or should be) exactly the same on your touchpad.
>
>The (1 &lt;&lt; 28) is a definite bug that needs to be fixed, please submit a
>separate patch for that.
>
>::: src/evdev-mt-touchpad.c
>@@ +106,4 @@
><span class="m_-4496730228206097942quote">&gt;  	 * is reset whenever a new finger is down, so we&#39;d be resetting the
>&gt;  	 * speed and failing.
>&gt;  	 */
>&gt; +	if (t-&gt;history.count &lt; HISTORY_COUNT_THRESHOLD) {</span>
>
>fwiw, I haven&#39;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&#39;s
>no strict rule for this, there is some gut feeling involved :)
>
>Same goes for the NON_MOTION_EVENT_COUNT
>
>@@ +435,4 @@
><span class="m_-4496730228206097942quote">&gt;  
>&gt;  	if (t-&gt;history.count &lt;= 1)
>&gt;  		return zero;
>&gt; +		</span>
>
>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 @@
><span class="m_-4496730228206097942quote">&gt;  	   reset that touch to non-dirty effectively swallowing that event
>&gt;  	   and restarting with the next event again.
>&gt;  	 */
>&gt; +	if (tp-&gt;device-&gt;model_flags &amp; EVDEV_MODEL_LENOVO_P50_<wbr>TOUCHPAD) {</span>
>
>this would break all the touchpads that currently rely on this setting. But I
>don&#39;t quite get why this is even needed, the flag name itself just doesn&#39;t
>matter.
>
>::: src/evdev.h
>@@ -126,4 @@
><span class="m_-4496730228206097942quote">&gt;  	EVDEV_MODEL_LOGITECH_MARBLE_<wbr>MOUSE = (1 &lt;&lt; 26),
>&gt;  	EVDEV_MODEL_TABLET_NO_<wbr>PROXIMITY_OUT = (1 &lt;&lt; 27),
>&gt;  	EVDEV_MODEL_MS_NANO_<wbr>TRANSCEIVER = (1 &lt;&lt; 28),
>&gt; -	EVDEV_MODEL_TABLET_NO_TILT = (1 &lt;&lt; 28),</span>
>
>oh crap, that&#39;s a real bug (both flags having 1&lt;&lt;28). Can you split this into a
>separate commit please and I&#39;ll get it merged.
>
>::: udev/90-libinput-model-quirks.<wbr>hwdb
>@@ +220,5 @@
><span class="m_-4496730228206097942quote">&gt; + 
>&gt; +# Lenovo P50
>&gt; +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:<wbr>pvrThinkPadP50*
>&gt; + LIBINPUT_MODEL_LENOVO_P50_<wbr>TOUCHPAD=1
>&gt; + LIBINPUT_ATTR_PALM_PRESSURE_<wbr>THRESHOLD=150</span>
>
>I really don&#39;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 @@
><span class="m_-4496730228206097942quote">&gt; +/help/
>&gt; +/.cproject
>&gt; +/.project
>&gt; +/tim_notes.txt
>&gt; +.externalToolBuilders</span>
>
>can&#39;t merge those, too specific. Please remove from the patch, thanks.</pre>
>        </div>
>      <p></p><span class="">
>
>
>      <hr>
>      <span>You are receiving this mail because:</span>
>
>      <ul>
>          <li>You reported the bug.</li>
>      </ul>
>    </span></div>
></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Tim Richardson<br></div>
></div>

<div dir="ltr">Thanks, I will take those actions, including learning a bit more about git. I will also see if I over-reacted by making a separate udev rule for the P50. Before I split it, the touchpad became unusable after a reboot; I assumed that other quirks somewhere were being applied which were causing the trouble (so my hypothesis is not that the udev properties need to be different, but that somewhere other quirks were being activated because a bunch of different ThinkPads were grouped together). However, these problems may have been due to some of my other exploratory changes (since backed out since none of them helped). You&#39;re right: the only active change is the rejection of non-touch events which you already coded, plus the use of a dedicated P50 rule (I thought you created that for the P50 bug report which is why I wasn&#39;t concerned about a specific P50 rule excluding other hardware from the non-touch event rejection).<div><br></div><div>The touchpad is still a poor experience at slow acceleration settings (&lt;-0.7) but it is considerably improved at xinput acceleration settings &gt;0.5 (previously close to unusable, but not any more, it&#39;s actually pleasant to use at last).Â </div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 11 April 2018 at 10:42,  <span dir="ltr">&lt;<a href="mailto:bugzilla-daemon@freedesktop.org" target="_blank">bugzilla-daemon@freedesktop.org</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
    
      
    
    <div>
      <p>
        </p><div>
            <b><a class="m_-4496730228206097942bz_bug_link m_-4496730228206097942bz_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" target="_blank">Comment # 10</a>
              on <a class="m_-4496730228206097942bz_bug_link m_-4496730228206097942bz_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" target="_blank">bug 105963</a>
              from <span class="m_-4496730228206097942vcard"><a class="m_-4496730228206097942email" href="mailto:peter.hutterer@who-t.net" title="Peter Hutterer &lt;peter.hutterer@who-t.net&gt;" target="_blank"> <span class="m_-4496730228206097942fn">Peter Hutterer</span></a>
</span></b>
        <pre>Comment on <span><a href="https://bugs.freedesktop.org/attachment.cgi?id=138744" name="m_-4496730228206097942_attach_138744" title="patch for P50" target="_blank">attachment 138744</a> <a href="https://bugs.freedesktop.org/attachment.cgi?id=138744&amp;action=edit" title="patch for P50" target="_blank">[details]</a></span> <a href="https://bugs.freedesktop.org/page.cgi?id=splinter.html&amp;bug=105963&amp;attachment=138744" target="_blank">[review]</a>
patch for P50

Review of <span><a href="https://bugs.freedesktop.org/attachment.cgi?id=138744" name="m_-4496730228206097942_attach_138744" title="patch for P50" target="_blank">attachment 138744</a> <a href="https://bugs.freedesktop.org/attachment.cgi?id=138744&amp;action=edit" title="patch for P50" target="_blank">[details]</a></span> <a href="https://bugs.freedesktop.org/page.cgi?id=splinter.html&amp;bug=105963&amp;attachment=138744" target="_blank">[review]</a>:
------------------------------<wbr>------------------------------<wbr>-----

Thanks, overall - I don&#39;t know why this patch works but the previous one
didn&#39;t. Functionally they are (or should be) exactly the same on your touchpad.

The (1 &lt;&lt; 28) is a definite bug that needs to be fixed, please submit a
separate patch for that.

::: src/evdev-mt-touchpad.c
@@ +106,4 @@
<span class="m_-4496730228206097942quote">&gt;  	 * is reset whenever a new finger is down, so we&#39;d be resetting the
&gt;  	 * speed and failing.
&gt;  	 */
&gt; +	if (t-&gt;history.count &lt; HISTORY_COUNT_THRESHOLD) {</span>

fwiw, I haven&#39;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&#39;s
no strict rule for this, there is some gut feeling involved :)

Same goes for the NON_MOTION_EVENT_COUNT

@@ +435,4 @@
<span class="m_-4496730228206097942quote">&gt;  
&gt;  	if (t-&gt;history.count &lt;= 1)
&gt;  		return zero;
&gt; +		</span>

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 @@
<span class="m_-4496730228206097942quote">&gt;  	   reset that touch to non-dirty effectively swallowing that event
&gt;  	   and restarting with the next event again.
&gt;  	 */
&gt; +	if (tp-&gt;device-&gt;model_flags &amp; EVDEV_MODEL_LENOVO_P50_<wbr>TOUCHPAD) {</span>

this would break all the touchpads that currently rely on this setting. But I
don&#39;t quite get why this is even needed, the flag name itself just doesn&#39;t
matter.

::: src/evdev.h
@@ -126,4 @@
<span class="m_-4496730228206097942quote">&gt;  	EVDEV_MODEL_LOGITECH_MARBLE_<wbr>MOUSE = (1 &lt;&lt; 26),
&gt;  	EVDEV_MODEL_TABLET_NO_<wbr>PROXIMITY_OUT = (1 &lt;&lt; 27),
&gt;  	EVDEV_MODEL_MS_NANO_<wbr>TRANSCEIVER = (1 &lt;&lt; 28),
&gt; -	EVDEV_MODEL_TABLET_NO_TILT = (1 &lt;&lt; 28),</span>

oh crap, that&#39;s a real bug (both flags having 1&lt;&lt;28). Can you split this into a
separate commit please and I&#39;ll get it merged.

::: udev/90-libinput-model-quirks.<wbr>hwdb
@@ +220,5 @@
<span class="m_-4496730228206097942quote">&gt; + 
&gt; +# Lenovo P50
&gt; +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:<wbr>pvrThinkPadP50*
&gt; + LIBINPUT_MODEL_LENOVO_P50_<wbr>TOUCHPAD=1
&gt; + LIBINPUT_ATTR_PALM_PRESSURE_<wbr>THRESHOLD=150</span>

I really don&#39;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 @@
<span class="m_-4496730228206097942quote">&gt; +/help/
&gt; +/.cproject
&gt; +/.project
&gt; +/tim_notes.txt
&gt; +.externalToolBuilders</span>

can&#39;t merge those, too specific. Please remove from the patch, thanks.</pre>
        </div>
      <p></p><span class="">


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You reported the bug.</li>
      </ul>
    </span></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Tim Richardson<br></div>
</div>

Actions: View

Attachments on bug 105963: 138720 | 138744 | 138745 | 138751 | 138755 | 138757

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.