Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The crop implementation crops an image regardless if there is really something to crop. I think it would make more sense to crop only if the image is actually bigger than the cropping area.
If I want to crop all images to a max height of 100px I don't want images with 50px height to be enlarged to 100px by inserting a transparent part. That's how it works today and it doesn't really make sense like that.
Comment | File | Size | Author |
---|---|---|---|
#22 | imagecache-367484-22.patch | 2.85 KB | c4rl |
#20 | imagecache-367484-20.patch | 2.86 KB | c4rl |
#9 | 367484_9_imagecache_upcrop.patch | 3.09 KB | greggles |
#8 | 367484_8_imagecache_upcrop.patch | 3.09 KB | greggles |
#7 | 367484_allow_upcrop.patch | 2.43 KB | greggles |
Comments
Comment #1
Tony Sharpe CreditAttribution: Tony Sharpe commentedI have the same issue. I have the occasional image that is less than my desired final image size and in those cases I just want that image to be unchanged. Using 'scale and crop' a small image gets upsized. Using 'depricated scale (outside)' then 'crop' puts a black background around it to bring it up to the crop size. This is no good as I then process it further with imagecache_actions. Is there a way to only crop if required? Tested with both beta 8 and latest dev.
Comment #2
drewish CreditAttribution: drewish commentedwell i think there's clearly a use for it. if you want to get a larger canvas you'd use the crop function to do so. that said adding an "allow upcropping" option (similar to the allow upscaling option) would be helpful for cases like yours where you don't want that behavior.
Comment #3
ambo CreditAttribution: ambo commentedI change that back to bug report because from the editorial point of view it is a bug that pictures that don't have enough sizes to be cropped are cropped.
The behaviour that is expected here is that images with smaller size are not upscaled. I don''t think there is at least one case that anybody wants to have upscaling...
thanks a lot
Comment #4
jddh CreditAttribution: jddh commentedAnyone find a solution to this?
Comment #5
Bilmar CreditAttribution: Bilmar commentedsubscribing
Comment #6
jddh CreditAttribution: jddh commentedI resolved this by hacking the module. Not a clean fix, as I haven't added an "upscale" toggle in admin, but I disabled all "upcrops".
Comment #7
gregglesAttached is a patch which adds this feature.
I felt like it makes sense to allow upcropping in either width or height or neither, so it's actually two new checkboxes in the UI. It defaults to match the current behavior.
I think that ideally we would add some more parameters to imageapi_image_crop, but for ease of releasing these two independent modules it seems best to put in just imagecache for now.
@ambo - it doesn't really matter what "editorial" says. This is an enhancement to the capabilities of the module so it is a feature. Arguing with the maintainer about "bug vs. feature" is unlikely to make them work faster and quite likely to make them frustrated.
The patch is against 6.x-2.0-beta10 from an svn repository, but it applies cleanly to the current 6.x-2.x-dev.
Comment #8
gregglesAnd a new patch with better t strings messages, more comments, and with isset it is now more e_notice compliant and keeps the default behavior exactly the same.
Comment #9
gregglesTypo in the comments. "check for is" vs. "check for isset"
Comment #10
ysjwang CreditAttribution: ysjwang commentedThanks, this patch worked. Awesome job!
Comment #11
PixelClever CreditAttribution: PixelClever commentedThis patch works perfectly, and in my opinion is very important for usability. +1 for getting it into an upcoming release.
Comment #12
YK85 CreditAttribution: YK85 commentedsubscribing
Comment #13
joeyabbs CreditAttribution: joeyabbs commentedsubscribing
Comment #14
osopolarThank you greggles for the patch in #9, works as expected.
Comment #15
fizk CreditAttribution: fizk commentedPlease reopen if this is still an issue with ImageCache 6.x-2.0-rc1.
Comment #16
gregglesYes, still an issue.
Comment #17
fizk CreditAttribution: fizk commentedMarking as ImageCache 3.x Todo.
Comment #18
osopolarState before change in #15 was and still should be: "reviewed & tested by the community".
Comment #19
c4rl CreditAttribution: c4rl commentedLooks good to me. Bump? :)
Comment #20
c4rl CreditAttribution: c4rl commentedHere's a Git-friendly version of the patch from #9
Comment #21
yannickooYou should remove all the whitespaces.
Comment #22
c4rl CreditAttribution: c4rl commentedOkay
Comment #23
dargno CreditAttribution: dargno commentedAny possibility to put this in Drupal7 core as well? Was kinda astonished to see this not work like described here actually :O
Comment #24
gregglesIt could become a drupal 8 core feature, but probably not Drupal 7 since it is a new feature.