Comment #142

Problem/Motivation

When creating an Image Style that applies the image_crop effect with one or both dimensions exceeding the source dimensions, the new area is rendered in black. Site administrators should be able to configure the color of the new area being created. This should be a per-style configuration, NOT a global one. Site administrators may have styles that integrates with light layouts and there, they want white background for cropped image. Other style may be necessary for dark pages and there they may want heavy-gray background color.

Proposed resolution

Adding a background color at effect level for image_crop.

Remaining tasks

N/A

User interface changes

When configuring a Crop (image_crop) effect at admin/config/media/image-styles/manage/{image_style}/add/image_rotate or admin/config/media/image-styles/manage/{image_style}/effects/{image_effect} admins should be able to provide a color, in hex string format, for the additional area created by upscaling crops.

API changes

Note: These API changes assure full backward compatibility.

core/includes/image.inc

image_crop() has 2 additional optional arguments:

  1. $background_color, defaults to NULL.
  2. $background_alfa, defaults to 0.

image_rotate() was unified to the same logic.

Before:

function image_crop($image, $x, $y, $width, $height)
function image_rotate($image, $degrees, $background = NULL)

Now:

function image_crop($image, $x, $y, $width, $background_color = NULL, $background_alpha = 0)
function image_rotate($image, $degrees, $background_color = NULL, $background_alpha = 0)

\Drupal\system\Plugin\ImageToolkitInterface

Applied to \Drupal\system\Plugin\ImageToolkit\GDToolkit implementation too.

Crop & rotate public methods have adapted their argument list in the same way.

New \Drupal\image\ImageEffectBackgroundColorBase abstract

Now CropImageEffect and RotateImageEffect are extending the new ImageEffectBackgroundColorBase:

<?php
abstract class ImageEffectBackgroundColorBase extends ImageEffectBase implements ConfigurableImageEffectInterface {
  ...
}
?>

This new base class adds some code reusing for effects with background color.

Original report by FiReaNG3L

ImageAPI.module sounds very interesting; I recently had problems with imagecache / image.inc putting a black background on images when scaling / cropping to a definite size (say 150x150 pixels) when the image is too wide or tall to fit the square.

Is there a way to customize the background to another color (say, white) with image API?

I tried to modify image.inc myself but the best I could achieve is a background change that worked, but bleeded in the image if black was present.

Thanks for all the awesome work on imagecache / imageAPI!

Files: 
CommentFileSizeAuthor
#141 drupal-image-crop-204497-141.patch29.25 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-image-crop-204497-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#138 drupal-image-crop-204497-138.patch27.91 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#140 drupal-image-crop-204497-140.patch27.91 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 56,729 pass(es).
[ View ]
#130 drupal8.crop-background-204497-130.patch19.73 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 34,271 pass(es).
[ View ]
#124 drupal8.crop-background-204497-124.patch19.71 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 34,270 pass(es).
[ View ]
#122 drupal8.crop-background-204497-122.patch19.62 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 34,275 pass(es).
[ View ]
#120 drupal8.crop-background-204497-120.patch19.56 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 34,270 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#116 drupal8.crop-background-204497-116.patch19.95 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 34,234 pass(es).
[ View ]
#114 drupal8.crop-background-204497-114.patch18.04 KBdman
PASSED: [[SimpleTest]]: [MySQL] 34,122 pass(es).
[ View ]
#112 drupal8.crop-background-204497-112.patch18.59 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 34,591 pass(es).
[ View ]
#109 drupal8.crop-background-204497-109.patch17.39 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 34,568 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#106 drupal8.crop-background-204497-106.patch16.88 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 34,400 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#97 background.jpg62.79 KBRaulMuroc
#94 bg-white-large.png208.81 KBjantoine
#94 bg-blue-large.png208.8 KBjantoine
#94 bg-green-large.png208.8 KBjantoine
#94 bg-red-large.png208.8 KBjantoine
#94 bg-black-large.png208.8 KBjantoine
#94 bg-none-large.png208.25 KBjantoine
#92 drupal8.crop-background-204497-92.patch12.52 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 33,825 pass(es).
[ View ]
#91 drupal8.crop-background-204497-91.patch11.76 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 33,821 pass(es).
[ View ]
#88 drupal8.crop-background-204497-88.patch11.75 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 33,816 pass(es), 1 fail(s), and 4 exception(es).
[ View ]
#86 drupal8.crop-background-204497-86.patch11.68 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 33,830 pass(es).
[ View ]
#83 drupal8.crop-background-204497-83.patch6.64 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 33,800 pass(es).
[ View ]
#74 drupal8.image-gd-crop-background.74.patch1.92 KBsun
PASSED: [[SimpleTest]]: [MySQL] 32,173 pass(es).
[ View ]
#37 imageapibackground.patch2.03 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapibackground.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#36 pgmatch.gif6.91 KBmorningtime
#36 pgmatch_7.gif1.84 KBmorningtime
#35 imageapi_background_color.patch1.9 KBedmund.kwok
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi_background_color.patch.
[ View ]
#26 204497.patch2.42 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 204497_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#24 204497.patch2.19 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 204497.patch.
[ View ]
#24 cropbackground.png37.73 KBRobLoach
#22 imageapi-transparent_backgrounds_02.patch3.58 KBtylor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-transparent_backgrounds_02.patch.
[ View ]
#19 imageapi-transparent_backgrounds-20081009.patch3.57 KBdman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-transparent_backgrounds-20081009.patch.
[ View ]
#17 imageapi-5.x-1.2_crop_background_1.patch4.35 KBMurz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-5.x-1.2_crop_background_1.patch.
[ View ]
#13 imageapi-5.x-1.1_crop_background.patch2.58 KBMurz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-5.x-1.1_crop_background.patch.
[ View ]
#9 image_crop_transparent_1.patch1.27 KBMurz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_crop_transparent_1.patch.
[ View ]
#8 Sweetheart-Leather.jpg10.9 KBidflorin

Comments

Small followup: a search on d.o. reveals that many users have the same problem; maybe ImageAPI.module would be a nice place to fix this since they are mostly related to imagecache.module and the new version isnt using image.inc anymore!

Non-extensive list:

Imagecache Scale Problem (Black background) : http://drupal.org/node/201163
Background in cropped images : http://drupal.org/node/123270
Changing Background Color of Thumbnails Re: CCK/VIews Image Gallery : http://drupal.org/node/190235

All the workarounds thus far involve doing some other action to avoid the crop, but if thats the result you want and nothing else, you're stuck with a black background, which is ugly if your theme is white based.

Also image_exact.module (http://drupal.org/project/image_exact) states in its description : The only limitation of this implementation is that imagecache doesn't check the image size before cropping, so if someone uploads a very small image, it will "crop" to the size specified, adding a black background to fill in the dimensions. I will submit a patch to imagecache that will make this configurable.

And here's a possible fix:

Here is a Patch to avoid black backgrounds on cropped images : http://drupal.org/node/199957

I'd really prefer you test the module rather than post feature requests right now. I would also be nice if you contributed this image.inc fix you attempted as a possible patch instead of just mentioning it. Code is gold and all..

As I said, there's a potential fix in the last issue I linked to - image.inc should be patched, but imageAPI too because its the future of imagecache, which is the main resource using image manipulation. I will test the patch as soon as I get back home (christmas vacation and all). This issue is just to document the problem as a feature request against the dev version, so its not like the house is on fire or something :)

I have successfully change background color when cropping with easy hack of Drupal core file includes/image.inc:
In function

<?php
function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
 
imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
 
$result = image_gd_close($res, $destination, $info['extension']);
...
}
?>

