Make moving the crop rectangle possible, tweak accept/cancel behaviour

[DarkDefender]

I have purposely not yet updated the tests as I think there might be need for some discussion or tweaks on the changes as I'm changing the current default behavior.

While using Beeref myself, I found it very weird that clicking inside of the crop rectangle would confirm and not move the rectangle itself. This lead to a few miss clicks here and there where I would accept the change and have to go back into crop mode again. Granted, the main reason for this PR is to make it possible to move the crop rectangle as I usually want to nudge the whole thing around a bit after cropping.

To still be able to quickly confirm with minimal mouse movement, I made it so that if you double click on the crop rect, it will confirm.

While working on this I also realized that the behavior of canceling the crop action by clicking outside of the rectangle felt a bit weird as this is not what happens with the text object. Now it is in line with the text box editing workflow where clicking outside will confirm and only escape will undo your changes. This makes it less tedious as you don't lose your cropping work if you miss click outside of the rectangle.

To me all of these changes are for the better, but I realize that no everyone might feel that same way :)

[rbreu]

Re the accepting/caneling behaviour when clicking outside: I'm a bit conflicted.

Pro your change:

• Erring on the side of keeping the change seems preferable, since you can always easily undo an accidental crop
• It's consistent with the text editing (which is Qt's default implementation)

Con:

• It removes the option to cancel the cropping with mouse only. This is also true for text editing, but with text you are already using a keyboard anyway and can easily press Esc.
• I've checked Krita and Gimp, and both have the outside-click-cancels behaviour.

So unless there are other art programs that have a different behaviour, I'm leaning towards keeping things as they are now.

Now to the moving feature: That was definitely a case of "should be done someday" which ... hasn't been done yet, so thank for working on that! It works very well. Just a couple of remarks:

The mouse pointer should change while hovering over the area where you can move. Off the top of my head:

self.set_cursor(Qt.CursorShape.SizeAllCursor)
...
self.unset_cursor()

While looking at Krita and Gimp for their behaviour, I noticed that Krita behaves exactly like you implemented it, i.e. to confirm the crop you double-click. But I find Gimp's behaviour pretty neat where you can confirm with a single click while also having the drag-to-move behaviour. This is probably do-able by moving the confirmation to the mouse realease event instead of the mouse press event, and checking for whether the mouse has moved between click and release to differentiate between a drag-to-move and a click-to-confirm action. Up to you if you want to look into that.

Do you know anything about unit tests at all yet? This PR breaks some of the unit tests since it's changing existing behaviour, and also introduces new code that isn't corvered by tests. I'd be grateful if you looked into it yourself, though I'd add the unit tests myself otherwise. Let me know if you have any questions regarding tests. The revelant test files for your changes are tests/test_scene.py and tests/items/test_pixmapitem.py, and CONTRIBUTING.rst has instructions on how to run tests locally.

Last but not least, you can add your changes to the CHANGELOG.rst file; the new move feature would go under the 'Added' section. Don't worry about the exact wording; I'll read over everything before doing a realease anyway. I've alread added your last PRs and credited you with your github handle, but feel free to change to whichever attribution you prefer.

[DarkDefender]

Just a heads up: I'll add/fix the tests and update the changelog when we have come to a conclusion about the behavior.
(To reduce the amount of work if we do multiple iterations)

Con:

• It removes the option to cancel the cropping with mouse only. This is also true for text editing, but with text you are already using a keyboard anyway and can easily press Esc.
  But to get into the crop mode you have to press shift + c so you most probably already have your hand on the keyboard, no?
• I've checked Krita and Gimp, and both have the outside-click-cancels behaviour.

So unless there are other art programs that have a different behaviour, I'm leaning towards keeping things as they are now.

You are correct that both GIMP and Krita cancels when clicking outside. However in Krita (and with an option turned on in GIMP), cropping is a destructive operation. So if the user restarts the program or reloads the file, they will not be able to restore the cropped image. The cropping tool in GIMP and Krita is more of a canvas crop instead of a layer/image crop though.

In Beeref data loss is not really an issue and it is trivial to grow/move/restore the crop after confirming.

Of course Beeref is more of a billboard/mindmap program instead of a Image editing/creation program. So I don't think you should lean too hard into what other programs do. Instead you should look at what makes sense for Beeref specifically. (Of course this doesn't mean that you shouldn't take lessons from other applications, but it should also be a good solution for Beeref itself)

In Beeref, I have more of a feeling that most workflows are oriented around organizing and positioning images quickly. With the "click outside to cancel" behavior I feel like this makes it more prone to force users to backtrack and redo their crop operation.

With "click outside to confirm" they can be more lazy with where their mouse is and move their mouse onto the next image without having to think about pressing enter or putting the mouse inside of the crop triangle first and click to confirm. They can instead click to instantly select the next image, thus saving them a mouse click:

recording.mp4

But I find Gimp's behaviour pretty neat where you can confirm with a single click while also having the drag-to-move behaviour. This is probably do-able by moving the confirmation to the mouse realease event instead of the mouse press event, and checking for whether the mouse has moved between click and release to differentiate between a drag-to-move and a click-to-confirm action.

The reason why I did double click instead of single click is that I've had issues with deadzone margins in the past when doing this. The issue is two fold:

1. The deadzone that we program in can sometimes depend on the DPI of the monitor. IE, the distance of 10 pixels is different on a 8k screen than on a 1080p screen. We could probably work around this (but it will make the code more complex)

2. While you can be very precise with mice, tablets or touch pads have a very hard time to click without moving the pointer. So the deadzone have to be kinda big, which makes this annoying for mouse users.
   I'm guessing this is why Krita has double click instead of single click.

---

Those are my counter arguments. Do you think they are reasonable? Note that I'm happy to discuss this further, no rush :)

[rbreu]

I've though about it some more. Let's do as you proposed!

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 87.23404% with 6 lines in your changes missing coverage. Please review.

Project coverage is 97.79%. Comparing base (6e3bc35) to head (e610333).
Report is 6 commits behind head on main.

Files	Patch %	Lines
beeref/items.py	90.69%	4 Missing ⚠️
beeref/scene.py	50.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
- Coverage   97.94%   97.79%   -0.15%     
==========================================
  Files          33       33              
  Lines        4375     4407      +32     
==========================================
+ Hits         4285     4310      +25     
- Misses         90       97       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DarkDefender]

I've though about it some more. Let's do as you proposed!

Awesome! Thank you so much! :)

I've added more commits with the test and flake8 changes.
However I'm a bit unsure how to fix the coverage complaints.
I have added tests for double click confirm and the new crop move functionality, so I'm a bit surprised that it thinks some of the new stuff is not covered.