Add arrow keys to move and resize selection rectangle
ClosedPublic

Authored by sharvey on Jun 9 2018, 9:45 PM.

Details

Summary

Add arrow keys to move and resize selection rectangle.
Arrows alone moves in large increment
+ Arrows moves in small (single pixel) increments
ALT + Arrows resize rectangle in large increment
ALT + + Arrows resize rectangle in small increment

BUG: 394947

Test Plan
  • Apply patch
    • Launch Spectacle; begin Rectangular Selection
    • Select a rectangle with the mouse
    • Use Arrows to move rectangle (large increment)
    • Use + Arrows to move in small increment
    • Use ALT + Arrows to resize rectangle (large increment
    • Use ALT + + Arrows to resize rectangle in small increment
    • Complete capture with mouse or Enter key
    • Ensure right selection is captured after moving and resizing rectangle

Diff Detail

Repository
R166 Spectacle
Branch
rectangle-arrows (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
abalaji added inline comments.Jun 27 2018, 4:18 PM
src/QuickEditor/EditorRoot.qml
121

I think you get the idea

sharvey updated this revision to Diff 37903.Jul 16 2018, 7:58 PM
  • Change movement sizes to handle HiDPI screens
sharvey marked 6 inline comments as done.Jul 16 2018, 7:59 PM

I apologize for the delay in getting this finalized. It's been a tumultuous month in real life. Including emergency heart surgery for my father, a flare-up of my own illnesses (I'm on disability), and finally - a kernel upgrade that somehow failed, rendering my machine unbootable. Lots of reinstalling and reconfiguring.

That's enough complaining from me. I just want to let everyone know I hadn't abandoned the patch. Real life intrusions are the worst.

Please let me know if there's more we think needs to be done.

Sorry to hear that, and I hope things get easier.

src/QuickEditor/EditorRoot.qml
409

Let's change the first line to just "Move selection rectangle", since that's all it does without a modifier key. The next line that introduces the modifier keys for resizing is enough, I think.

410

I'd recommend consistency with the terminology used in menus and instead say, "Alt".

sharvey updated this revision to Diff 37944.Jul 17 2018, 11:29 AM
  • String tweaks / Make smallChange and largeChange both "positive" for consistency
sharvey marked 2 inline comments as done.Jul 17 2018, 11:29 AM

I'm happy with this patch now. The behavior seems useful and is invoked naturally, the inline documentation is clear, the code is sensible, and I couldn't find any regressions.

There's only one remaining thing to do from my perspective: update the docbook to document this nice new feature! It would be great if you could do that in this patch. Keep in mind that 18.08 has already been branched and the freeze is in two days, so if you'd like to get this in for the next release, you'll have to work quickly. :)

sharvey updated this revision to Diff 37963.Jul 17 2018, 3:34 PM
  • Edit docbook to include arrow key usage; update release number and date
Restricted Application added a project: Documentation. · View Herald TranscriptJul 17 2018, 3:34 PM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
yurchor accepted this revision.Jul 17 2018, 3:36 PM

There's only one remaining thing to do from my perspective: update the docbook to document this nice new feature! It would be great if you could do that in this patch. Keep in mind that 18.08 has already been branched and the freeze is in two days, so if you'd like to get this in for the next release, you'll have to work quickly. :)

Arrow keys information added. I checked the .docbook file with the doc tools and it parsed properly and generated correct HTML files.

I changed the release tag in the docbook file to 18.08 and set the date to 2018-08-09 based on the Applications Tagging date on the Application Release Schedule page. That was an educated guess. If it should be something else, just let me know and I'll change it.

There's only one remaining thing to do from my perspective: update the docbook to document this nice new feature! It would be great if you could do that in this patch. Keep in mind that 18.08 has already been branched and the freeze is in two days, so if you'd like to get this in for the next release, you'll have to work quickly. :)

Arrow keys information added. I checked the .docbook file with the doc tools and it parsed properly and generated correct HTML files.

I changed the release tag in the docbook file to 18.08 and set the date to 2018-08-09 based on the Applications Tagging date on the Application Release Schedule page. That was an educated guess. If it should be something else, just let me know and I'll change it.

You probably just need checkXML5 to check the document.
About the date: that's not the release date, but the date where the document was last updated.
The release is the first available release where the change will show.

sharvey updated this revision to Diff 37964.Jul 17 2018, 3:46 PM
  • Correct revision date in docbook
ngraham added inline comments.Jul 17 2018, 3:47 PM
doc/index.docbook
154 ↗(On Diff #37963)

Two sentences use the descriptive mood ("Pressing the arrow keys...", "Holding the Alt key..."), while the middle one uses the imperative mood ("Hold down the Shift key..."). We should pick one and be consistent.

About the date: that's not the release date, but the date where the document was last updated.

Thanks. I corrected it to today's date.

ngraham added inline comments.Jul 17 2018, 3:48 PM
doc/index.docbook
154 ↗(On Diff #37963)

Alternatively, we could use the imperative mood for the top-level feature description (Press the arrow keys to slowly move the rectangle" and the descriptive mood for the possible modifications. Your choice. :)

sharvey updated this revision to Diff 37965.Jul 17 2018, 3:52 PM
  • Grammar consistency in docbook fixed
sharvey marked 2 inline comments as done.Jul 17 2018, 3:56 PM
ngraham accepted this revision.Jul 17 2018, 3:56 PM

Excellent. @rkflx, any last comments or objections, or shall we push this to the 18.08 branch?

This revision is now accepted and ready to land.Jul 17 2018, 3:56 PM
rkflx requested changes to this revision.Jul 17 2018, 11:52 PM

@sharvey 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:

I still disagree regarding the default speed selection. We determined by looking at other apps that 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.

Apart from that, the patch now works well for the most part, good job (and thanks to @abalaji for commenting on some things). In addition to some inline comments below, I found those issues:

  • When the rectangle covers the whole screen, I'm unable to resize, i.e. make the rectangle smaller again.
  • I'm unable to fine-tune both size and position with HiDPI scaling active (see also inline comment for the likely cause).
  • Selecting with the mouse sets a minimum size, however resizing with the arrow keys allows to resize to 0x0px.

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).


A few thing tho @sharvey, 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:

When Alt is pressed down:

  • We are in "expand" mode by default. Pressing left expands selection on the left by moving the left border outwards to the left, etc.
  • 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.

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).

doc/index.docbook
154 ↗(On Diff #37965)

"Holding the Shift while": Either remove "the", or add "key".

"using the arrows": Change to "Using the arrow keys"?

src/QuickEditor/EditorRoot.qml
41

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.)

42

That looks suspicious: How will you be able to store the result accurately in an int?

56–57

Unrelated whitespace change, please revert.

83

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?

89

Insert space after switch, please. Repeat for all other occurrences.

95

Do you know about the += operator, which would be more concise?

113–134

This duplicates most of the code for Qt.NoModifier. Could you instead assign largeChange or smallChange to a variable based on the modifier, which you then pass to checkBounds?

Also this is likely independent from whether you are moving or resizing (where all of this is duplicated again currently).

170

Insert space after case.

207

As the mapping between Qt.Key_* and "*" is static, for direction you could pass the key directly instead of creating an extra string.

254

Missing break (not that it would matter with those returns, but there should be at least a consistent style).

337–401

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.

371

Unrelated whitespace change.

406

Would it make sense to use the same capitalization as above, i.e. "keys" instead of "Keys"?

407

Do you mean | instead of && by any chance?

410

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.

410

"15px change" does sound a bit strange: What is "change", and why do "15px" matter? Let me repeat my suggestion from above:

Hold Alt to resize, fine-tune with Shift
This revision now requires changes to proceed.Jul 17 2018, 11:53 PM

I will attempt to get these completed early tomorrow morning (my time). It's already the end of the day here and I know (from unfortunate experience) that I don't do my best work "after hours".

  • Selecting with the mouse sets a minimum size, however resizing with the arrow keys allows to resize to 0x0px.

Where should it stop? 1x1?

sharvey added inline comments.Jul 18 2018, 12:33 PM
src/QuickEditor/EditorRoot.qml
41

This was suggested by @abalaji based on his work on reworking the selection rectangle. Should it be removed?

sharvey added inline comments.Jul 18 2018, 12:57 PM
src/QuickEditor/EditorRoot.qml
337–401

These are remnants (which will be reverted) from when the help text box was two columns wide.

sharvey updated this revision to Diff 38014.Jul 18 2018, 1:56 PM
sharvey marked an inline comment as done.
  • Change default adjustment to largeChange, uses smallChange for fine tuning
  • Numerous miscellaneous changes per reviewer comments, including:
    • Condensed adjustment calculations to use +=
    • Reverted help text box to match original layout
    • Revised docbook entry for clarity and consistency
sharvey marked 9 inline comments as done.Jul 18 2018, 1:59 PM
sharvey added inline comments.
doc/index.docbook
154 ↗(On Diff #37965)

Typos and/error sloppy grammar. Rewritten.

sharvey edited the summary of this revision. (Show Details)Jul 18 2018, 2:01 PM
sharvey edited the test plan for this revision. (Show Details)
sharvey marked 7 inline comments as done.

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.

sharvey added a comment.EditedJul 18 2018, 2:11 PM

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.

It works for me when holding and dragging a new rectangle. I'm a little fuzzy on how the magnifier is intended to work.

We've already commandeerd for small adjustments, so that's likely overriding the magnifier.

How do we think this should work?

Yeah, the magnifier shows up while dragging the selection rect (or not, depending on whether or not you've holding down ), but it currently doesn't show up when you're using Alt to resize the rect using the keyboard arrow keys.

IMHO, if you have the magnifier turned on in the settings, it should show up when you're in "fine tune" mode (holding down both and Alt and using the arrow keys to resize the rect by single pixels), because conceptually, if you're doing this, it's because you want a pixel-perfect rect.

sharvey added a comment.EditedJul 18 2018, 2:21 PM

I still disagree regarding the default speed selection. We determined by looking at other apps that 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.

Fixed. The default movement - Arrows alone - is largeChange, and + Arrows is a single pixel move.

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)

The help text box has been reformatted to match the prior layout. Esc has been moved to the bottom of the list. Also, the docbook entry for this patch has been rewritten and is hopefully clearer and more consistent.


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.

If you're okay with what I have now, are you comfortable pushing it as a beta while I work out the changes to checkBounds?

(Again, thank you for the code optimization suggestions. They are appreciated.)

Yeah, the magnifier shows up while dragging the selection rect (or not, depending on whether or not you've holding down ), but it currently doesn't show up when you're using Alt to resize the rect using the keyboard arrow keys.

IMHO, if you have the magnifier turned on in the settings, it should show up when you're in "fine tune" mode (holding down both and Alt and using the arrow keys to resize the rect by single pixels), because conceptually, if you're doing this, it's because you want a pixel-perfect rect.

Let's get @rkflx 's input on this. I'm not sure how easily it will be to implement. There's a deadline hanging over our heads.

Thanks for the updates. Found one more problem, the other changes are fine.

  • 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.

  • 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).

  • Change default adjustment to largeChange, uses smallChange for fine tuning

Thanks, appreciated!

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).


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 ;)

src/QuickEditor/EditorRoot.qml
89

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.

166

Alt+ is broken (should be fast, not slow)

170

Not yet done.

337–401

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.

410

"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.

Tagging is on Thursday at 23:59 UTC. I will accept the patch tomorrow so you can still land it in time (let me know if you are busy and I should commit on your behalf).

It would be great if you could still implement three changes until then:

  • Move one line in the help text around (see inline comment).
  • Fix Alt+.
  • Revert unnecessary layout changes.

The rest can be improved upon later.

Tagging is on Thursday at 23:59 UTC. I will accept the patch tomorrow so you can still land it in time (let me know if you are busy and I should commit on your behalf).

That’ll be 6:59pm in my world. I’ll see what I can accomplish in the next 24 hours.

  • Revert unnecessary layout changes.

Please clarify... layout for the help text? I don’t want to waste time.

The rest can be improved upon later.

If it’s not ready (E.g. the edge-touching issue), then it’s not ready. I don’t want to see anything released (even as a beta) that’s got bugs. Especially known bugs that we just ran out of time to fix. If it has to wait, it can wait. I don’t want to be responsible for any bending or breaking of rules, and I certainly don’t want to release dodgy software.

  • Revert unnecessary layout changes.

Please clarify... layout for the help text? I don’t want to waste time.

I was referring to the following inline comment: D13450#inline-74855, e.g. the addition of RowLayout etc., which should not be necessary if I understand things correctly.

sharvey updated this revision to Diff 38080.Jul 19 2018, 10:24 AM
  • Fix Alt + Down; Undo layout changes for help text; Reorder help text items
sharvey marked 2 inline comments as done.Jul 19 2018, 10:27 AM
rkflx accepted this revision.Jul 19 2018, 10:32 AM

Thanks, looking much better now.

Please make sure to commit to the stable branch, i.e. Applications/18.08 (let me know if you are unsure what that means).

This revision is now accepted and ready to land.Jul 19 2018, 10:32 AM
sharvey updated this revision to Diff 38082.Jul 19 2018, 10:33 AM
  • Minor whitespace cleanup

Thanks, looking much better now.

Please make sure to commit to the stable branch, i.e. Applications/18.08 (let me know if you are unsure what that means).

Following the Arcanist/Phabricator docs on the KDE wiki...

This revision was automatically updated to reflect the committed changes.

It seems you forgot to merge stable into master, which I now have done with

git checkout master
git pull
git merge -Xours origin/Applications/18.08
git push

Thanks again for the quick update so we could get the string changes in in time, and hopefully for the RC in two weeks you'll be able to solve the remaining problems with checkBounds.

It seems you forgot to merge stable into master, which I now have done with

I was actually in the process of trying to figure that out... I got a lengthy list of warnings when I tried it and the wiki is a bit vague on what to do in that case. I was hunting through the diff history trying to figure out if it was necessary in this case. Regardless, thanks for taking care of it.

Thanks again for the quick update so we could get the string changes in in time, and hopefully for the RC in two weeks you'll be able to solve the remaining problems with checkBounds.

checkBounds seemed so simple, yet it frustrated me endlessly. I'll dig into it and see what's going on. I have a few suspicions.


If you have time, would you mind dropping me a line at scott@spharvey.me? I have some unrelated questions I'd like to get your opinion on. Thanks in advance.