<table><tr><td style="">romangg requested changes to this revision.<br />romangg added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D20186">View Revision</a></tr></table><br /><div><div><p>Looks really nice. I have yet to try it out, but following a code review.</p>
<p>In general it is good practice to make class member variables private and have protected getters/setters if child classes need to interact with them. For example <tt style="background: #ebebeb; font-size: 13px;">TouchpadBackend::m_mode</tt>. Other cases of that are all the properties in the child classes of LibinputCommon. Fixing that might be difficult though because of the template usage. And it might induce regressions in Wayland case, so I would wait with that for after next release. Some more ideas I wrote in the inline comments for that. But the m_mode thing can be changed already in this diff.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118615">View Inline</a><span style="color: #4b4d51; font-weight: bold;">libinputcommon.h:1</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"></span><span style="color: #74777d"><span class="bright">/*</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"></span><span style="color: #74777d"><span class="bright"> * Copyright 2017 Roman Gilg <subdiff@gmail.com></span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> *</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * This program is free software; you can redistribute it and/or modify</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * it under the terms of the GNU General Public License as published by</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * the Free Software Foundation; either version 2 of the License, or</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * (at your option) any later version.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> *</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * This program is distributed in the hope that it will be useful,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * but WITHOUT ANY WARRANTY; without even the implied warranty of</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * GNU General Public License for more details.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> *</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * You should have received a copy of the GNU General Public License</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * along with this program; if not, write to the Free Software</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#ifndef KWINWAYLANDTOUCHPAD_H</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#define KWINWAYLANDTOUCHPAD_H</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"></span><span style="color: #304a96"><span class="bright">#ifndef LIBINPUTCOMMON_H</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"></span><span style="color: #304a96"><span class="bright">#define LIBINPUTCOMMON_H</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Needs copyright notice (you should of course add yourself to the notice for files with non-marginal changes).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118618">View Inline</a><span style="color: #4b4d51; font-weight: bold;">libinputtouchpad.h:54</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">bool</span> <span style="color: #004012">supportsDisableEvents</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="n">override</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span class="n">m_supportsDisableEvents</span><span class="p">.</span><span class="n">avail</span> <span style="color: #aa2211">?</span> <span class="n">m_supportsDisableEvents</span><span class="p">.</span><span style="color: #a0a000">val</span> <span class="p">:</span> <span style="color: #304a96">false</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Equivalent and more concise is:<br />
<tt style="background: #ebebeb; font-size: 13px;">return m_supportsDisableEvents.avail && m_supportsDisableEvents.val ;</tt><br />
For other getters below as well.</p>
<p style="padding: 0; margin: 8px;">Long term try to not access the member variables from child class though. You could just have these functions in the parent class and just always check if avail is true before accessing the value. This will change the behavior on Wayland backend as well, but it should be fine (let's do this after release though to not risk regressions).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118620">View Inline</a><span style="color: #4b4d51; font-weight: bold;">libinputtouchpad.h:57</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">bool</span> <span style="color: #004012">isEnabled</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="n">override</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span style="color: #aa2211">!</span><span class="n">m_enabled</span><span class="p">.</span><span class="n">val</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why is there the negation? Put the Enabled-related function in the parent class.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118623">View Inline</a><span style="color: #4b4d51; font-weight: bold;">libinputtouchpad.h:65</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">bool</span> <span style="color: #004012">supportsLeftHanded</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="n">override</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span class="n">m_leftHanded</span><span class="p">.</span><span class="n">avail</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I get why you query the avail field of this and other properties in the X case and not the special property as on Wayland, but maybe there is a way to find a common approach for both?</p>
<p style="padding: 0; margin: 8px;">Anyways stuff for after the release. It's fine this way at the moment.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118625">View Inline</a><span style="color: #4b4d51; font-weight: bold;">xlibtouchpad.h:48</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Q_GADGET</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Necessary?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118626">View Inline</a><span style="color: #4b4d51; font-weight: bold;">touchpadconfiglibinput.h:35</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">explicit</span> <span class="n">TouchpadConfigLibinput</span><span class="p">(</span><span class="n">TouchpadConfigContainer</span> <span style="color: #aa2211">*</span><span class="n">parent</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">TouchpadBackend</span><span style="color: #aa2211">*</span> <span class="n">backend</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">const</span> <span class="n">QVariantList</span> <span style="color: #aa2211">&</span><span class="n">args</span> <span style="color: #aa2211">=</span> <span class="n">QVariantList</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Pointer asterisk goes to the right.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20186#inline-118627">View Inline</a><span style="color: #4b4d51; font-weight: bold;">touchpadconfigplugin.h:32</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">explicit</span> <span class="n">TouchpadConfigPlugin</span><span class="p">(</span><span class="n">QWidget</span> <span style="color: #aa2211">*</span><span class="n">parent</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">explicit</span> <span class="n">TouchpadConfigPlugin</span><span class="p">(</span><span class="n">QWidget</span> <span style="color: #aa2211">*</span><span class="n">parent<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">TouchpadBackend</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span class="n"><span class="bright">backend</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">virtual</span> <span style="color: #aa2211">~</span><span class="n">TouchpadConfigPlugin</span><span class="p">()</span> <span class="p">{}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No need for explicit anymore.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R119 Plasma Desktop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D20186">https://phabricator.kde.org/D20186</a></div></div><br /><div><strong>To: </strong>atulbi, ngraham, romangg, davidedmundson, Plasma<br /><strong>Cc: </strong>GB_2, jriddell, knambiar, plasma-devel, jraleigh, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>