syamilmj / Aqua-Resizer

Resize WordPress images on the fly
502 stars 205 forks source link

More dynamic and strictness #22

Closed mxmzb closed 11 years ago

mxmzb commented 11 years ago
  1. Commit adds the ability to resize on height, too, and leave width free. It removes absolutely no functionality.
  2. Commit is enhances good behaviour. I think, it's somehow arbitrary to return just a partly resized image if one site extends the original length (for example if You want to resize an image with 100x100 to 110x90 it used to become a 100x90 image, which is hard to handle). Imho it is best to stay over everything and to have the ability to handle every detail. This way, You can now just check like
$resized_image_url = aqua_resize($image_url, $width, $height);
if(!$resized_image_url) {
    // handle that
}

If You want to resize an image to it's original size the script will return the image url from the arguments (which is the same for the case that only either width or height has been passed but the image size is still the same - done at line 55).

There is still some room for improvement in my opinion, especially at the point that now it will return false if some technical error occures and the original image is not as large as the resizing arguments. There should be a way to distinguish these two possibilities, but I think we could discuss that if You find the changes made by me useful.

PS.: changes in every line come from the fact that I converted all indentation to spaces.

syamilmj commented 11 years ago

Hi, thanks for this. Can you please confirm that you only changed this line?

if(!$url || (!$width && !$height)) return false;

mxmzb commented 11 years ago

What exactly do You mean by that? You can see above in the commits what I've changed exactly: https://github.com/dotwired/Aqua-Resizer/commit/5851b16a16aa4ff28ebb5633eff48c8e82d07900 and https://github.com/dotwired/Aqua-Resizer/commit/2cfb2ac09676c019bb5827452046d87616ba2575, in the second one I unfortunately did change the indentation mode in my sublimetext so now all the lines are marked as changed. But the main part there is

// return the original image only if it exactly fits the needed measures
    if(!$dims && ((($height === null && $orig_w == $width) xor ($width === null && $orig_h == $height)) xor ($height == $orig_h && $width == $orig_w))) {
        $img_url = $url;
        $dst_w = $orig_w;
        $dst_h = $orig_h;
    } else {
        ...
        if(!$dims || $dst_w < $width || $dst_h < $height) {
            //can't resize, so return false saying that the action to do could not be processed as planned.
            return false;
        }
        ...

If You prefer I can do the changes again without the indentation so the touched lines will be marked exactly?

syamilmj commented 11 years ago

Yes, that would've been more preferable, so that others including myself could track the exact changes made.

Also, a little concern here - the script would return false if the image can't be resized? What's the benefit of that or did I miss anything?

Cheers :)

mxmzb commented 11 years ago

Well the point is, that You have right now no control about the produced image size, if the image is being resized. Although you can specify the width and height of the thumbnail you want generate, it can be that the thumbnail will not have these measures (e.g. try to resize an 100x100px image to a 90x110px one. You will get a 90x100px image, which may be surprising especially if You do not know the exact measurement of the image before resizing).

That means You would either have to get the image size before or use the array output always, which would give You the knowledge about the real generated size, but it could be that you don't want any other image size than specified (even likely). Also, if You check the array and see the size there, what happens with that needless generated image?

In my opinion, more control always is an advantage, which is why I did this. ;)

syamilmj commented 11 years ago

Cool thanks! Would you please update the README file & the version number as well? Also, since this added a new feature, it would be helpful if you can also contribute a little instructions/pointers to authors in the Wiki section. Make sure to link to it from the README file (under changelog)

Cheers!