D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#294820 , @rkflx wrote:
  
  > 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 `sc...@spharvey.me`? I 
have some unrelated questions I'd like to get your opinion on. Thanks in 
advance.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Henrik Fehlauer]

rkflx added a comment.


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

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

This revision was automatically updated to reflect the committed changes.
Closed by commit R166:48eacd1ba149: Add arrow keys to move and resize selection 
rectangle (authored by sharvey).

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=38082&id=38083

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#294808 , @rkflx wrote:
  
  > 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...

REPOSITORY
  R166 Spectacle

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey updated this revision to Diff 38082.
sharvey added a comment.


  - Minor whitespace cleanup

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=38080&id=38082

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R166 Spectacle

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey marked 2 inline comments as done.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey updated this revision to Diff 38080.
sharvey added a comment.


  - Fix Alt + Down; Undo layout changes for help text; Reorder help text items

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=38014&id=38080

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Henrik Fehlauer]

rkflx added a comment.


  In D13450#294541 , @sharvey wrote:
  
  > In D13450#294538 , @rkflx wrote:
  >
  > > - 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#294538 , @rkflx wrote:
  
  > 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Henrik Fehlauer]

rkflx added a comment.


  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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Henrik Fehlauer]

rkflx added a comment.


  Thanks for the updates. Found one more problem, the other changes are fine.
  
  In 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 , @sharvey wrote:
  
  > In 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 , @sharvey wrote:
  
  > - Change default adjustment to `largeChange`, [⇧] uses `smallChange` for 
fine tuning
  
  
  Thanks, appreciated!
  
  In 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 , @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

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#294304 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#294047 , @rkflx wrote:
  
  > 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In 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.
  
  
  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 commandeers [⇧] for small adjustments, so that's likely 
overriding the magnifier.
  
  How do we think this should work?

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey marked 7 inline comments as done.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey edited the summary of this revision.
sharvey edited the test plan for this revision.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey marked 9 inline comments as done.
sharvey added inline comments.

INLINE COMMENTS

> rkflx wrote in index.docbook:154
> "Holding the Shift while": Either remove "the", or add "key".
> 
> "using the arrows": Change to "Using the arrow keys"?

Typos and/error sloppy grammar. Rewritten.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey updated this revision to Diff 38014.
sharvey marked an inline comment as done.
sharvey added a comment.


  - 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

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=37965&id=38014

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added inline comments.

INLINE COMMENTS

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

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

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added inline comments.

INLINE COMMENTS

> rkflx wrote in 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.)

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

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In 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?

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


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

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Henrik Fehlauer]

rkflx requested changes to this revision.
rkflx added a comment.
This revision now requires changes to proceed.


  @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).
  
  ---
  
  In D13450#283620 , @abalaji wrote:
  
  > 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).

INLINE COMMENTS

> index.docbook:154
> +
> +You can use the arrow keys to move and adjust 
> the rectangle. Pressing the arrow keys will slowly move the rectangle. 
> Holding the Shift while using the arrow keys will move the 
> rectangle more quickly. Holding the Alt key while using the 
> arrows will adjust the size of the rectangle. 
> +

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

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

> EditorRoot.qml:41
>  property int magOffset: 32;
> +property int largeChange: 15 / Screen.devicePixelRatio;
> +property int smallChange: 1 / Screen.devicePixelRatio

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

> EditorRoot.qml:42
> +property int largeChange: 15 / Screen.devicePixelRatio;
> +property int smallChange: 1 / Screen.devicePixelRatio
>  

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

> EditorRoot.qml:54-57
> + "x":   xx,
> + "y":   yy,
> + "height":  hh,
> + "width":   ww

Unrelated whitespace change, please revert.

> EditorRoot.qml:84
> +
> +var delta;  // valid for either direction
> +

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?

> EditorRoot.qml:92
> +// nested switches for arrow keys based on modifier keys
> +switch(event.modifiers) {
> +

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

> EditorRoot.qml:98
> +delta = checkBounds(-smallChange, 0, "left");
> +selection.x = selection.x + delta;
> +break;

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

> EditorRoot.qml:116-137
> +case Qt.ShiftModifier:
> +switch(event.key) {
> +case Qt.Key_Left:
> + delta = checkBou

D13450: Add arrow keys to move and resize selection rectangle

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R166 Spectacle

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey marked 2 inline comments as done.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey updated this revision to Diff 37965.
sharvey added a comment.


  - Grammar consistency in docbook fixed

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=37964&id=37965

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> ngraham wrote in index.docbook:154
> 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.

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

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> index.docbook:154
> +
> +You can use the arrow keys to move and adjust 
> the rectangle. Pressing the arrow keys will slowly move the rectangle. Hold 
> down the Shift to move in larger increments. Holding the 
> +Alt key while using the arrows will 
> adjust the size of the rectangle. 

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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#293854 , @ltoscano wrote:
  
  > 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey updated this revision to Diff 37964.
sharvey added a comment.


  - Correct revision date in docbook

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=37963&id=37964

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Luigi Toscano]

ltoscano added a comment.


  In D13450#293846 , @sharvey wrote:
  
  > In D13450#293781 , @ngraham 
wrote:
  >
  > > 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey added a comment.


  In D13450#293781 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Yuri Chornoivan]

yurchor accepted this revision.

REPOSITORY
  R166 Spectacle

REVISION DETAIL
  https://phabricator.kde.org/D13450

To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: kde-doc-english, abalaji, #spectacle, skadinna

D13450: Add arrow keys to move and resize selection rectangle

[Scott Harvey]

sharvey updated this revision to Diff 37963.
sharvey added a comment.
Restricted Application added a project: Documentation.
Restricted Application added a subscriber: kde-doc-english.


  - Edit docbook to include arrow key usage; update release number and date

REPOSITORY
  R166 Spectacle

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13450?vs=37944&id=37963

BRANCH
  arcpatch-D13450

REVISION DETAIL
  https://phabricator.kde.org/D13450

AFFECTED FILES
  doc/index.docbook
  src/QuickEditor/EditorRoot.qml

To: sharvey, rkflx, ngraham, #spectacle
Cc: kde-doc-english, abalaji, #spectacle, skadinna