Remove extra pixel when cropping with an aspect ratio
ClosedPublic

Authored by alexmi on Sep 29 2019, 4:45 AM.

Details

Summary

This fixes a bug where using the crop tool and resizing the crop
area by adjusting the top left or top right corners with a fixed
aspect ratio will make the crop area's height a pixel higher than
expected. This is because QRect::bottom() doesn't return the
exact y coordinate.

From Qt Documentation: https://doc.qt.io/qt-5/qrect.html#bottom

Test Plan
  1. Use the crop tool with a 1:1 aspect ratio
  2. Change the crop area by adjusting the top left and top right corners
  3. Verify both width and height have the same value

Diff Detail

Repository
R260 Gwenview
Branch
gwenview-bugfix (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17152
Build 17170: arc lint + arc unit
alexmi created this revision.Sep 29 2019, 4:45 AM
Restricted Application added a project: Gwenview. · View Herald TranscriptSep 29 2019, 4:45 AM
alexmi requested review of this revision.Sep 29 2019, 4:45 AM
anthonyfieroni added inline comments.
lib/crop/croptool.cpp
377

So use y() + height

alexmi added inline comments.Sep 29 2019, 6:13 AM
lib/crop/croptool.cpp
377

I was initially going to do that as the Qt documentation suggests, but as far as I can tell both accomplish the same thing and I chose to simply add 1 instead for consistency with the rest of the file where the same thing was done a couple of times.

anthonyfieroni added inline comments.Sep 29 2019, 6:40 AM
lib/crop/croptool.cpp
377

Yes, they do same thing, but +1 can be forgotten why's its purpose, but when you see y() + height it does not have to think about.

alexmi updated this revision to Diff 67033.Sep 29 2019, 3:24 PM

Use y() + height() instead of bottom() + 1

alexmi marked an inline comment as done.Sep 29 2019, 3:31 PM
ngraham accepted this revision.Oct 2 2019, 1:45 PM
This revision is now accepted and ready to land.Oct 2 2019, 1:45 PM
This revision was automatically updated to reflect the committed changes.