<table><tr><td style="">rkflx requested changes to this revision.<br />rkflx 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/D13450">View Revision</a></tr></table><br /><div><div><p><a href="https://phabricator.kde.org/p/sharvey/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@sharvey</a> Thanks for the updates. As always, first I'd like to iterate to get the patch working properly, and then I'll look at the code in detail. As I'm now put under pressure again to squeeze the patch into 18.08, we'll see how it goes (in general rushing leads to bugs, see last Plasma release…). First of all:</p>
<p>I still disagree regarding the default speed selection. We determined by looking at other apps that <kbd title="Shift" style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">⇧</kbd> is the modifier to use, and I argued (in line with what I meant when triaging the bug) that for making the rectangle feature useful for keyboard users by default the movement should be fast. BTW, this is also what KWin is doing, and I see no reason at all why Spectacle should deviate from that standard. (See above for even more arguments.) I don't feel comfortable approving the current default, sorry.</p>
<p>Apart from that, the patch now works well for the most part, good job (and thanks to <a href="https://phabricator.kde.org/p/abalaji/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@abalaji</a> for commenting on some things). In addition to some inline comments below, I found those issues:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">When the rectangle covers the whole screen, I'm unable to resize, i.e. make the rectangle smaller again.</li>
<li class="remarkup-list-item">I'm unable to fine-tune both size and position with HiDPI scaling active (see also inline comment for the likely cause).</li>
<li class="remarkup-list-item">Selecting with the mouse sets a minimum size, however resizing with the arrow keys allows to resize to 0x0px.</li>
</ul>
<hr class="remarkup-hr" />
<p>To summarize, here's what I propose: If we can reach agreement on the default and you are fast with the string changes still required, the patch can go in for the Beta. Then you have time to work on the other points until the RC (at which point we have to decide whether to ship or to revert again, based on your progress).</p>
<hr class="remarkup-hr" />
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D13450#283620" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#283620</a>, <a href="https://phabricator.kde.org/p/abalaji/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@abalaji</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>A few thing tho <a href="https://phabricator.kde.org/p/sharvey/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@sharvey</a>, it looks like currently the resize functionality only moves the bottom right corner, and I've preserved that. But was wondering if we can add in Ctrl or something to control that. Maybe something like:</p>
<p>When Alt is pressed down:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">We are in "expand" mode by default. Pressing left expands selection on the left by moving the left border outwards to the left, etc.</li>
<li class="remarkup-list-item">Also holding Ctrl enables a "shrink" mode. Now pressing left shrinks the selection from the right by moving the right border inwards, as if we are nudging it from the right towards the left, etc.</li>
</ul></div>
</blockquote>
<p>I think that's too complicated, and you still need two steps to get to the final selection (just like setting one corner by moving and the other corner by resizing are two steps too).</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/D13450#inline-74709">View Inline</a><span style="color: #4b4d51; font-weight: bold;">index.docbook:154</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);"> <para>You can use the arrow keys to move and adjust the rectangle. Pressing the arrow keys will slowly move the rectangle. Holding the <keycap>Shift</keycap> while using the arrow keys will move the rectangle more quickly. Holding the <keycap>Alt</keycap> key while using the arrows will adjust the size of the rectangle.</para>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"Holding the Shift while": Either remove "the", or add "key".</p>
<p style="padding: 0; margin: 8px;">"using the arrows": Change to "Using the arrow keys"?</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/D13450#inline-74712">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:41</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: #004012">property</span> <span style="color: #304a96">int</span> <span style="color: #aa4000">magOffset:</span> <span style="color: #601200">32</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">property</span> <span style="color: #304a96">int</span> <span style="color: #aa4000">largeChange:</span> <span style="color: #601200">15</span> <span style="color: #aa2211">/</span> <span style="color: #004012">Screen</span><span class="p">.</span><span style="color: #004012">devicePixelRatio</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">property</span> <span style="color: #304a96">int</span> <span style="color: #aa4000">smallChange:</span> <span style="color: #601200">1</span> <span style="color: #aa2211">/</span> <span style="color: #004012">Screen</span><span class="p">.</span><span style="color: #004012">devicePixelRatio</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That does not seem right, because this will result in slower movement on a HiDPI screen compared to a regular screen of the same size. (It's needed for the 1px case, of course.)</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/D13450#inline-74715">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.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: #004012">property</span> <span style="color: #304a96">int</span> <span style="color: #aa4000">largeChange:</span> <span style="color: #601200">15</span> <span style="color: #aa2211">/</span> <span style="color: #004012">Screen</span><span class="p">.</span><span style="color: #004012">devicePixelRatio</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">property</span> <span style="color: #304a96">int</span> <span style="color: #aa4000">smallChange:</span> <span style="color: #601200">1</span> <span style="color: #aa2211">/</span> <span style="color: #004012">Screen</span><span class="p">.</span><span style="color: #004012">devicePixelRatio</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That looks suspicious: How will you be able to store the result accurately in an <tt style="background: #ebebeb; font-size: 13px;">int</tt>?</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/D13450#inline-74711">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:54-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(251, 175, 175, .7);"> <span style="color: #766510">"x"</span><span style="color: #aa2211">:</span> <span style="color: #004012">xx</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #766510">"y"</span><span style="color: #aa2211">:</span> <span style="color: #004012">yy</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #766510">"height"</span><span style="color: #aa2211">:</span> <span style="color: #004012">hh</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #766510">"width"</span><span style="color: #aa2211">:</span> <span style="color: #004012">ww</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"x"</span><span style="color: #aa2211">:</span> <span class="bright"> </span><span style="color: #004012">xx</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"y"</span><span style="color: #aa2211">:</span> <span class="bright"> </span><span style="color: #004012">yy</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"height"</span><span style="color: #aa2211">:</span> <span class="bright"> </span><span style="color: #004012">hh</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"width"</span><span style="color: #aa2211">:</span> <span class="bright"> </span><span style="color: #004012">ww</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unrelated whitespace change, please revert.</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/D13450#inline-74728">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:84</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">var</span> <span style="color: #004012">delta</span><span class="p">;</span> <span style="color: #74777d">// valid for either direction</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Without the context of the patch in mind, this is a bit hard to understand what it is about when you stumble upon this while reading the code. Could you come up with a better name and/or comment?</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/D13450#inline-74726">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:92</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: #74777d">// nested switches for arrow keys based on modifier keys</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">switch</span><span class="p">(</span><span style="color: #004012">event</span><span class="p">.</span><span style="color: #004012">modifiers</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;">Insert space after <tt style="background: #ebebeb; font-size: 13px;">switch</tt>, please. Repeat for all other occurrences.</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/D13450#inline-74734">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:98</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">delta</span> <span style="color: #aa2211">=</span> <span style="color: #004012">checkBounds</span><span class="p">(</span><span style="color: #aa2211">-</span><span style="color: #004012">smallChange</span><span class="p">,</span> <span style="color: #601200">0</span><span class="p">,</span> <span style="color: #766510">"left"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">selection</span><span class="p">.</span><span style="color: #004012">x</span> <span style="color: #aa2211">=</span> <span style="color: #004012">selection</span><span class="p">.</span><span style="color: #004012">x</span> <span style="color: #aa2211">+</span> <span style="color: #004012">delta</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">break</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Do you know about the <tt style="background: #ebebeb; font-size: 13px;">+=</tt> operator, which would be more concise?</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/D13450#inline-74731">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:116-137</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">case</span> <span style="color: #aa4000">Qt.ShiftModifier:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">switch</span><span class="p">(</span><span style="color: #004012">event</span><span class="p">.</span><span style="color: #004012">key</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">case</span> <span style="color: #aa4000">Qt.Key_Left:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">delta</span> <span style="color: #aa2211">=</span> <span style="color: #004012">checkBounds</span><span class="p">(</span><span style="color: #aa2211">-</span><span style="color: #004012">largeChange</span><span class="p">,</span> <span style="color: #601200">0</span><span class="p">,</span> <span style="color: #766510">"left"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">selection</span><span class="p">.</span><span style="color: #004012">x</span> <span style="color: #aa2211">=</span> <span style="color: #004012">selection</span><span class="p">.</span><span style="color: #004012">x</span> <span style="color: #aa2211">+</span> <span style="color: #004012">delta</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">break</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This duplicates most of the code for <tt style="background: #ebebeb; font-size: 13px;">Qt.NoModifier</tt>. Could you instead assign <tt style="background: #ebebeb; font-size: 13px;">largeChange</tt> or <tt style="background: #ebebeb; font-size: 13px;">smallChange</tt> to a variable based on the modifier, which you then pass to <tt style="background: #ebebeb; font-size: 13px;">checkBounds</tt>?</p>
<p style="padding: 0; margin: 8px;">Also this is likely independent from whether you are moving or resizing (where all of this is duplicated again currently).</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/D13450#inline-74730">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:173</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">case</span><span class="p">(</span><span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">ShiftModifier</span> <span style="color: #aa2211">+</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">AltModifier</span><span class="p">)</span><span style="color: #aa2211">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">switch</span><span class="p">(</span><span style="color: #004012">event</span><span class="p">.</span><span style="color: #004012">key</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;">Insert space after <tt style="background: #ebebeb; font-size: 13px;">case</tt>.</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/D13450#inline-74732">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:210</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">function</span> <span style="color: #004012">checkBounds</span><span class="p">(</span><span style="color: #004012">deltaX</span><span class="p">,</span> <span style="color: #004012">deltaY</span><span class="p">,</span> <span style="color: #004012">direction</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;">As the mapping between <tt style="background: #ebebeb; font-size: 13px;">Qt.Key_*</tt> and <tt style="background: #ebebeb; font-size: 13px;">"*"</tt> is static, for <tt style="background: #ebebeb; font-size: 13px;">direction</tt> you could pass the key directly instead of creating an extra string.</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/D13450#inline-74729">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:257</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 class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">case</span> <span style="color: #766510">"down"</span><span style="color: #aa2211">:</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Missing <tt style="background: #ebebeb; font-size: 13px;">break</tt> (not that it would matter with those <tt style="background: #ebebeb; font-size: 13px;">returns</tt>, but there should be at least a consistent style).</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/D13450#inline-74725">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:371</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(251, 175, 175, .7);">
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unrelated whitespace change.</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/D13450#inline-74722">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:443-507</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 style="color: #aa4000">height:</span> <span style="color: #004012">midHelpTextElement</span><span class="p">.</span><span style="color: #004012">height</span> <span style="color: #aa2211">+</span> <span class="bright"></span><span style="color: #601200"><span class="bright">4</span>0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">width:</span> <span style="color: #004012">midHelpTextElement</span><span class="p">.</span><span style="color: #004012">width</span> <span style="color: #aa2211">+</span> <span class="bright"></span><span style="color: #601200"><span class="bright">4</span>0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">height:</span> <span style="color: #004012">midHelpTextElement</span><span class="p">.</span><span style="color: #004012">height</span> <span style="color: #aa2211">+</span> <span class="bright"></span><span style="color: #601200"><span class="bright">2</span>0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">width:</span> <span style="color: #004012">midHelpTextElement</span><span class="p">.</span><span style="color: #004012">width</span> <span style="color: #aa2211">+</span> <span class="bright"></span><span style="color: #601200"><span class="bright">2</span>0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">radius:</span> <span style="color: #601200">4</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">border.color:</span> <span style="color: #004012">systemPalette</span><span class="p">.</span><span style="color: #004012">windowText</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">color:</span> <span style="color: #004012">labelBackgroundColour</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">visible:</span> <span style="color: #000a65">false</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Currently I don't understand what all those changes are for and how they relate to the topic of the patch (and I would disagree with some).</p>
<p style="padding: 0; margin: 8px;">Contrary to <a href="https://phabricator.kde.org/p/ngraham/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@ngraham</a> I'd say this actually is a visual regression.</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/D13450#inline-74721">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:512</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">Label</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">"Arrow Keys:"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">Layout.alignment:</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">AlignLeft</span> <span style="color: #aa2211">&&</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">AlignTop</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Would it make sense to use the same capitalization as above, i.e. "keys" instead of "Keys"?</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/D13450#inline-74724">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:513</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">text:</span> <span style="color: #004012">i18n</span><span class="p">(</span><span style="color: #766510">"Arrow Keys:"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">Layout.alignment:</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">AlignLeft</span> <span style="color: #aa2211">&&</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">AlignTop</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;">Do you mean <tt style="background: #ebebeb; font-size: 13px;">|</tt> instead of <tt style="background: #ebebeb; font-size: 13px;">&&</tt> by any chance?</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/D13450#inline-74719">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:516</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">Label</span> <span class="p">{</span> <span style="color: #aa4000">text:</span> <span style="color: #004012">i18n</span><span class="p">(</span><span style="color: #766510">"Move selection rectangle \n"</span> <span style="color: #aa2211">+</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"Alt to resize, Shift for %1px change"</span><span class="p">,</span> <span style="color: #004012">largeChange</span><span class="p">);</span> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would suggest to keep "Esc" at the bottom, to make it easier to find. "Arrow keys" could go after "Shift", so "Right-click" would still be next to "Esc" to which it belongs.</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/D13450#inline-74720">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:516</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">Label</span> <span class="p">{</span> <span style="color: #aa4000">text:</span> <span style="color: #004012">i18n</span><span class="p">(</span><span style="color: #766510">"Move selection rectangle \n"</span> <span style="color: #aa2211">+</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"Alt to resize, Shift for %1px change"</span><span class="p">,</span> <span style="color: #004012">largeChange</span><span class="p">);</span> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"15px change" does sound a bit strange: What is "change", and why do "15px" matter? Let me repeat my suggestion from above:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">Hold Alt to resize, fine-tune with Shift</pre></div></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R166 Spectacle</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D13450">https://phabricator.kde.org/D13450</a></div></div><br /><div><strong>To: </strong>sharvey, rkflx, ngraham, Spectacle, yurchor<br /><strong>Cc: </strong>ltoscano, kde-doc-english, abalaji, Spectacle, skadinna<br /></div>