Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cropping for LocalImages #169

Merged
merged 1 commit into from Mar 3, 2019

Conversation

Projects
None yet
2 participants
@karinassuni
Copy link
Contributor

karinassuni commented Mar 3, 2019

Summary

In re: #77
Implements cropping for LocalImages. The cropping happens before resizing the rendition, so a cropped image can be subsequently rescaled.

Detailed description

This PR adds cropping functionality to LocalImages. A new argument to Markdown image inlines, crop=(left,top,right,bottom), will be processed to this end. To handle cropping "before" resizing logic so that they can happen separately and synergize, I calculate the size of the crop box and use it for the input size in the resizing calculation function LocalImage.get_rendition_size. Resizing based off of the dimensions of the original image wouldn't work of course since cropping gives us a new image with new dimensions. In LocalImage._render, PIL.Image.Image.crop actually does the cropping for the generated image, and happens right before PIL.Image.Image.resize.

Developer/user impact

The user now has the option to crop local images via a crop=(l,t,r,b) argument to image inlines in Markdown.

Test plan

PlaidWeb/publ-site#4 includes one example/test of resizing and cropping simultaneously in the "Image rendition tests" entry. One can test this functionality in an entry against other crop values—not included in the PR is the test ![alt text](rawr.jpg{crop=(0,0,960,704)} "test crop double the size of the original image dimensions"), which actually just extends the image to double the size, but filling in the new size with a black background. I'm not sure what the desired functionality should be in this case of probable user error.

@fluffy-critter
Copy link
Collaborator

fluffy-critter left a comment

So, the reason I never got around to adding this functionality myself is that I thought there shouldn't be any need to pass crop in as a separate parameter to the rendering function, since box already provides an input cropping box. I was intending that any change for this would simply be changing the get_rendition_size function to adjust the input box to produce a new output box so the crop-scale operation all happens in a single step.

However: I realize now that I was massively overthinking this. :) My reasoning around it was that this would both perform better (which doesn't matter because the output is cached indefinitely, and PIL might even optimize it out already) and would produce more humane rendition image filenames (which doesn't matter because people shouldn't be directly using rendition filenames to begin with). I also had a vague notion that it would produce better image quality although when I actually think about that it makes no sense either.

So, historical rambling aside, this looks pretty good and is nice and straightforward. A few things I'd like to see before I merge it in:

  • A couple more tests, particularly in combination with resize="fill"
  • Support for fullsize_crop (should just be a matter of adding the crop argument to the get_fullsize key list, which should probably be turned into a tuple and made a static class property come to think of it), and tests for that too

Seeing some of the added pylint exclusions also has me wanting to refactor some things here but don't worry about that. :)

Regarding support for StaticImage/RemoteImage, they do already use a CSS container for the purpose of doing resizing (since the input size is unknown), but because the input size is unknown we don't really have any way to meaningfully adjust the container positioning anyway -- so I wouldn't worry about that. It's already in the docs that non-local images don't get the full support of everything, and I'm totally fine with this being another thing it doesn't support.

Show resolved Hide resolved publ/image.py
@fluffy-critter

This comment has been minimized.

Copy link
Collaborator

fluffy-critter commented Mar 3, 2019

Ah, heh, looking at the Pillow docs, Image.crop used to be lazy but now it happens immediately. Not that it even matters.

Oh, and regarding how to handle the user error of a crop rectangle exceeding the original bounds, I think just letting Pillow handle that is fine.

@fluffy-critter fluffy-critter merged commit 3b4b391 into PlaidWeb:master Mar 3, 2019

@fluffy-critter

This comment has been minimized.

Copy link
Collaborator

fluffy-critter commented Mar 3, 2019

Actually I'll just accept this PR and make my preferred changes myself while I refactor things a bit anyway. Thanks for contributing!

@karinassuni

This comment has been minimized.

Copy link
Contributor Author

karinassuni commented Mar 4, 2019

Alrightie times two! Thank you for accepting my contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.