add string:
<?php
function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
 
imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
 
imageFill($res, 0, 0, imageColorAllocate($res,255,255,255) );
 
$result = image_gd_close($res, $destination, $info['extension']);
...
}
?>

When I try to add imageFill function before imageCopy, i see black backgrond. But when I add it after imageCopy, all works good.

Numbers 255,255,255 you can change to any RGB color values as you want.

Here is a Patch to avoid black backgrounds on cropped images : http://drupal.org/node/199957

This issue doesn't solve problem with black backgrounds, it's avoid clipping if size of clip area large than image.

Another (better) method to fill background with some color:
In function

<?php
function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
 
$res = imageCreateTrueColor($width, $height);
 
imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
 
$result = image_gd_close($res, $destination, $info['extension']);
...
}
?>

add string:

<?php
function image_gd_crop($source, $destination, $x, $y, $width, $height) {
...
 
$res = imageCreateTrueColor($width, $height);
 
imagefilledrectangle($res, 0, 0, $width, $height, imagecolorallocate($res,255,255,255) );
 
imageCopy($res, $im, 0, 0, $x, $y, $width, $height);
 
$result = image_gd_close($res, $destination, $info['extension']);
...
}
?>

I've tried this, and not been able to make it work... Nothing at all happens... Any idea as to what I could be doing wrong? I'm using Imagecache 1.3 on Drupal 5.6.

I tried the other hack you posted, and that worked like a charm - that is, until I uploaded an image consisting mainly of black... Apparently, imageFill doesn't handle alpha-colors too well...

Nevertheless; if you (or anyone else for that matter) have any more ideas on how to make this work, it would be greatly appreciated! It's such a bummer going back to the black background images after having it "working" sweetly for a while ;)

Thanks
joeboris

StatusFileSize
new10.9 KB

#4 works partially (see attachment)
#6 no change.
Now I'll try both.
--------------------------
Velvet Cushion

StatusFileSize
new1.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_crop_transparent_1.patch.
[ View ]

adresaklumea, I create another method for filling background when cropping, it should be work good with half -transparent png's too. Try my new patch and feedback please about it works.
You must set background color as HTML color in string

<?php
     $bg_color
= variable_get('image_crop_background', '#FF0000');
?>

or set a Drupal variable 'image_crop_background' to HTML color you need.
This patch is for Drupal 5.7 core, not for ImageAPI module.

Beautiful, works like a charm
10*e99 thanks

How to patch the ImageAPI ?

I tried the patch in #9 but my whole site goes down and I get the following error messages:

Warning: require_once(./includes/image.inc) [function.require-once]: failed to open stream: Permission denied in drupal\includes\common.inc on line 1850

Fatal error: require_once() [function.require]: Failed opening required './includes/image.inc' (include_path='.;C:\php5\pear') in drupal\includes\common.inc on line 1850

Which is really strange I don't know where this comes from? I am using the 5.7 Drupal.

StatusFileSize
new2.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-5.x-1.1_crop_background.patch.
[ View ]

This is a patch for imageapi to set the background color for cropping.

At now you can set the color manually in source code by modifying this string:

<?php
$color
=array('red'=>255,'green'=>255,'blue'=>255,'alpha'=>0);
?>

And this patch have a workaround with wrong image size for function imagecopyresampled() when cropping.

Thank you again.
you rock!

Status:Active» Needs work

This still isn't quite the right solution... I'm tempted to split canvas resize and crop into seperate functions and not allow oversized crops.... and have an explicit set background for canvas resizing that inherits from the image->res if unset.

regardless this breaks background transparency preservation in it current state.

.darrel.

Subscribing,

The patch in #9 works for me, using Imagecache 5.x-1.6 and Drupal 5.9. FWIW I use scaling to outside dimensions followed by cropping slightly smaller. The background now blends nicely.

I assume the plan is to make the string value assignable in the image toolkit admin page?

Something along the lines of

<?php
function image_gd_settings() {

$form
['image_crop_background'] = array(
 
'#type' => 'textfield',
 
'#title' => t('Background color'),
 
'#description' => t('Define the image background color for manipulations. Values need to be hexdecimal, eg #FFFFFF is white.'),
 
'#size' => 7,
 
'#maxlength' => 7,
 
'#default_value' => variable_get('image_crop_background', "#FFFFFF"),
  );

}
?>

A couple of random thoughts…
The default should be probably white, since it would sit well with default theme(s).
The # should be removed from the field and only the 6 characters are saved for the color variable, eg ff00ff I think we can use a field prefix to display the # at the start of the field.

@ dopry, I didn't realize there was transparency, my images seem to end up with a black background when cropping oversize. Is there anything I can do, this strikes me as a nice feature to get into core, I'd easily use it on all my sites.

Drew

StatusFileSize
new4.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-5.x-1.2_crop_background_1.patch.
[ View ]

I have edited the patch for support imageapi-5.x-1.2 and add an option in imageapi_gd settings to set background color manually. It's worked for me and, I hope, help others.

Version:5.x-1.x-dev» 6.x-1.x-dev

this would really need to go into HEAD (6.x-1.x) and be backported.

Title:Background color when scaling / cropping - customizable?Background color when scaling / cropping. And a fix for transparent backgrounds.
Status:Needs work» Needs review
StatusFileSize
new3.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-transparent_backgrounds-20081009.patch.
[ View ]

Looking at #7, It's wrong to use variable_set/get for this which should be a per-preset setting.

Anyway...

I've tweaked imageapi to :
- Properly support transparent inputs - it just wasn't doing it before. Now if you crop a PNG or GIF the background will remain transparent - even if cropped to a larger size. There was code there to support this but it seems to have rotted away at some point and stopped doing it properly.
- Allow crop to larger dimensions : behave like "set canvas size" - without creating huge black chunks. Jpegs still create white chunks, but that's less ugly usually.

It involved moving jpeg handling over next to png handling (truecolor), as the previous index transparency only works on GIFs AFAIK.

In the future, setting the preferred bg color via the UI is still a TODO

Patch is against todays HEAD/6-dev

