Propagate unused mouse press events in edit tools
ClosedPublic

Authored by muhlenpfordt on Jul 24 2018, 9:58 AM.

Details

Summary

If one of the edit tools Crop or Red Eye Reduction is active,
zooming by Middle-Click or Ctrl+Left/Right-Click
does not work.
This patch ignores mouse press events not used by the edit tools
itself and thereupon they are propagated to the image view.

FIXED-IN: 18.08.0

Test Plan
  • Open Gwenview in View Mode
  • Enter Crop or Red Eye Reduction tool
  • Check if Ctrl+Left/Right-Click zoom works
  • Check if () Middle-Click zoom works
  • Check if crop tool ratio lock (Ctrl/) works

Diff Detail

Repository
R260 Gwenview
Branch
tool-zoom-by-click (branched from Applications/18.08)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1160
Build 1173: arc lint + arc unit
muhlenpfordt requested review of this revision.Jul 24 2018, 9:58 AM
muhlenpfordt created this revision.
muhlenpfordt added inline comments.
lib/crop/croptool.cpp
305

Directly setting mMovingHandle will lead to hidden handle boxes if the event is ignored.

rkflx requested changes to this revision.Jul 24 2018, 5:54 PM
rkflx added a subscriber: rkflx.

Thanks! Uses accept and ignore just like I suspected when suggesting this, and works very well for middle click.

Ctrl zooms nicely too, but would it be too much to ask to show the correct cursor, i.e. the magnifying glass? Also, for Red Eye Reduction, the cursor seems to be stuck on ClosedHandCursor. Perhaps this is an issue of propagating the key press events?

(I'm only requesting changes for the inline comment, though, the rest is optional.)

lib/crop/croptool.cpp
298–299

left-click allows to change the cursor shape, but I don't see what for. (Left-click to drag the image when not over the selection would make sense, although that's probably a different patch.)

I wonder what you need Qt::ShiftModifier for at all here, because you have already handled all non-left-clicks in the first if.

This revision now requires changes to proceed.Jul 24 2018, 5:54 PM
muhlenpfordt marked an inline comment as done.

Removed filtering Qt::ShiftModifier

Ctrl zooms nicely too, but would it be too much to ask to show the correct cursor, i.e. the magnifying glass? Also, for Red Eye Reduction, the cursor seems to be stuck on ClosedHandCursor. Perhaps this is an issue of propagating the key press events?

I got this partly working by ignoring some more events in the right places and let AbstractImageView do the cursor work.
But I'm stuck with resetting the cursor since the mouseReleaseEvent seems to get lost for Ctrl+click - that's why the ClosedHandCursor still shows up.
I think this is a bit more complicated and maybe better go to a different patch.

lib/crop/croptool.cpp
298–299

Right, of course Qt::ShiftModifier is not needed.

rkflx accepted this revision.Jul 25 2018, 10:02 AM

Off you go!

Ctrl zooms nicely too, but would it be too much to ask to show the correct cursor, i.e. the magnifying glass? Also, for Red Eye Reduction, the cursor seems to be stuck on ClosedHandCursor. Perhaps this is an issue of propagating the key press events?

I got this partly working by ignoring some more events in the right places and let AbstractImageView do the cursor work.
But I'm stuck with resetting the cursor since the mouseReleaseEvent seems to get lost for Ctrl+click - that's why the ClosedHandCursor still shows up.
I think this is a bit more complicated and maybe better go to a different patch.

Okay, fair enough. I also worked on improving zoom cursor handling in Compare mode a while ago, but that also got really complicated and needs a bit more work. Let's not worry about the problem in this Diff.

This revision is now accepted and ready to land.Jul 25 2018, 10:02 AM
This revision was automatically updated to reflect the committed changes.

But I'm stuck with resetting the cursor since the mouseReleaseEvent seems to get lost for Ctrl+click - that's why the ClosedHandCursor still shows up.

Found the reason for this. :)
The previous mousePressEvent needs to be accepted somewhere (for zooming in AbstractImageView), otherwise the subsequent mouseReleaseEvent is not triggered.

rkflx added a comment.Jul 26 2018, 6:36 PM

Found the reason for this. :)
The previous mousePressEvent needs to be accepted somewhere (for zooming in AbstractImageView), otherwise the subsequent mouseReleaseEvent is not triggered.

That's great, send a patch!