In 20ff80871670 and 152b591642c7 zooming by Ctrl+wheel and
Ctrl+click was enabled. But while pressing Ctrl the
mouse cursor does not change to the magnifying glass as used in
the normal image view.
This patch updates the cursor while Ctrl is pressed and it
is not over some tool related item e.g. the crop handles.
Details
- Reviewers
rkflx
- Open Gwenview in View Mode
- Enter Crop or Reduce Red Eye tool
- Check that while pressing Ctrl the cursor shows the magnifying glass in appropriate places
- Check that zoom cursor and zoom by Ctrl+click correspond
Diff Detail
- Repository
- R260 Gwenview
- Branch
- tool-zoom-cursor2 (branched from master)
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2118 Build 2136: arc lint + arc unit
This version does its own cursor handling inside the edit tools.
I first tried it by ignoring/accepting the mouse and key events and let AbstractImageView show the magnifying glass (see preview Diff 38774), but for crop tool it still doesn't work in all situations. Sometimes an event has to be accepted for one action but the same should be ignored to do something in AbstractImageView.
What do you think, is this way here ok?
Still missing: After clicking on a tool option widget (e.g. red eye size slider) the focus is removed from the image and no more keyPressEvents are received.
lib/documentview/abstractrasterimageviewtool.cpp | ||
---|---|---|
40 | Maybe this could be changed to a global object for using here and in AbstractImageView? |
Thanks for the patch.
The focus and keyboard problems you are describing sound much like what I was faced with when trying to improve the cursor handling for zooming in Compare mode. Perhaps it might make sense to wait for that in case I'm able to finish that patch (IIRC there were still some issues, and getting 18.08 out is currently more important). Then we'll see what is a generic issue, and what has to be fixed for Crop specifically.
Thus, can we work on this later, given that it's not quite ready yet and it's for master anyway?
Sure! I did not work on the focus problem yet. I thought about forwarding the events from the widgets but did not try anything.
Let's see what your patch brings.
Sorry for the wait, but I did not get to the focus/hover issue yet (and thus did not start with the actual review).
Meanwhile, I discovered a problem: Ctrl-double-click now wrongly accepts the Crop instead of zooming, same thing for Reduce Red Eye where invalid rect is printed (after D14755 it still worked fine, though). In particular, calling accept() from AbstractImageView::mousePressEvent triggers the issue.
However, maybe the root cause is elsewhere: I noticed that without your patch double-clicking will only call RasterImageView::mouseDoubleClickEvent when no tool is active. With an active tool, there are two calls to RasterImageView::mousePressEvent, which is a bit strange.
lib/documentview/abstractimageview.cpp | ||
---|---|---|
138 | Hiding that in the function can be a bit unexpected. Maybe we should return a bool, and depending on that accept or do something else in the caller? |
No hurry, this isn't the easiest one and has a couple of pitfalls.
Meanwhile, I discovered a problem: Ctrl-double-click now wrongly accepts the Crop instead of zooming, same thing for Reduce Red Eye where invalid rect is printed (after D14755 it still worked fine, though). In particular, calling accept() from AbstractImageView::mousePressEvent triggers the issue.
However, maybe the root cause is elsewhere: I noticed that without your patch double-clicking will only call RasterImageView::mouseDoubleClickEvent when no tool is active. With an active tool, there are two calls to RasterImageView::mousePressEvent, which is a bit strange.
I think this diff was branched before your invalid-rect and double-click-accept patches. After rebase and adding checks in mouseDoubleClickEvent these issues should be gone.
In Crop there is a difference for left/right click while hovering over a handle. Left click is passed to the handle dragging code while right click zooms out. Should we filter this out to be consistent?
No, I rebased already, because otherwise I would not have been able to trigger the incorrect accept by double-clicking in the first place ;)
In Crop there is a difference for left/right click while hovering over a handle. Left click is passed to the handle dragging code while right click zooms out. Should we filter this out to be consistent?
Ah, good catch. I think this should be consistent with what the cursor is showing. Ideally holding Ctrl will never change to a resizing cursor, and zooming in and out will work everywhere.
This is already working great for both the OpenHandCursor and ArrowCursor areas, only the handles are still off.
What about the lock ratio by holding Ctrl / ⇧? That's why I did not change the cursor in the handle areas. Ctrl+click over a handle does not zoom but resize with locked ratio.
Maybe we could do it this way?
- pressing Ctrl first and then clicking on a handle (or anywhere else) does zoom, including zoom cursor anywhere
- first clicking on handle and then pressing Ctrl / ⇧ will lock the ratio
Oh, I missed that, sorry.
Maybe we could do it this way?
- pressing Ctrl first and then clicking on a handle (or anywhere else) does zoom, including zoom cursor anywhere
- first clicking on handle and then pressing Ctrl / ⇧ will lock the ratio
I think that's a bit too complicated, both from a UI standpoint and in the implementation, in particular when actions depend on the order you click and press a key. Some users might not expect zooming in the second case, because they are used to press the modifier first. (Not zooming when clicking on a handle might also be surprising, but given that zooming works everywhere else and resizing only works over the handles, we should probably assume resizing to happen more often when the cursor is over the handle.)
I'd say the best compromise is to not zoom over handles, and adapt the cursor accordingly. This way there is a clear separation: The standard method of zooming is overridden by a more specialized interaction, in this case locked resizing.