Index: imageapi_gd.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/imageapi/imageapi_gd.module,v
retrieving revision 1.12
diff -u -p -r1.12 imageapi_gd.module
--- imageapi_gd.module 9 Jul 2008 02:49:47 -0000 1.12
+++ imageapi_gd.module 9 Oct 2008 07:28:38 -0000
@@ -32,6 +32,10 @@ function imageapi_gd_settings_form() {
function imageapi_gd_image_open($image) {
   if ($image->res =  _gd_open($image->source, $image->info['extension'])) {
+    // Retain transparency for PNGs
+    if ($image->info['extension'] == 'png') {
+      imagesavealpha($image->res, TRUE);
+    }
     return $image;
   }
   return false;
@@ -44,7 +48,11 @@ function imageapi_gd_image_close($image,
function imageapi_gd_image_crop(&$image, $x, $y, $width, $height) {
   $res = _gd_create_tmp($image, $width, $height);
-  if (!imagecopyresampled($res, $image->res, 0, 0, $x, $y, $width, $height, $width, $height)) {
+  // Support 'cropping' to a size larger than the source - but do not copy null values into new canvas - so PNGs and GIFs can remain transparent.
+  $targetwidth = min($image->info['width'], $width);
+  $targetheight = min($image->info['height'], $height);
+
+  if (!imagecopyresampled($res, $image->res, 0, 0, $x, $y, $targetwidth, $targetheight, $targetwidth, $targetheight)) {
     return false;
   }
   // destroy the original image and return the modified image.
@@ -134,7 +142,7 @@ if (!function_exists('imagefilter')) {
function _gd_create_tmp($image, $width, $height) {
   $res = imagecreatetruecolor($width, $height);
-  if ($image->info['extension'] == 'gif' || $image->info['extension'] == 'jpg') {
+  if ($image->info['extension'] == 'gif') {
     // grab transparent color index from src.
     $transparent = imagecolortransparent($image->res);
@@ -142,22 +150,23 @@ function _gd_create_tmp($image, $width,
     if ($transparent >= 0) {
       // get color(r,g,b) for index;
       $transparent = imagecolorsforindex($image->res, $transparent);
-      // allocate to new image and get new index.
-      $transparent =  (isset($color['alpha']))
-        ? imagecolorallocatealpha($res, $color['red'], $color['green'], $color['blue'], $color['alpha'])
-        : imagecolorallocate($res, $color['red'], $color['green'], $color['blue']);
+      // Allocate the values to the new image and get new color.
+      $transparent =  (isset($transparent['alpha']))
+        ? imagecolorallocatealpha($res, $transparent['red'], $transparent['green'], $transparent['blue'], $transparent['alpha'])
+        : imagecolorallocate($res, $transparent['red'], $transparent['green'], $transparent['blue']);
-      $transparent = imagecolorallocate($res,  $transparent['red'], $transparent['green'],  $transparent['blue']);
       // flood with our new transparent color.
       imagefill($res, 0, 0, $transparent);
       // tell the new image which color is transparent.
       imagecolortransparent($res, $transparent);
     }
   }
-  elseif ($image->info['extension'] == 'png') {
+  elseif ($image->info['extension'] == 'png' || $image->info['extension'] == 'jpg') {
+    imagesavealpha($res, true);
     imagealphablending($res, false);
-    $transparency = imagecolorallocatealpha($res, 0, 0, 0, 127);
-    imagefill($res, 0, 0, $transparency);
+    $transparency = imagecolorallocatealpha($res, 255, 255, 255, 127);
+    #imagefill($res, 0, 0, $transparency); // imagefill doesn't always work with transparent apparently
+    imagefilledrectangle($res, 0, 0, $width, $height, $transparency);
     imagealphablending($res, true);
     imagesavealpha($res, true);
   }

Input on patch #19...

If you set the X offset to right than a black background is added to the left and the image is cropped.

Steps to reproduce:

  • Source image: 100px x 100px
  • Add a preset with a CROP action
  • Set the Crop action to...
    • With: 120px
    • Height: 120px
    • X offset: right
    • Y offset: top

The image should be surround at left & bottom by a white background. At bottom is OK but on X axis it adds white background to right and black background to left. Also it reduces the X dimension by cropping the image.

But... it works fine with X offset set to left

StatusFileSize
new3.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi-transparent_backgrounds_02.patch.
[ View ]

Input for #19 and #20, I had the same problem but found I could make everything work (left, right, and center) by changing:

<?php
if (!imagecopyresampled($res, $image->res, 0, 0, $x, $y, $targetwidth, $targetheight, $targetwidth, $targetheight))
?>

to

<?php
if (!imagecopyresampled($res, $image->res, 0, 0, $x*(-1), $y*(-1), $targetwidth, $targetheight, $targetwidth, $targetheight))
?>

Attached is a new patch with this change.

For CENTRING I have applied patch from #22 and change one row in function imageapi_gd_image_crop from

<?php
if (!imagecopyresampled($res, $image->res, 0, 0, $x*(-1), $y*(-1), $targetwidth, $targetheight, $targetwidth, $targetheight))
?>

to

<?php
  
if (!imagecopyresampled($res, $image->res, ($width-$image->info['width'])/2-$x, ($height-$image->info['height'])/2-$y, ($x)*(-1), $y*(-1), $targetwidth, $targetheight, $targetwidth, $targetheight)) {
?>

and all work fine for me)
I use drupal 6

Priority:Normal» Critical
StatusFileSize
new37.73 KB
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 204497.patch.
[ View ]

How's this? Moving this to critical since it's been an issue for so long. Here's a patch as well as a screenshot.

The patch was created from HEAD.

I'd like some extra comments so it's clear what's happening there. why are/were we passing the $width, $height parameters if we're using $image->info['height'], $image->info['width']?

StatusFileSize
new2.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 204497_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Here we go. This patch says why we're using the source width and height instead of the destination width and height. Think we should remind people that we're cropping here and not scaling? Or is that good? I thought it made sense.....

Status:Needs review» Needs work

the comments look better... but looking at the docs for imagecopyresampled() it references them as the src and dest sizes so wouldn't the first set be $height and $width and the second be $image['height'] and $image['width']?

this has me really wishing we had some unit tests to validate some of these type of changes.

and if we're adding a feature we need to add it to the imagemagick api or at least determine that it's unsupported by IM...

I've been wishing for unit tests for my image processes also ... but I just cannot imagine how to validate that the produced image is what I expected :-)

dman: maybe check the output / md5 with http://ca.php.net/function.file-get-contents compared to an 'expected' image from a source image

Byte/hash matching won't really help for JPEG algorithms against different builds, libraries, image toolkits, compression preferences, OS etc.
:-(

Andrew: We're using the same width and height for both the source and destination because we are cropping, not scaling. If we were scaling, we'd use different source/destination widths and heights.

Status:Needs work» Fixed

that makes sense. okay this looks pretty good to me. committed to HEAD. thanks!

Yeah, should've included that as part of the patch with the added documentation. Thanks a lot, Drewish.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Priority:Critical» Normal
Status:Closed (fixed)» Needs review
StatusFileSize
new1.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapi_background_color.patch.
[ View ]

This patch is not in the latest 1.x-dev? Updated patch for this.

StatusFileSize
new1.84 KB
new6.91 KB

Above patch works for png and jpg, but not for gif images. Im scaling images to fit e.g. 100x100, and then cropping to 100x100. See attached gif images problem: only the top part gets the #FFF background I set in the ImageAPI config page. The bottom part is still black. Why doesnt it apply the set background color for the entire image, why only the top part?

StatusFileSize
new2.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch imageapibackground.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Updated to latest in DRUPAL-6--1.

Subsc

The #37 patch works for me. Thanks a lot.

Subscribing

subscribing

Did not work for me.

subscribing

wouldn't it be nice to check image dimensions rather than filling it with whatever background on crop?

Thanks

#37 patch worked for me in 1.8.

very nice! thank you.

it probably says this on the thread somewhere, but you can modify color background by editing line #116 in the patched file. for example:

$background = variable_get('imageapi_crop_background', '#cccccc');

Status:Needs review» Active

Hello, I can confirm that the patch in #37 is working!

Will this be committed, what's the status on this issue?

Greetings Wappie

Status:Active» Reviewed & tested by the community

I can also confirm that patch #37 works perfectly with ImageAPI 6.x-1.8 and ImageCache 6.x-2.0-beta10.

@#46 instead of coding the color into the code you can go into the ImageAPI configuration screen and set the color in there.

And don't forget to flush the cache to see the changes. :)

...

marked rtbc

@#49 oh, yes. that's much easier. thanks.

#49 john_kenney - June 22, 2010 - 20:35
@#49 oh, yes. that's much easier. thanks.

:))) Umm, talking to yourself there? ;)

right. i meant #48.

been a long day... :)

Confirm...the patch works fine...thanks a lot...

Version:6.x-1.x-dev» 6.x-1.6

#37 worked for me also on 1.6

#37 works as expected. Please commit this ASAP

#37 works very good for me as well and I agree - please commit this ASAP!

Works great here. @#43, if it's not working for you please explain how.

subscribing

Could this please be commited to HEAD (If it has been committed could the maintainer close this issue)? There seem to be enough positive community reviews here.

I second the motion to commit crop background color patch added to HEAD.

works..

please commit this longstainding functionality.

Version:6.x-1.6» 6.x-1.x-dev

Herp derp? Should we push this to 7.x at this point? Might need an update to DRUPAL-6--1?

Patch in #37 works for setting bg color. Not sure about the transparent bg part.

Are there outstanding issues with this patch? Or are we now abandoned because our sites are on Drupal 6?
This has been fixed since October 2009 (post 37), and it still isn't showing up in the dev or the 6.x-1.9 releases.

What about ImageCache actions, Define Canvas action does this pretty good out of the box.

I manually patched dev and it worked great, thanks

#37 works as expected. Please commit.

Status:Reviewed & tested by the community» Fixed

Committed to 6.x-1.x

I converted the change for drupal 7, do not know if there is another issue for it so please delete or move if needed. Here are some notes:

  • The file to change is image.gd.inc in /modules/system/
  • To keep the same language, the name of the field should be 'image_crop_background'
  • drupal contrib shows that there is imageapi_hex2rgba, I could not find it. in order to make it work, I added the function at the end of the file.
  • A suggestion - Add the background color to the crop form in the styles admin (image.admin.inc). In order to do it there will be a need for a change in the way that you send variables to image gd. It could be better to send an array with options than a different variable to every definition (less changes in every code enhancement, function definitions stay the same).
  • There is no validation, i added a validation to check if the number entered is hexadecimal.

my code:

<?php
function image_gd_settings() {
...
...
$form['image_crop_background'] = array(
   
'#type' => 'textfield',
   
'#title' => t('Crop background'),
   
'#description' => t('Hex string specifying the background color to use when cropping images. If not provided, will use the default. Examples: "ABC", "ABCD", "AABBCC", "AABBCCDD".'),
   
'#size' => 10,
   
'#maxlength' => 8,
   
'#default_value' => variable_get('image_crop_background', ''),
   
'#field_prefix' => '#',
    );
...
?>

<?php
function image_gd_settings_validate($form, &$form_state) {
 
// Validate image quality range.
 
$value = $form_state['values']['image_jpeg_quality'];
  if (!
is_numeric($value) || $value < 0 || $value > 100) {
   
form_set_error('image_jpeg_quality', t('JPEG quality must be a number between 0 and 100.'));
  }
 
$value = $form_state['values']['image_crop_background'];
  if (!(
$value=='')&&!ctype_xdigit ($value)){
   
form_set_error('image_crop_background', t('Value entered is not hexadecimal.'));
  }
}
?>

<?php
function image_gd_crop(stdClass $image, $x, $y, $width, $height) {
 
$res = image_gd_create_tmp($image, $width, $height);
 
// Fill the background color if desired.
 
$background = variable_get('image_crop_background');
  if (!empty(
$background)) {
   
$background = imageapi_hex2rgba($background);
   
$background = imagecolorallocatealpha($res, $background[0], $background[1], $background[2], $background[3]);
   
imagefill($res, 0, 0, $background);
  }
 
// Copy the source image to our new destination image. We use
  // $image->info['width] instead of $width because we are copying
  // using the source image's width and height, not the destination
  // width and height.
 
if (!imagecopyresampled($res, $image->resource, -$x, -$y, 0, 0, $image->info['width'], $image->info['height'], $image->info['width'], $image->info['height'])) {
    return
FALSE;
  }
...
?>

link to the imageapi_hex2rgba function

Project:ImageAPI» Drupal core
Version:6.x-1.x-dev» 7.x-dev
Component:ImageAPI GD» image system
Status:Fixed» Needs review

Hi ymeiner, thanks very much for this code, I can confirm that this works!

one thing: your link to imageapi_hex2rgba isn't working, this should be: http://drupalcontrib.org/api/drupal/contributions--imageapi--imageapi.mo...

I moved this issue to drupal core as this is now in core, maybe someone can comment on the need for this to be in d7, I personally think that this should be an option one should be able to set!

Greetings Wappie

Status:Needs review» Needs work

The last submitted patch, imageapibackground.patch, failed testing.

Version:7.x-dev» 8.x-dev
Category:feature» bug

Into Drupal 8.x before hitting Drupal 7.x.

Version:8.x-dev» 7.2
Category:bug» feature

This is a great, simple, and useful feature. Is there a formal request in to have this included in the next build of D7?

Version:7.2» 8.x-dev
Issue tags:+needs backport to D7

Need in D8 first, then can backport to Drupal 7.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
PASSED: [[SimpleTest]]: [MySQL] 32,173 pass(es).
[ View ]

Re-rolled against D8.

subscribe

subscribe for 7.x

#26: 204497.patch queued for re-testing.

I just applied this to Drupal 7.8. It works great.

Only issue is see is that the "Crop background color " hex string value you enter in the form at admin/config/media/image-toolkit is not displayed after saving. That hex string you enter should be displayed when you return to admin/config/media/image-toolkit in the future.

For anyone looking for a quick solution (D7) to this problem: the Imagecache Actions project provides a "Define canvas" action which essentially works the same way as a "crop", but also allows to define a background color.

#37: imageapibackground.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Assigned:claudiu.cristea» Unassigned
Status:Needs review» Needs work

With all respect to the work that has been done here, my believe is that this is NOT the right approach. This patch let site admins setting a global background color which is not correct. You may have styles/presets that are integrated on a layout with white background - there you want a white background for cropped image. Other style/preset may be placed on a dark page - there you want other background color. Et cetera...

The point is that background image when cropping must be a per-style setting and NOT a global setting.

EDIT: Just like when rotating.

Assigned:Unassigned» claudiu.cristea
Status:Needs work» Needs review
StatusFileSize
new6.64 KB
PASSED: [[SimpleTest]]: [MySQL] 33,800 pass(es).
[ View ]

Here's patch made in the idea of #82.

Just a note related to #74. That patch uses a function imageapi_hex2rgba() that is not part of the core. I wonder how the patch passed the automated tests :)

+++ b/modules/system/image.gd.inc
@@ -185,9 +195,21 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
+    $background = imageapi_hex2rgba($background);

In the proposed patch I added that function to core (image.inc) because I saw that we can reuse that code, for example in image_gd_rotate() where is something similar.

The approach in #83 is interesting. Thanks for the patch.

If it passed automated tests with a function that does not exist, that indicates to me that we are missing test coverage for it. Also, let's get some manual testing with #83. Screenshots and an issue summary would be helpful too. :)

Status:Needs work» Needs review

Oops, let's leave at NR now for more patch reviews and testing.

Title:Background color when scaling / cropping. And a fix for transparent backgrounds.Configurable background color when cropping to larger dimensions
Issue tags:-Needs tests, -Needs issue summary update, -Novice, -Needs manual testing
StatusFileSize
new11.68 KB
PASSED: [[SimpleTest]]: [MySQL] 33,830 pass(es).
[ View ]

I already worked on tests :) Added Issue summary and automatic tests.

Issue summary:View changes

Updated issue summary.

Thanks @claudiu.cristea! I read through the patch again and I have a few small suggestions.

+++ b/core/includes/image.incundefined
@@ -438,5 +444,53 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Convert a hex string to its RGBA (Red, Green, Blue, Alpha) integer
+ * components.

Let's try to shorten this slightly so it fits on one 80-character line. How about, "Converts a hex string to RGBA (Red, Green, Blue, Alpha) integer values."

+++ b/core/includes/image.incundefined
@@ -438,5 +444,53 @@ function image_save(stdClass $image, $destination = NULL) {
+ * @return
+ *   An array with four elements for red, green, blue, and alpha.
...
+  return array($r, $g, $b, $a);

Let's use associative keys for these similar to those used elsewhere ('red', etc.)

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -458,3 +458,130 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
+ * Test if cropping to larger dimensions add
+ * the correct color in the uncovered area.

Let's change this to something like "Tests image cropping functionality." (That way it's a good container for additional tests that could use this setup.) In general, function/class/method summaries should be a single line, 80 chars or less, and begin with a verb in the 3rd person present (e.g. "Tests"). (Reference: http://drupal.org/node/1354#functions)

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -458,3 +458,130 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
+   * Testing crop backgound color.

For this one, how about: "Tests that image cropping uses the correct background color."

StatusFileSize
new11.75 KB
FAILED: [[SimpleTest]]: [MySQL] 33,816 pass(es), 1 fail(s), and 4 exception(es).
[ View ]

OK. Attached together with other small changes.

Status:Needs review» Needs work

The last submitted patch, drupal8.crop-background-204497-88.patch, failed testing.

Ah, looks like the test needs a little adjustment:

Undefined variable: r Notice image.inc 485 image_hex2rgba()
Undefined variable: g Notice image.inc 486 image_hex2rgba()
Undefined variable: b Notice image.inc 487 image_hex2rgba()
Undefined variable: a Notice image.inc 488 image_hex2rgba()
Pixel from (5, 5) is red (#FF0000) Other image.test 583 ImageCropBackgroundTestCase->testCropBackgroundColor()

Status:Needs work» Needs review
StatusFileSize
new11.76 KB
PASSED: [[SimpleTest]]: [MySQL] 33,821 pass(es).
[ View ]

Bypassing local tests it's a bad behavior :-)

StatusFileSize
new12.52 KB
PASSED: [[SimpleTest]]: [MySQL] 33,825 pass(es).
[ View ]

Some comments and typo fixes.

Code in #92 looks great to me.

Now let's get a couple different people doing that manual testing:

  1. Apply #92
  2. Create various image styles using the new background color property
  3. Verify that images are created correctly.
  4. Report your results here. Screenshots would be good (but crop them to only show the relevant part of the screen).

StatusFileSize
new208.25 KB
new208.8 KB
new208.8 KB
new208.8 KB
new208.8 KB
new208.81 KB

Wow... I have got to say, great work!!!

Rather than providing screen shots, I have attached the actual images that were generated. My only questions is should this be added to the "Scale and Crop" effect as well?

Attached are images that were generated using the following background color values:

#000000
#FF0000
#00FF00
#0000FF
#FFFFFF

should this be added to the "Scale and Crop" effect as well?

@AntoineSolutions, "Scale and Crop" doesn't need it while every time is cropping to a smaller area. "Scale and Crop" never creates additional new area. At least this is my understanding.

@claudiu.cristea, You are correct. Not sure what I was thinking. In light of that, I would like to confirm that the patch is working as expected. Not sure how many tests are needed before RTBC, so I'll hold off on updating the status.

StatusFileSize
new62.79 KB

"Scale and Crop" doesn't need it while every time is cropping to a smaller area. "Scale and Crop" never creates additional new area. At least this is my understanding.

But with "Scale & Smart Crop" + Canvas the problem exists. For instance, I have defined to Scale & Crop to 540x340 and added Canvas background #969696 center,center to it and it returns a black background where it should be gray.

I tried to solve it without success.

Thx.

@RaulMuroc, we are not dealing here with "Canvas" effect.

#82 Claudiu -- Absolutely. Second that!

Gábor

P:S: thank you all for the work.
BTW subscribe for D7

Looks awesome! Subscribe for D7

You don't need to "subscribe" anymore. There's a green "Follow" button in the upper-right corner of the issue. :)

Status:Needs review» Needs work

The form element isn't properly validated. Maybe we could even make it show the color, like the color widgets do, when modifying a theme?

Status:Needs work» Needs review

That is not in the scope. It may be an improvement, subject of a new ticket. Right now the rotate effect is collecting the background in the same way. If works for rotate than must work also for crop.

While "Maybe we could even make it show the color, like the color widgets do, when modifying a theme?" is optional, the other point isn't: "The form element isn't properly validated."

We need to check that. (And the rotate effect checks it, too.) Try, for example #zzzzzz as the color code.

Status:Needs review» Needs work

@Niklas Fiekas, you're right. But we need to unify the way how rotate and crop are validating colors. Right now, rotate is not accepting colors like #RGB but only #RRGGBB.

Status:Needs work» Needs review
StatusFileSize
new16.88 KB
FAILED: [[SimpleTest]]: [MySQL] 34,400 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

Here's the patch with color validation. Note that now crop & rotate background colors use the same validation mechanism and storage.

Status:Needs review» Needs work

The last submitted patch, drupal8.crop-background-204497-106.patch, failed testing.

@claudia.cristea: Yes, I'd really like a unified color widget in core. But you're right. Probably we shouldn't block this issue on that. We can clean up that later.

Tests failing now?

StatusFileSize
new17.39 KB
FAILED: [[SimpleTest]]: [MySQL] 34,568 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

Tests failed because were not adapted after rotation color validation changes. Fixed now.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.crop-background-204497-109.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new18.59 KB
PASSED: [[SimpleTest]]: [MySQL] 34,591 pass(es).
[ View ]

Fixed tests...

Status:Needs review» Needs work

I can confirm that patch #112 applies well and fixes the (original) described problem.
The UI for color selection works fine (though is not fancy). Includes Alpha, seems OK.
Validation seems to work.

I find it *not perfect* that is allows either '#ffffff' or 'ffffff' as values.
I think it SHOULD *allow* either input, so that's good. But I'd prefer it for consistency if it either always prepended or always truncated the # when saving. It leaves the "what is the correct syntax" question open to interpretation.
For example, someones screenshot may differ from someone elses settings (one with #, one without) and that can look like a source of error. When it's not really.

I suggest that the validation hook should correct and rewrite the input early on. Also probably putting the hex to uppercase ... to reduce inconsistencies in the UI. I don't want this to be getting into the color widget feature request question again, but I think we can do this right here and now. To me this is a validation issue for UX.

I wanted to flag this 'reviewed' ... but now I'm wondering if in light of this UI inconsistency, perhaps I've gotta 'needs work' on it. Almost there. I'll try a re-roll with my suggestion

Status:Needs work» Needs review
StatusFileSize
new18.04 KB
PASSED: [[SimpleTest]]: [MySQL] 34,122 pass(es).
[ View ]

I'm just putting this here as a suggestion. If nobody likes the idea, then call #112 'reviewed'

This patch (a tiny adjustment to #112) means that the color values will be tidied consistently if you enter :
"#123456" => "123456"
"#abcdef" => "ABCDEF"
"abcdef" => "ABCDEF"

Personally, I'd prefer to ALWAYS have # in the value, but the UI currently places a # outside the form element. And that looked odd.

If we were to make this improvement, I'd maybe think about instead running
image_rgba2hex(image_hex2rgba($value));
as that sort of approach always has the side effect of fixing inconsistent formats like this.
But for now, it's just string fixes.

Status:Needs review» Needs work

While this patch will be backported to Drupal 7, my suggestion is to keep the existing color storage format used now for rotate effect. In this way existing sites will not need any upgrade path - no hook_update_N(). So, you're right, we need store the value prefixed with '#'.

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new19.95 KB
PASSED: [[SimpleTest]]: [MySQL] 34,234 pass(es).
[ View ]

This patch is fixing the input and backend for the color hex string as follows:

  1. The user may enter '#' as color prefix. The #field_prefix has been removed.
  2. Alphabetic characters may be entered as lower or upper case.
  3. The argument of image_hex2rgba(&$hex) is passed now by reference and the function is tidying up the $hex string resulting a consolidated value, prefixed by '#' and with uppercase chars. Examples:
    • #fa89aE => #FA89AE
    • FA89AE => #FA89AE
    • fa89aE => #FA89AE
  4. The color is stored prefixed by '#' and with uppercase chars. This assures compatibility with the D7 existing image_rotate color storage format and will make no need for an update path on backport.

Status:Needs review» Needs work

Ok. Since we're down to nitpicking:

+++ b/core/includes/image.incundefined
@@ -292,10 +292,9 @@ function image_resize(stdClass $image, $width, $height) {
+ *   uncovered area of the image after the rotation. Examples: "#RGB", "#RGBA",
+ *   "#RRGGBB", "#RRGGBBAA".  For images that support transparency, this will
+ *   default to transparent. Otherwise it will be white.

One might argue, that RGB isn't really an "example".

+++ b/core/includes/image.incundefined
@@ -320,6 +319,11 @@ function image_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

"Examples".

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (Red, Green, Blue, Alpha) integer values and

Capitalize the colors?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   A string specifing an RGB color in the formats: '#ABC', '#ABC', '#ABCD',
+ *   '#ABCD','#AABBCC','#AABBCC','#AABBCCDD','#AABBCCDD'. The color is passed

Do we have some formats twice here? Maybe RGBA instead of ABCD?

Missing spaces after commas: , .

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * @return
+ *   An associative array having 'red', 'green', 'blue' and 'alpha' as keys and
+ *   corresponding color decimal integer as values.

Document FALSE, too?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+function image_hex2rgba(&$hex) {
+
+  // Assure consistency for the color string.

Remove the empty line.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+  if (preg_match('/^#[0-9A-F]{3}$/', $hex)) {

Maybe loop through the patterns and actually capture something in the regexes?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+  // Convert colors to decimal integers.
+  $red = hexdec($red);
+  $green = hexdec($green);
+  $blue = hexdec($blue);
+  $alpha = hexdec($alpha);
+  return array('red' => $red, 'green' => $green, 'blue' => $blue, 'alpha' => $alpha);

Maybe(!):

return array(
  'red' => hexdec($red),
  ...

+++ b/core/modules/image/image.admin.incundefined
@@ -587,6 +592,15 @@ function image_crop_form($data) {
+    '#description' => t('Hex string specifying the background color to use when cropping the image to dimensions that are exceeding the source dimensions. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". Leave blank for transparency on image types that support it.'),

"Examples".

+++ b/core/modules/image/image.testundefined
@@ -307,13 +307,13 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
+    $this->assertTrue(image_rotate_effect($this->image, array('degrees' => 90, 'bgcolor' => '#FFF')), t('Function returned the expected value.'));

While we're touching it: Don't translate assertion messages.

+++ b/core/modules/image/image.testundefined
@@ -307,13 +307,13 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
+    $this->assertEqual($calls['rotate'][0][2], '#FFF', t('Background color was passed correctly'));

Again, no t(), when we touch it anyway.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    $this->assertTrue(is_file($this->image->source), t('The source image (50x25) filled with blue (#0000FF) has been created: %url.', array('%url' => $this->image->source)));

No t().

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    $style1 = image_style_load(NULL, $this->isid1);
+    $style1['description'] = t('Cropping center-to-center from 50x20 to 50x50 with red (#FF0000) color on additional area.');
+    $style1['expected_color'] = t('red (#FF0000)');
+    $style1['expected_color_map'] = array('red' => 255, 'green' => 0, 'blue' => 0, 'alpha' => 0);
+    $style2 = image_style_load(NULL, $this->isid2);
+    $style2['description'] = t('Cropping center-to-center from 50x20 to 50x50 with no color specified for additional area.');
+    $style2['expected_color'] = t('transparent');
+    $style2['expected_color_map'] = array('red' => 0, 'green' => 0, 'blue' => 0, 'alpha' => 127);

No t() just for test cases.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      $this->assert(TRUE, t('Checking a pixel color from the new area, at (5, 5) in "%file".<br />Expecting %color.', $color + array('%file' => $uri)));

t(). <br /> in the message?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      $this->assertIdentical($colors, $style['expected_color_map'], t('Pixel from (5, 5) is %color', $color));

t().

+++ b/core/modules/system/image.gd.incundefined
@@ -105,10 +105,10 @@ function image_gd_resize(stdClass $image, $width, $height) {
+ *   uncovered area of the image after the rotation. Examples: "#RGB", "#RGBA",

"Examples".

+++ b/core/modules/system/image.gd.incundefined
@@ -105,10 +105,10 @@ function image_gd_resize(stdClass $image, $width, $height) {
+ *   default to transparent. Otherwise it will be white.
+ *   ¶
  * @return

Trailing space.

 

What are the arguments for using strings to store color values instead of integers like 0x11223300?

+++ b/core/includes/image.incundefined
@@ -292,10 +292,9 @@ function image_resize(stdClass $image, $width, $height) {
+ *   uncovered area of the image after the rotation. Examples: "#RGB", "#RGBA",
+ *   "#RRGGBB", "#RRGGBBAA".  For images that support transparency, this will
+ *   default to transparent. Otherwise it will be white.

One might argue, that RGB isn't really an "example".

It's not really an example except the place and number of chars for each format. I would let it in that form.

+++ b/core/includes/image.incundefined
@@ -320,6 +319,11 @@ function image_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

"Examples".

Don't understand. You suggest to remove the colon (:)? Or?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (Red, Green, Blue, Alpha) integer values and

Capitalize the colors?

OK. Changed to lowercase.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   A string specifing an RGB color in the formats: '#ABC', '#ABC', '#ABCD',
+ *   '#ABCD','#AABBCC','#AABBCC','#AABBCCDD','#AABBCCDD'. The color is passed

Do we have some formats twice here? Maybe RGBA instead of ABCD?

Missing spaces after commas: , .

OK. Changed to "RGB" format and correct the missing spaces.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+ * @return
+ *   An associative array having 'red', 'green', 'blue' and 'alpha' as keys and
+ *   corresponding color decimal integer as values.

Document FALSE, too?

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+function image_hex2rgba(&$hex) {
+
+  // Assure consistency for the color string.

Remove the empty line.

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,59 @@ function image_save(stdClass $image, $destination = NULL) {
+  if (preg_match('/^#[0-9A-F]{3}$/', $hex)) {

Maybe loop through the patterns and actually capture something in the regexes?

I thought also to validate and extract all colors in a single regexp step. Do you know some magic regexp snippet?

Maybe(!):

return array(
  'red' => hexdec($red),
  ...

OK.

+++ b/core/modules/image/image.testundefined
@@ -307,13 +307,13 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
+    $this->assertTrue(image_rotate_effect($this->image, array('degrees' => 90, 'bgcolor' => '#FFF')), t('Function returned the expected value.'));

While we're touching it: Don't translate assertion messages.

Why? I see translations in all assertion in the .test file. There's no explanation in docs http://api.drupal.org/api/drupal/core!modules!simpletest!drupal_web_test.... I saw http://drupal.org/node/500866 but as per http://drupal.org/node/500866#comment-5564970 it's not very clear what and why should not be translated. If we go without t() we should consider cleaning up all the .test file.

+++ b/core/modules/system/image.gd.incundefined
@@ -105,10 +105,10 @@ function image_gd_resize(stdClass $image, $width, $height) {
+ *   default to transparent. Otherwise it will be white.
+ *   ¶
  * @return

Trailing space.

OK.

What are the arguments for using strings to store color values instead of integers like 0x11223300?

Read past comments. In Drupal 7, for image_rotate, colors are stored in hex string formatt (#RRGGBB). While this patch will be backported to D7 we don't want to add an upgrade path (hook_update_N() to convert existing stored colors from string to integers.

Thank you.

Don't understand. You suggest to remove the colon (:)? Or?

No, I guess that's fine. Just arguing against the word "Example" there, too. But - why not - leave it as it is.

One more:

+++ b/core/modules/image/image.admin.incundefined
@@ -475,10 +475,15 @@ function image_effect_integer_validate($element, &$form_state) {
+      form_error($element, t('!name must be a hexadecimal color value. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA".', array('!name' => $element['#title'])));
We shouldn't use !name but either @name or %name here.

If we go without t() we should consider cleaning up all the .test file.

Yep, that's the plan. (Obviously not in this issue). I encountered the t() thing, too. "But it's all around here!". sun and xjm clarified it in IRC: Yes, don't translate stuff just for test cases.

Status:Needs work» Needs review
StatusFileSize
new19.56 KB
FAILED: [[SimpleTest]]: [MySQL] 34,270 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Compacted the image_hex2rgba() function but still cannot process in a single regexp step.

Status:Needs review» Needs work

Forgot to test locally.

Status:Needs work» Needs review
StatusFileSize
new19.62 KB
PASSED: [[SimpleTest]]: [MySQL] 34,275 pass(es).
[ View ]

Fixed test errors.

Status:Needs review» Needs work

+++ b/core/includes/image.inc
@@ -292,10 +292,9 @@ function image_resize(stdClass $image, $width, $height) {
  *   An hexadecimal integer specifying the background color to use for the

This should be "An hexadecimal string", not integer. Fixing...

18 days to next Drupal core point release.

Status:Needs work» Needs review
StatusFileSize
new19.71 KB
PASSED: [[SimpleTest]]: [MySQL] 34,270 pass(es).
[ View ]

Final patch...

Issue summary:View changes

Updated issue summary.

Status:Needs review» Needs work

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (red, green, blue, alpha) integer values and
+ * tidy up the input hex color string.

Converts and tidies?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   by reference in order to return a tidy up color string for consistency.

tidied up color string?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+    $hex_color = preg_replace('|([0-9A-F])|', "\\1\\1", $hex_color);

Or '\1\1' having a single quoted string.
Btw. I like this step of normalizing. It's probably clearer than any magic long regex doing everything at once.

+++ b/core/modules/image/image.admin.incundefined
@@ -475,10 +475,15 @@ function image_effect_integer_validate($element, &$form_state) {
+    if ($hex != $element['#value']) {

Do we need to have this $hex variable? Couldn't we just do form_set_value() regardless of what happened?

+++ b/core/modules/image/image.effects.incundefined
@@ -258,26 +259,14 @@ function image_rotate_effect(&$image, $data) {
+  $background_color = empty($data['bgcolor']) ? NULL : $data['bgcolor'];

I notice we have $data['bgcolor'] here and $data['background_color'] above.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+class ImageCropBackgroundTestCase extends DrupalWebTestCase {

Maybe use protected $profile = 'testing'; and parent:setUp(array('required module 1', ...)); just the required modules?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+
+    // Invoke parent method.
+    parent::setUp();

Superflous empty line and comment.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Source image width & height.

I'd go with and.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Create 2 dummy styles.

two, maybe.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      // Check if the Image Style has been created.

Image Style?

 

Edit: Some more:

  • When I save #FFF the next time I load #FFF is still shown? Shouldn't that have been normalized to #FFFFFF?
  • When having #FF0000FF the border is black, whereas #FF000055 is reddish-transparent. Is that expected?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ * Converts a hex string to RGBA (red, green, blue, alpha) integer values and
+ * tidy up the input hex color string.

Converts and tidies?

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   by reference in order to return a tidy up color string for consistency.

tidied up color string?

OK.

+++ b/core/includes/image.incundefined
@@ -432,5 +436,45 @@ function image_save(stdClass $image, $destination = NULL) {
+    $hex_color = preg_replace('|([0-9A-F])|', "\\1\\1", $hex_color);

Or '\1\1' having a single quoted string.

OK.

+++ b/core/modules/image/image.admin.incundefined
@@ -475,10 +475,15 @@ function image_effect_integer_validate($element, &$form_state) {
+    if ($hex != $element['#value']) {

Do we need to have this $hex variable? Couldn't we just do form_set_value() regardless of what happened

Well. It is faster when the color is correct entered. I suggest to leave it in this way... it gives a good message that $hex was tidied up.

+++ b/core/modules/image/image.effects.incundefined
@@ -258,26 +259,14 @@ function image_rotate_effect(&$image, $data) {
+  $background_color = empty($data['bgcolor']) ? NULL : $data['bgcolor'];

I notice we have $data['bgcolor'] here and $data['background_color'] above.

Yes. I saw that but I decided to left it as 'bcolor'. That was there at image_rotate. What you think, is a change (to 'background_color') that can impact other modules?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+class ImageCropBackgroundTestCase extends DrupalWebTestCase {

Maybe use protected $profile = 'testing'; and parent:setUp(array('required module 1', ...)); just the required modules?

I don't get it :) Please explain.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+
+    // Invoke parent method.
+    parent::setUp();

Superflous empty line and comment.

OK.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Source image width & height.

I'd go with and.

OK.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Create 2 dummy styles.

two, maybe.

OK.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+      // Check if the Image Style has been created.

Image Style?

Yes. If "Style" creation failed in setUp() it cannot be loaded. So we check if was successfully loaded and ready to be applied.

When I save #FFF the next time I load #FFF is still shown? Shouldn't that have been normalized to #FFFFFF?

Well this needs a discussion. If user enters #F00 he will expect to get the same. So, i would not normalize that in storage. It's his choice.

When having #FF0000FF the border is black, whereas #FF000055 is reddish-transparent. Is that expected?

That I was checking after submitting the patch. Alpha is in range 0 => 127 (7F). So I will limit to 7F.

Thanks.

Yes. I saw that but I decided to left it as 'bcolor'. That was there at image_rotate. What you think, is a change (to 'background_color') that can impact other modules?

Right, I think we shouldn't change that here. Maybe the other way around, background_color to bgcolor, to have it consistent? Or we just leave that as it is in the patch.

I don't get it :) Please explain.

Admittedly that was too much for too few words :D
So ... DrupalWebTestCase is going to do a Drupal installation. The default installation profile has lot's of stuff we aren't going to need, which makes tests runs longer. So the suggestion is to select the testing profile by setting

<?php
 
protected $profile = 'testing';
?>
at the top of the class. That also means some of the modules we do need are not going to be enabled. Those will have to be passed as an array to parent::setUp().

Well. It is faster when the color is correct entered. I suggest to leave it in this way... it gives a good message that $hex was tidied up.

Agreed.

Yes. If "Style" creation failed in setUp() it cannot be loaded. So we check if was successfully loaded and ready to be applied.

Ok ... the bold first letters we're a little hard to see. I wanted to know why the first letters of "Image Style" were capital.

Well this needs a discussion. If user enters #F00 we will expect to get the same. So, i would not normalize that in storage. It's his choice.

Ok. I don't feel it should be one way or another - just to bring it up. Well ... one could indeed argue it should stay as it is.

About...

  • 'bgcolor'. We need to keep it for D7 backport in order to avoid an update path. That array key is seralized and stored in backlend. I would keep also the new 'background_color' as it's a better spelling. Maybe we will unify it in the future...
  • Test profile. Makes sense... It seems that Image module is enabled by default. Checking...

'bgcolor'. We need to keep it for D7 backport in order to avoid an update path. That array key is seralized and stored in backlend. I would keep also the new 'background_color' as it's a better spelling. Maybe we will unify it in the future...

I see. Fair enough.

testing profile will have none but the absolutely required core modules enabled by default.

Status:Needs work» Needs review
StatusFileSize
new19.73 KB
PASSED: [[SimpleTest]]: [MySQL] 34,271 pass(es).
[ View ]

Latest updates...

ImageMagick toolkit implemented in ImageAPI needs a patch after backporting this to D7. Opened e placeholder issue there: #1437036: API change for image_crop effect.

Status:Needs review» Needs work

That I was checking after submitting the patch. Alpha is in range 0 => 127 (7F). So I will limit to 7F.

Yay, works now :) The only remaining problem now is this: Imagine a user enters #F0FE. The error message is: Background color must be a hexadecimal color value. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". How can I guess what's wrong, from that?

 

Other places in core seam to use @name instead of %name. (For example width and height of the crop effect.)

 

+++ b/core/includes/image.incundefined
@@ -432,5 +436,44 @@ function image_save(stdClass $image, $destination = NULL) {
+  // Save a uppercase version without leading "#" for later processing.

an uppercase version.

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Invoke parent method.

I think this states the obvious.

 

Now some stuff you could ignore, as well:

+++ b/core/includes/image.incundefined
@@ -320,6 +319,11 @@ function image_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

When you're rerolling it anyway maybe standardize on single quotes as below?

+++ b/core/includes/image.incundefined
@@ -432,5 +436,44 @@ function image_save(stdClass $image, $destination = NULL) {
+ *   'RGBA', '#RRGGBB', 'RRGGBB', '#RRGGBBAA', 'RRGGBBAA'. The color is passed

(For reference: Single quotes used here.)

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Attach a single "image_crop" effect to the first style that enlarges the

Single quotes?

+++ b/core/modules/simpletest/tests/image.testundefined
@@ -503,3 +503,123 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    // Attach a single "image_crop" effect to the second style that enlarges the

Single quotes?

+++ b/core/modules/system/image.gd.incundefined
@@ -179,15 +177,32 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
+ *   Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". For images that support

Single quotes?

I fixed my problem using Canvas from http://drupal.org/project/imagecache_actions

There are several ways to approach this issue, and this seamed the simplest at the moment.

Gabriel

The only remaining problem now is this: Imagine a user enters #F0FE. The error message is: Background color must be a hexadecimal color value. Examples: "#RGB", "#RGBA", "#RRGGBB", "#RRGGBBAA". How can I guess what's wrong, from that?

That I wouldn't change the validation function because it mean to explain how a hex color string must be composed. #HF0 may be another error that must be explained, no? In prefer to enrich a little bit the error message - maybe with a link to some Wikipedia definition.

What you think?

Error message proposal:

Crop background color must be a hexadecimal string color value taking one of the next possible formats: '#RRGGBB', '#RRGGBBAA', '#RGB', '#RGBA', where:

  • 'RR' (shorthand 'R'): Red component. Range: '00' to 'FF'.
  • 'GG' (shorthand 'G'): Green component. Range: '00' to 'FF'.
  • 'BB' (shorthand 'B'): Blue component. Range: '00' to 'FF'.
  • 'AA' (shorthand 'A'): Optional alpha transparency. Range: '00' to '7F'. Caution on using the shorthand version, you cannot enter a value greater than '7'!

I really don't think we should support so many options for validation. It actually makes for worse UX. I'd suggest only supporting what the HTML 5 color element does (since we will want to convert the D8 patch here once #1445224: Add new HTML5 FAPI element: color is in). That is, only support longhand values--and make alpha a separate field. The hex range for alpha is just weird. We can standardize case name, etc. silently.

Can you atach all you changed file, or send file to email modestas@irankiubaze.com
Thank you

StatusFileSize
new27.91 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

What's new:

  • This is a BUG, not a feature request.
  • The patch reflects @xjm comment in #136.
  • Reworked the patch to use the new HTML5 color form element.
  • The HTML5 color picker (with textfield fallback) doesn't allow to pass an empty value to the color. So, I had to add an additional checkbox.
  • The image_rotate effect was changed too in order to provide an unified way to pass the "new areas" color.
  • New API changes are backward compatible, so they don't break D8 API freeze.

Didn't provide an interdiff.txt as is not relevant due to the big number of changes.

Category:feature» bug

The last submitted patch, drupal-image-crop-204497-138.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.91 KB
PASSED: [[SimpleTest]]: [MySQL] 56,729 pass(es).
[ View ]

Removed trailing spaces.

StatusFileSize
new29.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-image-crop-204497-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reworked on top of last changes to image.

Issue summary:View changes

Updated issue summary.

Updated issue summary.

Issue summary:View changes

Summary changed.

I don't understand why this is a bug, sounds like a feature request.

Also this should wait on #2033669: Image file objects should be classed

This issue hangs from the good times of Image Cache module and was moved to core starting with D6. It's a bug because it renders in black the new areas but you don't expect that. Rotate and crop are the only core effects that may generate new areas. On crop there's no way to control the color or transparency (when applicable) of the new areas and you'll get unexpected black new areas. Rotate it's not buggy from this point of view. Basically all we want to do here is a simple fix to crop that is always present in rotate (but in other way).

I agree that this should go after #2033669: Image file objects should be classed. I'm following that issue as I'm following all related to image & stuff.

#141: drupal-image-crop-204497-141.patch queued for re-testing.

Looking at the timestamps, the last test took place just before #2033669: Image file objects should be classed was committed.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +needs backport to D7

The last submitted patch, drupal-image-crop-204497-141.patch, failed testing.

Hey @fietserwin, the patch doesn't apply. I'm already working on reroll but also on much more better approach.

@fietserwin, #2063373: Cannot save image created from resource is blocking this. Can you help reviewing that? Thanks!

Issue summary:View changes

Updated issue summary.

The problem still exists in D6 and D7. How can we proceed here?

@Anybody, see the ImageCache Actions module. Install and enable the "Imagecache Canvas Actions" module. It provides a "Define canvas" image style action which will let you set the background color and image dimensions.