<table><tr><td style="">ngraham added a comment.
</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/D28152">View Revision</a></tr></table><br /><div><div><p>Wow, this is waaaaaay better. The workflow is really smooth. It's, like, incredibly great now. Also, the rules actually work for me now! So that's nice.</p>
<p>I really only have nitpicks:</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/D28152#inline-165091">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RuleItemDelegate.qml:63</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">id: itemEnabled</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">icon.name:</span> <span style="color: #766510">"list-remove"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">opacity:</span> <span class="p">(</span><span style="color: #004012">ruleDelegate</span><span class="p">.</span><span style="color: #004012">hovered</span> <span style="color: #aa2211">&&</span> <span style="color: #004012">model</span><span class="p">.</span><span style="color: #004012">selectable</span><span class="p">)</span> <span style="color: #aa2211">?</span> <span style="color: #601200">1</span> <span style="color: #aa2211">:</span> <span style="color: #601200">0</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">let's use <tt style="background: #ebebeb; font-size: 13px;">edit-delete</tt> for this, as that uses a trashcan icon and we're moving towards using that everywhere</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/D28152#inline-165092">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RulesEditor.qml:42</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">model:</span> <span style="color: #004012">enabledRulesModel</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">delegate:</span> <span style="color: #004012">RuleItemDelegate</span> <span class="p">{}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">section</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">the list items are back to changing their background on hover, which isn't 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/D28152#inline-165093">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RulesEditor.qml: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; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">Kirigami</span><span class="p">.</span><span style="color: #004012">Heading</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">level:</span> <span style="color: #601200">3</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Might be a good idea to move the "Add properties... button right below this text so it's obvious to the user now they add more properties. When the placeholder message is not shown, the button could just be there at the bottom of the list, and maybe could change its text to "Add more properties..."</p>
<p style="padding: 0; margin: 8px;">The reason for this is to make it easy for the user to understand what they need to do. We do this in the online accounts KCM: <a href="https://phabricator.kde.org/F8245459" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F8245459: Screenshot_20200418_112625.png</a></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/D28152#inline-165094">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RulesEditor.qml:73</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">footer:</span> <span style="color: #004012">RowLayout</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">QQC2</span><span class="p">.</span><span style="color: #004012">Button</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would make this footer a header to mimic the natural flow. Typically you'll click the "detectwindow properties" button first to target a window, which means it should be at the top of the window to suggest that it's the first thing you want to click on.</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/D28152#inline-165095">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RulesEditor.qml:75</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: #004012">QQC2</span><span class="p">.</span><span style="color: #004012">Button</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">text:</span> <span style="color: #004012">i18n</span><span class="p">(</span><span style="color: #766510">"Detect window properties"</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">icon.name:</span> <span style="color: #766510">"edit-find"</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Button titles use Title Case ("Detect Window Properties")</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/D28152#inline-165096">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RulesEditor.qml:96</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: #004012">QQC2</span><span class="p">.</span><span style="color: #004012">Button</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">text:</span> <span style="color: #004012">i18n</span><span class="p">(</span><span style="color: #766510">"Add properties"</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">icon.name:</span> <span style="color: #766510">"list-add-symbolic"</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">use Title Case and add ellipses ("Add Properties...")</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R108 KWin</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D28152">https://phabricator.kde.org/D28152</a></div></div><br /><div><strong>To: </strong>iasensio, Plasma, KWin, VDG<br /><strong>Cc: </strong>ngraham, davidedmundson, hchain, broulik, zzag, kwin, dmenig, manueljlin, Orage, cacarry, LeGast00n, The-Feren-OS-Dev, cblack, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, mkulinski, trickyricky26, ragreen, jackyalcine, iodelay, crozbo, ndavis, bwowk, ZrenBot, firef, alexeymin, skadinna, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, mbohlender, mart<br /></div>