Remove extra pixel when cropping with an aspect ratio
ClosedPublic

Authored by alexmi on Sun, Sep 29, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alexmi created this revision.Sun, Sep 29, 4:45 AM
Restricted Application added a project: Gwenview. · View Herald TranscriptSun, Sep 29, 4:45 AM
alexmi requested review of this revision.Sun, Sep 29, 4:45 AM
anthonyfieroni added inline comments.
lib/crop/croptool.cpp
377

So use y() + height

alexmi added inline comments.Sun, Sep 29, 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.Sun, Sep 29, 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.Sun, Sep 29, 3:24 PM

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

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