<table><tr><td style="">rkflx 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/D13450">View Revision</a></tr></table><br /><div><div><p>Thanks for the updates. Found one more problem, the other changes are fine.</p>
<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#294047" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#294047</a>, <a href="https://phabricator.kde.org/p/rkflx/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rkflx</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><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>
</ul></div>
</blockquote>
<p>Note that this problem is more general: Once an edge of the selection touches the border of the screen, decreasing the size in that direction is blocked, only increasing the size works.</p>
<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#294068" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#294068</a>, <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> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><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#294047" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#294047</a>, <a href="https://phabricator.kde.org/p/rkflx/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rkflx</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><ul class="remarkup-list">
<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></div>
</blockquote>
<p>Where should it stop? 1x1?</p></div>
</blockquote>
<p>It should be consistent with how it works for selecting with the mouse (grep for <tt style="background: #ebebeb; font-size: 13px;">minRectSize</tt>).</p>
<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#294279" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#294279</a>, <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> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><ul class="remarkup-list">
<li class="remarkup-list-item">Change default adjustment to <tt style="background: #ebebeb; font-size: 13px;">largeChange</tt>, <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> uses <tt style="background: #ebebeb; font-size: 13px;">smallChange</tt> for fine tuning</li>
</ul></div>
</blockquote>
<p>Thanks, appreciated!</p>
<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#294291" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#294291</a>, <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> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I'm noticing that the magnifier doesn't er show up while you're using the keyboard to resize the selection rect. Might wanna hook that up.</p></div>
</blockquote>
<p>Yeah, I also had that idea a couple of weeks back, but decided to not bring it up in the review, because for conceptual reasons I doubt that's so easy to implement like you make it sound: The magnifier shows up at the cursor position, which for the mouse case is either at one of the corners, or at an arbitrary position along one of the edges. Since with the arrow keys you can only control horizontal/vertical movement of the edges (this is true both for resizing and moving), there is no well-defined corner, i.e. it does not make any sense to show a square magnifier at a random position on the edge. You'd have to show a magnifier covering the complete width/height of the respective edge, because you cannot know where the user is looking at and which part of the screenshot he wants to align the selection to (it might not be the corner).</p>
<p>One way forward could be to not only show the magnifier when resizing (i.e. clicking on a handle) as it is implemented currently, but in addition show the magnifier directly attached to the cursor as soon as <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Shift</span></span></span> is hold down, e.g. more like KWin's magnifying glass. However, this has a couple of drawbacks: In case you checked the checkbox to show the magnifier by default, <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Shift</span></span></span> will confusingly trigger hiding the magnifier for clicking, but trigger showing when not clicking (because without any click or modifier the magnifier should never be shown at all). Next, for resizing by keyboard you'd have to first move/resize the selection rectangle, and then reach for the mouse to position the magnifier where you want it, and iterate as needed. That's pretty awkward, if you ask me.</p>
<p><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> Feel free to send a follow-up patch if you can get it working in a non-confusing way and have a convincing answer to the question where to show the magnifier (or rather what to magnify).</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#294305" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D13450#294305</a>, <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> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>You suggested multiple changes to the <tt style="background: #ebebeb; font-size: 13px;">checkBounds</tt> function, all of which I understand and agree with. This function caused me the most heartburn and I feel uncomfortable attempting to rework it while under a time crunch. Unfortunately, this leaves the <tt style="background: #ebebeb; font-size: 13px;">0x0px</tt> resize and the "cannot resize when expanded to full screen` bugs unresolved.</p></div>
</blockquote>
<p>Indeed, those are not critical and can be worked on for the RC (although that's a huge responsibility, as normally you are not supposed to ship broken stuff where the fix is only promised for the future…).</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>(Again, thank you for the code optimization suggestions. They are appreciated.)</p></blockquote>
<p>Glad you find them useful. It's not always easy to find the correct abstractions to get concise, readable and maintainable code, but practice makes perfect ;)</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-74850">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:169</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.Key_Down:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">change</span> <span style="color: #aa2211">=</span> <span style="color: #004012">checkBounds</span><span class="p">(</span><span style="color: #601200">0.0</span><span class="p">,</span> <span style="color: #004012">smallChange</span><span class="p">,</span> <span style="color: #766510">"down"</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">height</span> <span style="color: #aa2211">=</span> <span style="color: #004012">selection</span><span class="p">.</span><span style="color: #004012">height</span> <span style="color: #aa2211">+</span> <span style="color: #004012">change</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><kbd 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;">Alt</kbd><span class="kbd-join" style="padding: 0 4px; color: #92969D;">+</span><kbd title="Down" 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 broken (should be fast, not slow)</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;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:92</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not done, but maybe I should have given you an example. This is how I meant it:</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);">switch (event.modifiers) {</pre></div>
<p style="padding: 0; margin: 8px;">IOW, this should be consistent with how the spacing works for <tt style="background: #ebebeb; font-size: 13px;">if</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-74730">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:173</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Insert space after <tt style="background: #ebebeb; font-size: 13px;">case</tt>.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not yet done.</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;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:443-507</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not done. While you fixed the alignment, there are still a couple of extra changes (lines 448 to 490, line 525). You might want to look at the Diff again, either locally or in Phab.</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;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">EditorRoot.qml:516</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">"Arrow keys" could go after "Shift"</p></blockquote>
<p style="padding: 0; margin: 8px;">Not done. Why not move the new entry up one more line? IIRC I specifically put "Reset" next to "Cancel" when reorganizing this recently.</p></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>