The attached patch refactors theme_image_gallery() to allow for easier theming by pulling the code for creating the html for 1 image into its own theme_image_gallery_img() function.

Comments?

CommentFileSizeAuthor
#4 image_gallery-theme-2.patch2.67 KBray007
image_gallery-theme-1.patch2.79 KBray007

Comments

drewish’s picture

drewish commented
Status: Needs review » Needs work

i think it's a good change. i'll commit it with a few tweaks. rather that having a static size variables in theme_image_gallery_img(), i'd rather pass $size in as a parameter. also is there a reason that $image is being passed by reference?

ray007’s picture

ray007 commented

Old habits die hard?
I think I've read passing object by reference improves performance, no?

Feel free to change the signature to pass the size as parameter, but I thought the overwriting theming function may want to use another preset size, ignore this size, ... and can get it if wanted. It seemed special to the default theming, and not something we want as parameter, but I may be wrong here ;-)

drewish’s picture

drewish commented

i don't believe it makes any appreciable difference and but it sends an incorrect message that the theme function may be modifying the image object.

there are a few reasons for passing the size:
* it's cleaner cleaner than having to have the statics and code to detect and set the variables.
* i think the size information would be useful in the general case and it's much easier to ignore the information than it is to have every implementing function have to load it.

i don't really have time to work on this right now so but if you'd re-roll a patch, i'll commit it.

ray007’s picture

ray007 commented
Status: Needs work » Needs review
StatusFileSize
new image_gallery-theme-2.patch2.67 KB

All right, here a new version of the patch with $size as additional parameter to theme_image_gallery_img(). Or would you prefer separate $width and $height parameters?

drewish’s picture

drewish commented
Status: Needs review » Fixed

great, committed this to HEAD and DRUPAL-5.

ray007’s picture

ray007 commented

Maybe the following little patch would improve performance a bit:

diff --cc contrib/image_gallery/image_gallery.module
index 4bcf21c,62b2707..0000000
--- a/contrib/image_gallery/image_gallery.module
+++ b/contrib/image_gallery/image_gallery.module
@@@ -350,8 -350,8 +350,9 @@@ function theme_image_gallery($galleries
  
    if (!empty($images)) {
      $content .= '<ul class="images">';
++    $tfunc = theme_get_function('image_gallery_img');
      foreach ($images as $image) {
--      $content .= theme('image_gallery_img', $image, $size);
++      $content .= $tfunc($image, $size);
      }
      $content .= "</ul>\n";
    }

patch is on top of what you already commited.

drewish’s picture

drewish commented
Status: Fixed » Reviewed & tested by the community

that's a good idea, i'll probably rename $tfunc to $function. i'll get this committed on monday.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Closed (won't fix)

Sorry, 5.x-1.x won't see any new features.