Permalink
Browse files

#1968 - Improve album cover generation/removal/etc.

- Added stanza to Item_Model::save that handles when cover id is null.
- Added logic to graphics::generate to copy/convert album cover thumbs from their item thumbs to ensure they're always jpg, and eliminate the possibility that we copy/convert a dirty thumb.
- Redirected other places in code where we want to do one of the above two things to use these two functions instead (gallery_event::item_updated_data_file, item::make_album_cover, item::remove_album_cover).
- Improved validation in Item_Model so only albums can have covers and all covers must be non-albums.
- Added unit tests to Graphics_Helper_Test.
  • Loading branch information...
1 parent 861e862 commit cf077425953a6a492dea97eaf1517e7d08c9648f @shadlaws shadlaws committed Jan 30, 2013
@@ -141,10 +141,9 @@ static function item_updated_data_file($item) {
foreach (ORM::factory("item")
->where("album_cover_item_id", "=", $item->id)
->find_all() as $target) {
- copy($item->thumb_path(), $target->thumb_path());
- $target->thumb_width = $item->thumb_width;
- $target->thumb_height = $item->thumb_height;
+ $target->thumb_dirty = 1;
$target->save();
+ graphics::generate($target);
}
}
@@ -115,36 +115,12 @@ static function deactivate_rules($module_name) {
* @param Item_Model $item
*/
static function generate($item) {
- if ($item->is_album()) {
- if (!$cover = $item->album_cover()) {
- // This album has no cover; there's nothing to generate. Because of an old bug, it's
- // possible that there's an album cover item id that points to an invalid item. In that
- // case, just null out the album cover item id. It's not optimal to do that at this low
- // level, but it's not trivial to find these cases quickly in an upgrade script and if we
- // don't do this, the album may be permanently marked as "needs rebuilding"
- //
- // ref: http://sourceforge.net/apps/trac/gallery/ticket/1172
- // http://galleryproject.org/node/96926
- if ($item->album_cover_item_id) {
- $item->album_cover_item_id = null;
- $item->save();
- }
- return;
- }
- $input_file = $cover->file_path();
- $input_item = $cover;
- } else {
- $input_file = $item->file_path();
- $input_item = $item;
- }
-
if ($item->thumb_dirty) {
$ops["thumb"] = $item->thumb_path();
}
- if ($item->resize_dirty && !$item->is_album() && !$item->is_movie()) {
+ if ($item->resize_dirty && $item->is_photo()) {
$ops["resize"] = $item->resize_path();
}
-
if (empty($ops)) {
$item->thumb_dirty = 0;
$item->resize_dirty = 0;
@@ -154,37 +130,67 @@ static function generate($item) {
try {
foreach ($ops as $target => $output_file) {
+ $working_file = $item->file_path();
// Delete anything that might already be there
@unlink($output_file);
- if ($input_item->is_movie()) {
+ switch ($item->type) {
+ case "movie":
// Run movie_extract_frame events, which can either:
// - generate an output file, bypassing the ffmpeg-based movie::extract_frame
// - add to the options sent to movie::extract_frame (e.g. change frame extract time,
// add de-interlacing arguments to ffmpeg... see movie helper for more info)
// Note that the args are similar to those of the events in gallery_graphics
$movie_options_wrapper = new stdClass();
$movie_options_wrapper->movie_options = array();
- module::event("movie_extract_frame", $input_file, $output_file,
- $movie_options_wrapper, $input_item);
+ module::event("movie_extract_frame", $working_file, $output_file,
+ $movie_options_wrapper, $item);
// If no output_file generated by events, run movie::extract_frame with movie_options
clearstatcache();
if (@filesize($output_file) == 0) {
try {
- movie::extract_frame($input_file, $output_file, $movie_options_wrapper->movie_options);
+ movie::extract_frame($working_file, $output_file, $movie_options_wrapper->movie_options);
} catch (Exception $e) {
// Didn't work, likely because of MISSING_FFMPEG - use placeholder
graphics::_replace_image_with_placeholder($item, $target);
}
}
$working_file = $output_file;
- } else {
- $working_file = $input_file;
- }
- foreach (self::_get_rules($target) as $rule) {
- $args = array($working_file, $output_file, unserialize($rule->args), $item);
- call_user_func_array($rule->operation, $args);
- $working_file = $output_file;
+ case "photo":
+ // Run the graphics rules (for both movies and photos)
+ foreach (self::_get_rules($target) as $rule) {
+ $args = array($working_file, $output_file, unserialize($rule->args), $item);
+ call_user_func_array($rule->operation, $args);
+ $working_file = $output_file;
+ }
+ break;
+
+ case "album":
+ if (!$cover = $item->album_cover()) {
+ // This album has no cover; there's nothing to generate. Because of an old bug, it's
+ // possible that there's an album cover item id that points to an invalid item. In that
+ // case, just null out the album cover item id. It's not optimal to do that at this low
+ // level, but it's not trivial to find these cases quickly in an upgrade script and if we
+ // don't do this, the album may be permanently marked as "needs rebuilding"
+ //
+ // ref: http://sourceforge.net/apps/trac/gallery/ticket/1172
+ // http://galleryproject.org/node/96926
+ if ($item->album_cover_item_id) {
+ $item->album_cover_item_id = null;
+ $item->save();
+ }
+ return;
+ }
+ if ($cover->thumb_dirty) {
+ graphics::generate($cover);
+ }
+ if (!$cover->thumb_dirty) {
+ // Make the album cover from the cover item's thumb. Run gallery_graphics::resize with
+ // null options and it will figure out if this is a direct copy or conversion to jpg.
+ $working_file = $cover->thumb_path();
+ gallery_graphics::resize($working_file, $output_file, null, $item);
+ }
+ break;
}
}
@@ -78,15 +78,9 @@ static function make_album_cover($item) {
model_cache::clear();
$parent->album_cover_item_id = $item->is_album() ? $item->album_cover_item_id : $item->id;
- if ($item->thumb_dirty) {
- $parent->thumb_dirty = 1;
- graphics::generate($parent);
- } else {
- copy($item->thumb_path(), $parent->thumb_path());
- $parent->thumb_width = $item->thumb_width;
- $parent->thumb_height = $item->thumb_height;
- }
$parent->save();
+ graphics::generate($parent);
+
$grand_parent = $parent->parent();
if ($grand_parent && access::can("edit", $grand_parent) &&
$grand_parent->album_cover_item_id == null) {
@@ -97,15 +91,10 @@ static function make_album_cover($item) {
static function remove_album_cover($album) {
access::required("view", $album);
access::required("edit", $album);
- @unlink($album->thumb_path());
model_cache::clear();
$album->album_cover_item_id = null;
- $album->thumb_width = 0;
- $album->thumb_height = 0;
- $album->thumb_dirty = 1;
$album->save();
- graphics::generate($album);
}
/**
@@ -420,6 +420,15 @@ public function save() {
}
}
+ // If an album's cover has changed (or been removed), delete any existing album cover,
+ // reset the thumb metadata, and mark the thumb as dirty.
+ if (array_key_exists("album_cover_item_id", $this->changed) && $this->is_album()) {
+ @unlink($original->thumb_path());
+ $this->thumb_dirty = 1;
+ $this->thumb_height = 0;
+ $this->thumb_width = 0;
+ }
+
if (array_intersect($this->changed, array("parent_id", "name", "slug"))) {
$original->_build_relative_caches();
$this->relative_path_cache = null;
@@ -966,10 +975,12 @@ public function valid_album_cover(Validation $v, $field) {
return;
}
- if ($this->album_cover_item_id && db::build()
+ if ($this->album_cover_item_id && ($this->is_photo() || $this->is_movie() ||
+ db::build()
->from("items")
->where("id", "=", $this->album_cover_item_id)
- ->count_records() != 1) {
+ ->where("type", "<>", "album")
+ ->count_records() != 1)) {
$v->add_error("album_cover_item_id", "invalid_item");
}
}
@@ -44,6 +44,39 @@ public function generate_movie_test() {
$this->assert_equal(0, $movie->thumb_dirty);
}
+ public function generate_album_cover_test() {
+ $album = test::random_album();
+ $photo = test::random_unique_photo($album);
+ $album->reload();
+ // Check that the image was copied directly from item thumb
+ $this->assert_equal(file_get_contents($photo->thumb_path()),
+ file_get_contents($album->thumb_path()));
+ // Check that the items table got updated
+ $this->assert_equal(array(200, 150), array($album->thumb_width, $album->thumb_height));
+ // Check that the image is not marked dirty
+ $this->assert_equal(0, $album->thumb_dirty);
+ }
+
+ public function generate_album_cover_from_png_test() {
+ $input_file = MODPATH . "gallery/tests/test.jpg";
+ $output_file = TMPPATH . test::random_name() . ".png";
+ gallery_graphics::resize($input_file, $output_file, null, null);
+
+ $album = test::random_album();
+ $photo = test::random_photo_unsaved($album);
+ $photo->set_data_file($output_file);
+ $photo->name = "album_cover_from_png.png";
+ $photo->save();
+ $album->reload();
+ // Check that the image was correctly resized and converted to jpg
+ $this->assert_equal(array(200, 150, "image/jpeg", "jpg"),
+ photo::get_file_metadata($album->thumb_path()));
+ // Check that the items table got updated
+ $this->assert_equal(array(200, 150), array($album->thumb_width, $album->thumb_height));
+ // Check that the image is not marked dirty
+ $this->assert_equal(0, $album->thumb_dirty);
+ }
+
public function generate_bad_photo_test() {
$photo = test::random_photo();
// At this point, the photo is valid and has a valid resize and thumb. Make it garble.
@@ -86,4 +119,29 @@ public function generate_bad_movie_test() {
// Check that the image is *not* marked as dirty
$this->assert_equal(0, $movie->thumb_dirty);
}
+
+ public function generate_album_cover_from_bad_photo_test() {
+ $album = test::random_album();
+ $photo = test::random_photo($album);
+ $album->reload();
+ // At this point, the photo is valid and has a valid resize and thumb. Make it garble.
+ file_put_contents($photo->file_path(), test::lorem_ipsum(200));
+ // Regenerate album from garbled photo.
+ $photo->thumb_dirty = 1;
+ $photo->save();
+ $album->thumb_dirty = 1;
+ try {
+ graphics::generate($album);
+ $this->assert_true(false, "Shouldn't get here");
+ } catch (Exception $e) {
+ // Exception expected
+ }
+ // Check that the image got replaced with a missing image placeholder
+ $this->assert_same(file_get_contents(MODPATH . "gallery/images/missing_photo.jpg"),
+ file_get_contents($album->thumb_path()));
+ // Check that the items table got updated with new metadata
+ $this->assert_equal(array(200, 200), array($album->thumb_width, $album->thumb_height));
+ // Check that the images are marked as dirty
+ $this->assert_equal(1, $album->thumb_dirty);
+ }
}

0 comments on commit cf07742

Please sign in to comment.