ENH Remove animation for thumbnails by default. #1475
Conversation
code/Model/ThumbnailGenerator.php
Outdated
| @@ -107,6 +110,14 @@ public function generateThumbnail(AssetContainer $file, $width, $height) | |||
| $file = $file->existingOnly(); | |||
| } | |||
|
|
|||
| // Try to remove the animation if we can | |||
| if (!$this->getAllowsAnimation() && $file->getIsAnimated() && ClassInfo::hasMethod($file, 'RemoveAnimation')) { | |||
There was a problem hiding this comment.
Note it doesn't make sense to add the RemoveAnimation method to AssetContainer interface, because it's about image manipulation.
Even though that interface is only used by DBFile and File which both use the ImageManipulation trait, we shouldn't assume the object has this method for the sake of strict correctness. (even though below we assume it has whatever $method is lol)
GuySartorelli
force-pushed
the
pulls/3/remove-animation-for-thumbnails
branch
2 times, most recently
from
6056770 to
fc13971
Compare
GuySartorelli
force-pushed
the
pulls/3/remove-animation-for-thumbnails
branch
2 times, most recently
from
f49d2d7 to
c32badc
Compare
|
Swapped sample gif to one we explicitly own |
GuySartorelli
force-pushed
the
pulls/3/remove-animation-for-thumbnails
branch
from
c32badc to
0e65505
Compare
| if (!$this->getAllowsAnimation()) { | ||
| if (ClassInfo::hasMethod($file, 'getImageBackend')) { | ||
| /** @var Image_Backend $backend */ | ||
| $backend = $file->getImageBackend(); | ||
| $origAllowAnimation = $backend->getAllowsAnimationInManipulations(); | ||
| $backend->setAllowsAnimationInManipulations(false); | ||
| } elseif ($file->getIsAnimated() && ClassInfo::hasMethod($file, 'RemoveAnimation')) { | ||
| $noAnimation = $file->RemoveAnimation(); | ||
| if ($noAnimation) { | ||
| $file = $noAnimation; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
See #1475 (comment) - this is now more complex logic, but that principle remains that we should adhere to the API of the AssetContainer interface. As such I've provided two ways to do this - one where there's an Image_Backend available and one where that's not available but RemoveAnimation() is.
GuySartorelli
force-pushed
the
pulls/3/remove-animation-for-thumbnails
branch
from
0e65505 to
2586b54
Compare
Needs silverstripe/silverstripe-assets#621 for CI to go green.
Issue