Lock crop ratio to current rect when holding Ctrl/Shift
ClosedPublic

Authored by huoni on Mar 16 2018, 5:55 AM.

Details

Summary

In many graphics apps, holding Ctrl and/or Shift locks the ratio of
the rectangle being resized, making it quicker than manually choosing
a ratio. This patch adds this functionality to the Crop tool.

When Ctrl or Shift is pressed, the current ratio of the crop rect
is saved, "locking it". When resizing, so long as Ctrl or Shift is
pressed, the ratio is enforced to this locked value. Otherwise it
behaves the same as before - enforcing ratio if one was chosen in
the GUI

Depends on D11379

Test Plan
  • Crop tool behaves normally when Ctrl/Shift not pressed
  • Crop ratio locked when Ctrl/Shift is pressed
  • Ctrl/Shift has no effect if ratio is already restricted by the Basic toggle or Advanced combobox

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.
huoni requested review of this revision.Mar 16 2018, 5:55 AM
huoni created this revision.
rkflx requested changes to this revision.Mar 19 2018, 12:11 AM

Works great, just some comments on the code.

lib/crop/croptool.cpp
91

The variable name sounds like a lock, but actually it's a ratio. Perhaps mLockedCropRatio?

226–227

Do you also need to initialize mCropRatioLock here?

406

Adding qDebug(), this triggers not only for or Ctrl, but for lots of other keys too, e.g. k.

This revision now requires changes to proceed.Mar 19 2018, 12:11 AM
huoni updated this revision to Diff 29965.Mar 20 2018, 5:34 AM
  • Fix so only triggers on Ctrl and Shift
huoni updated this revision to Diff 29966.Mar 20 2018, 5:39 AM
  • Change name of member variable to something more descriptive
huoni marked 2 inline comments as done.Mar 20 2018, 5:41 AM
huoni added inline comments.
lib/crop/croptool.cpp
226–227

Well, that isn't mCropRatio :)

That said, we don't need this anymore now we are always setting the ratio from the config, so we can remove (I assume I should update the config save patch, not this one)

huoni added inline comments.Mar 20 2018, 5:42 AM
lib/crop/croptool.cpp
226–227

Oops, what I meant was, that isn't mCropRatioLock.

(gotta proofread my comments more)

rkflx requested changes to this revision.Mar 21 2018, 1:06 AM

Fix so only triggers on Ctrl and Shift

Confirmed ;)

lib/crop/croptool.cpp
226–227

Sorry for being stubborn, but in mouseMoveEvent for me mLockedCropRatio is 1.57687e-316. Yes, it won't do much harm, because as soon as you press the modifier it will be initialized.

Still, someone might change things around in the future, so your assumption might break. Always initialize member variables to a known value in the constructor, please.

This revision now requires changes to proceed.Mar 21 2018, 1:06 AM
huoni added inline comments.Mar 21 2018, 1:36 AM
lib/crop/croptool.cpp
226–227

Sorry I completely misunderstood what you meant here, no need to apologise.

huoni updated this revision to Diff 30168.Mar 22 2018, 12:59 AM
  • Rebase
rkflx accepted this revision.Mar 22 2018, 10:49 AM
This revision is now accepted and ready to land.Mar 22 2018, 10:49 AM
This revision was automatically updated to reflect the committed changes.