Fix rectangle selection edge not follow mouse
ClosedPublic

Authored by guotao on Mar 23 2018, 8:29 AM.

Details

Summary
  1. In rectangle mode, when to adjust selection, the right, bottom edge not follow mouse
  2. Fix wrong rectangle size
Test Plan
  1. In rectangle mode, adjust the ( topright, right, bottom right, bottom, bottom left) selection and check mouse and edge position.

2 select full screen and check size
3 save image and check image size

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
guotao requested review of this revision.Mar 23 2018, 8:29 AM
guotao created this revision.
rkflx requested changes to this revision.Mar 24 2018, 12:43 PM
rkflx added subscribers: ngraham, rkflx.

Thanks, that's a nice bug you caught there! Basically the snapping of the cursor to the selection rectangle has an unwanted offset for both the bottom and the right edge.

However, are you sure your patch is working right? On a 1280x720px screen, without your patch the largest rectangle I'm able to draw is 1280x720px, but with your patch it only has a size of 1279x719px.

I'm also not a fan of those magic numbers in the code. Could you try to figure out where those are coming from and perhaps define them as a constant?


That said, I wonder why we need that snapping behaviour at all. Both GIMP and Inkscape do not snap the cursor to the object and instead simply allow to resize directly, resulting in a less jumpy and unexpected experience which is important for this kind of precision work.

If @ngraham agrees, maybe that would be a good direction for your patch (and also for Gwenview's Crop tool, I guess…).

This revision now requires changes to proceed.Mar 24 2018, 12:43 PM

I second everything @rkflx said. Let's disambiguate the magic numbers, and try to rid of this jumpy behavior in the first place.

guotao updated this revision to Diff 30471.Mar 25 2018, 9:18 AM
guotao edited the summary of this revision. (Show Details)
guotao edited the test plan for this revision. (Show Details)
  1. fix wrong rect width and height the pixels is start from 0, so the width should be start_offset - end_offset + 1
  2. fix right and bottom edge move unexpected bug
    1. select a rect, make sure the right/bottom edge is beside moniter edge
    2. drag the topleft, top, top right up quickly or drage top left, left, bottom left to left quickly, the bottom/ right edge move unexpected to fix this issue, just set x/y before width/height
guotao updated this revision to Diff 30472.Mar 25 2018, 9:18 AM

just update diff, prev diff miss some changes

guotao updated this revision to Diff 30473.Mar 25 2018, 9:21 AM

just update diff, prev diff is still wrong

rkflx requested changes to this revision.Mar 25 2018, 7:56 PM

Thanks for the updates, works great now and looks better with those constants. Some cleanup in the code left, then this can go in.

@ngraham Could you have a second look on the code with your QML experience? Thanks ;)

(I guess reducing the "jumpiness" is better done in a separate patch.)

src/QuickEditor/SelectionRectangle.qml
28–29

Please change var to int, which should be faster.

84–86

Is it really necessary to swap those lines?

(Same comment applies to a bunch of other places…)

This revision now requires changes to proceed.Mar 25 2018, 7:56 PM

QML looks sane to me, modulo your comments, @rkflx.

guotao updated this revision to Diff 30577.Mar 26 2018, 2:35 AM
guotao edited the test plan for this revision. (Show Details)

update diff

ngraham accepted this revision.Mar 26 2018, 3:07 AM

Thanks for this patch, Tao! The code looks sensible to me, and it works as advertised. A good improvement to Rectangular Region mode

I noticed you didn't use arc to produce the patch, which means we need to ask you for your email address before we can land this (if and when Henrik signs off too). Can you provide your email address?

You might look into using arc in the future: https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

guotao marked 2 inline comments as done.Mar 26 2018, 3:44 AM

Thank you ngraham
Tao Guo <guotao945@gmail.com>

src/QuickEditor/SelectionRectangle.qml
84–86

Yes, It's necessary, these changes fix another bug:

select a rect, make sure the right/bottom edge is beside moniter edge
drag the topleft, top, top right up quickly or drag top left, left, bottom left to left quickly, the bottom/ right edge jump to up / left

I will submit these changes in separate patch

guotao marked 2 inline comments as done.Mar 26 2018, 1:51 PM
rkflx accepted this revision.Mar 26 2018, 8:34 PM

Thanks again, I'll now land this on your behalf.

This revision is now accepted and ready to land.Mar 26 2018, 8:34 PM
This revision was automatically updated to reflect the committed changes.