Add crop feature

[Yutsuten]

As discussed in #8 , I'm fixing the issues left in the crop branch.

Update from master

For now, I merged master into crop. Did only a simple manual test, everything except the :crop command seems to work as expected. I'll deal with that later.

ModuleNotFoundError: No module named 'vimiv.gui.transform_widget'

Conflicts

Most conflicts was import ones. Slightly easy to fix.

But a conflict in vimiv/imutils/_file_handler.py was a little more tricky. The code bellow was added to vimiv/imutils/_file_handler.py in @property changed()

if self.original is None:
    return False

But removed in master: 94dc2d5

After some investigation and debugging I moved/adapted that code to vimiv/imutils/imtransform.py in @property changed().

@property
def changed(self):
    """True if transformations have been applied."""
    transformed = not self.isIdentity()
    if self._original is None:
        return transformed
    cropped = self.current.rect() != self._original.rect()
    return transformed or cropped

Next steps

1. Make the :crop command work
2. Look for and fix bugs

[karlch]

Oh wow, thanks a lot, that was super fast!
Excellent catch with the changed-related code, looks correct to me on a first glimpse.

Reading into your comments, I noticed I was an idiot in at least two ways ...

1. I didn't push a few commits to the crop branch, nothing super important, but some styling and bugfixing. I would try to cherry-pick these onto your branch.
2. I guess I never commited vimiv.gui.transform_widget, which is now vimiv.gui.transformwidget.

I'll try to figure these two out, and hopefully get the "old" state of crop running on your branch based on the current master. You could then continue with the second task, i.e. "look for and fix bugs" 😊 I'll try to be fast 🤞

[Yutsuten]

About the number 2, I think it was renamed... At least I think I saw something like this while fixing the conflicts. Maybe there was some old code still using the transform_widget and wasn't detected by git as a conflict..?

[karlch]

Definitely seems to be the case, so either I forgot to commit or git is confused by the rename 😊 no matter what, the cherry-picking was straightforward, after you have done all the hard work. I pushed to crop-yutsuten where the :crop command seems to run like it was at the time. Feel free to just grab the few commits, and I can delete the branch here again.

There are a few simple linter / mypy errors, but those are fast to change. I noticed currently one is not allowed to zoom after :crop, so that is probably what I did as a quickfix to avoid having to deal with zoomed images. Not sure if that is a dealbreaker, I guess zooming would be nice for detailed cropping, but maybe this is just out-of-scope for vimiv 🤔

[Yutsuten]

Just merged crop-yutsuten into crop !!

About zooming while cropping, implementing it will probably be give us some trouble and may not worth the work... We can add that in another PR if needed.

[Yutsuten]

I did a fast test here and the crop is indeed working now!

Just one thing I noticed, when the image have a white background, it is hard to find the squares used to resize the crop area... We could try changing it to some color that contrast with any color. Maybe black borders with white fill or something like that.

For comparison, Firefox's screenshot feature uses some shadow around the circles to contrast with white background:

[karlch]

Nice!! 🥳

That is definitely not a great look, good catch! If you like, you can play around with the styling, and commit your favourite option. The css that is applied as a Qt stylesheet is in the class definition of vimiv.gui.crop.CropWidget, where the QSizeGrip part is applied to the handles. The style definition (stuff in curly-braces) is in vimiv.config._style_options at the very bottom 😊 and user-configurable as usual with styles, but the defaults should of course be solid.

The firefox version does look great, but not sure how we would get shadows with Qt, that are not just offsets using QGraphicsDropShadowEffect 🤔

[Yutsuten]

Some changes

1. Now the grip border is gray and its fill is white. Changed the selection border to gray to match the grip border color. I believe to be good defaults, but these all can be customized by the user.
2. Default the crop selection to center of the image instead of top left. This is probably very personal, but I do think it being initialized in the center is better.

3. Fixed some lint and mypy errors. But there are two of them I need your opinion.

For this one I just removed the type: ignore and seems to be good. Unsure why it was there in the first place...

vimiv/gui/resize.py:27: error: Unused "type: ignore" comment
            self.destroyed.connect(QApplication.restoreOverrideCursor)  # type: ignore

This one will probably require some refactoring...

************* Module vimiv.gui.image
vimiv/gui/image.py:44:0: R0904: Too many public methods (21/20) (too-many-public-methods)

Search for bugs

I also did some testing... Seems to work pretty good, but also there aren't too much to check... We cannot run commands, use shortcuts or anything except Enter or ESC to get out the cropping mode. I also did not find any way to select outside of the image or anything that could crash vimiv.

Test code

Looking at GitHub seems that some code don't have test code, but I'm unsure I'll be able to create those. While I do have experience with python testing, my experience is with web systems, not desktop programs... Some directions might help.

[karlch]

Thanks again, this is looking great!

1. Love it, thanks 😊
2. I agree, centred makes a lot of sense. Would you mind fixing the crop-related tests which now need different coordinates in tests/end2end/features/edit/crop.feature? Here the coordinates assume the crop rectangle starts at 0+0 which is now of course no longer the case.
3. Removing is perfect. I assume the PyQt5 stubs were improved over the past few years, so from time to time what used to need a type: ignore is now perfectly valid.
4. Feel free to just add a pylint: disable=too-many-public-methods in the appropriate place along with a comment somewhere along # TODO consider refactoring or so. This definitely doesn't have to happen now and in this PR.

Search for bugs

Sounds good to me 👍

Tests

As there are already a few tests related to crop, I am totally fine keeping it as-is. Improving coverage is always nice of course, but I wouldn't consider this breaking, especially as mouse-related stuff is always quite a hassle to get right and stable.

AUTHORS

Finally, if you like, please add yourself to the AUTHORS file 😊

[Yutsuten]

I've fixed the crop tests.

By the way I needed to change tests/end2end/features/edit/test_crop_bdd.py too, adding crop.pos() to start was causing the drag to not work. My guess is that the crop variable we're passing to mousedrag() is using coordinates relative to the crop widget already.

Also added my name to the AUTHORS file even though I don't think I've done anything big here. Most work was done by you, I only updated the code to master and did some small adjusts. Hopefully someday I'll be able to help again! 😉

[karlch]

Thanks a lot, merged 🎉

This makes sense, thanks for catching!

Well without you crop would still be dead in some ancient branch, this was certainly a lot of help! Definitely share that hope 😉

[Yutsuten]

By the way I think we'll need to update the documentation about this (and also the new cursor-position statusbar module), I can help on it!

[karlch]

At least the very basic description does show up in the statusbar and commands documentation, no? If you would like to add more specific information, absolutely go for it 😊 but for me this would be a good starting point, as manual documentation always needs, well, manual maintenance ..

[Yutsuten]

Oohhh there is something already! Nice, didn't notice that... I'll take a look more closely later and if I feel there is something missing I'll add it!

[Yutsuten]

Now that I've take a look at it, I think it is good as is. Short and concise. Straight to the point. No need to make it more verbose...
Good job with those doc updates while changing the code 👍

[karlch]

Perfect 😊

Hint in case you ever work on the documentation in the future: this part of the documentation (commands, statusbar modules, settings, ...) is auto-generated from the code + docstrings, no need to edit any of the .rst files by hand. Editing these explicitly is only required for more details, explanations, whatever. That is what I referred to as manual documentation - in contrast to the generated part. The slightly hacky script that does this is in scripts/src2rst.py.