rkflx added a comment.
Thanks for the updates. Found one more problem, the other changes are fine. In D13450#294047 <https://phabricator.kde.org/D13450#294047>, @rkflx wrote: > - When the rectangle covers the whole screen, I'm unable to resize, i.e. make the rectangle smaller again. 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. In D13450#294068 <https://phabricator.kde.org/D13450#294068>, @sharvey wrote: > In D13450#294047 <https://phabricator.kde.org/D13450#294047>, @rkflx wrote: > > > - Selecting with the mouse sets a minimum size, however resizing with the arrow keys allows to resize to 0x0px. > > > Where should it stop? 1x1? It should be consistent with how it works for selecting with the mouse (grep for `minRectSize`). In D13450#294279 <https://phabricator.kde.org/D13450#294279>, @sharvey wrote: > - Change default adjustment to `largeChange`, [⇧] uses `smallChange` for fine tuning Thanks, appreciated! In D13450#294291 <https://phabricator.kde.org/D13450#294291>, @ngraham wrote: > 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. 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). 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 Shift 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, Shift 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. @ngraham 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). --- In D13450#294305 <https://phabricator.kde.org/D13450#294305>, @sharvey wrote: > You suggested multiple changes to the `checkBounds` 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 `0x0px` resize and the "cannot resize when expanded to full screen` bugs unresolved. 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…). > (Again, thank you for the code optimization suggestions. They are appreciated.) 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 ;) INLINE COMMENTS > EditorRoot.qml:169 > + case Qt.Key_Down: > + change = checkBounds(0.0, smallChange, "down"); > + selection.height = selection.height + change; [Alt] + [↓] is broken (should be fast, not slow) > rkflx wrote in EditorRoot.qml:92 > Insert space after `switch`, please. Repeat for all other occurrences. Not done, but maybe I should have given you an example. This is how I meant it: switch (event.modifiers) { IOW, this should be consistent with how the spacing works for `if`. > rkflx wrote in EditorRoot.qml:173 > Insert space after `case`. Not yet done. > rkflx wrote in EditorRoot.qml:443-507 > 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). > > Contrary to @ngraham I'd say this actually is a visual regression. 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. > rkflx wrote in EditorRoot.qml:516 > 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. > "Arrow keys" could go after "Shift" Not done. Why not move the new entry up one more line? IIRC I specifically put "Reset" next to "Cancel" when reorganizing this recently. REPOSITORY R166 Spectacle REVISION DETAIL https://phabricator.kde.org/D13450 To: sharvey, rkflx, ngraham, #spectacle, yurchor Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna
