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

Fix #769 #770

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion Imagine/Filter/Loader/DownscaleFilterLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,16 @@ public function load(ImageInterface $image, array $options = array())
$widthRatio = $width / $origWidth;
$heightRatio = $height / $origHeight;

$ratio = min($widthRatio, $heightRatio);
// faster check than is_null
if (null === $width || null === $height) {
$ratio = max($widthRatio, $heightRatio);
} else {
$ratio = min($widthRatio, $heightRatio);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be $ratio = ($width === null || $height === null) ? max($widthRatio, $heightRatio) : min($widthRatio, $heightRatio); ?

Copy link
Contributor Author

@deviprsd deviprsd Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, either way that is just a style thing, I don't think it will be a performance issue, so I'm okay with anything. you might see this info http://stackoverflow.com/a/11643364

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in Symfony we generally use null === $foo


if ($ratio > 1) {
return $image;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be controversial thing, discuss on this 🎱

Copy link
Collaborator

@alexwilson alexwilson Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, now that's my kinda fix! 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be an option, defaulting to off

Copy link
Contributor Author

@deviprsd deviprsd Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more like, creating another filter "scale" which will do either, the filters should do only what they should, I don't know maybe it's just clear that way, moreover downscale and upscale should be derived from scale

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine for me too


$filter = new Resize(new Box($origWidth * $ratio, $origHeight * $ratio));

Expand Down
11 changes: 10 additions & 1 deletion Imagine/Filter/Loader/UpscaleFilterLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,16 @@ public function load(ImageInterface $image, array $options = array())
$widthRatio = $width / $origWidth;
$heightRatio = $height / $origHeight;

$ratio = $widthRatio > $heightRatio ? $widthRatio : $heightRatio;
// faster check than is_null
if (null === $width || null === $height) {
$ratio = max($widthRatio, $heightRatio);
} else {
$ratio = min($widthRatio, $heightRatio);
}

if ($ratio < 1) {
return $image;
}

$filter = new Resize(new Box(round($origWidth * $ratio), round($origHeight * $ratio)));

Expand Down