Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

This patch only changes the thumbnails you see on node/x/edit.

Imagecache settings are only available if imagecache is installed, without imagecache all remains the same.

Can someone please do a quick code review on the patch we can make this RTBC.

attiks’s picture

Status: Active » Needs review
FileSize
9.73 KB

New patch attached, only text changes

quicksketch’s picture

Status: Needs review » Needs work

Thanks for taking the effort to put this together! I'd like to see this available in ImageField, however I'm not entirely sure this is really the approach we'd like to take with ImageField. Could you explain why you'd like to use ImageCache (nearly making it a dependency for ImageField) rather than having ImageField be self-sufficient? #305621: Imagefield configurable upload thumbnail size has a lot of interesting discussion on the topic.

Besides that, some comments on the patch:
- This needs to be updated for HEAD, as the code has changed drastically.
- We don't need to pass in "useimagecachepreset" and "imagecachepreset" into theme functions. If "imagecachepreset" is NULL, then that means we're not using a preset.
- Variable names should not be all crammed together. To shorten it up a bit, could we use "use_preset" and "preset_name"?
- If we decide to make an imagecache-only solution, the code for hook_file_insert() and hook_file_delete() should be removed. And we'd need to write an update function that removes all the thumbnails.
- Rather than wrapping form elements in if (module_exists('imagecache')) {, a better approach is to use #access => module_exists('imagecache'). Values can be maintained even when ImageCache is disabled.

But before getting into rerolling, more explanation of your motives would be helpful. I personally don't like the way that ImageField puts a .thumb. file right in the directory with the original image, but my solution to this was to move all the thumbnails to a separate directory, similar to ImageCache. This would have the advantage of still not requiring ImageCache (which has the additional burden of requiring Clean URLs and ImageAPI).

attiks’s picture

Thanks for the review, the patch was a quick and dirty job so I'll make a new one against head.

Motivation is three-fold:
1/ get rid of the thumb.jpg files to get rid of the clutter with WYSIWYG editors.
2/ all thumbs are generated as jpg files, even if you upload a (transparent) png.
3/ be able to give more accurate previews, suppose you display images in black and white on your site, wouldn't it be nice to have the thumbnails on the edit form in black and white as well.

The idea is to make the imagecache part optional so that imagefield can work without imagecache. I'm also in favor of a separate direcotry for all thumbnails generated by vanilla imagefield (see http://drupal.org/node/305621#comment-1068302).

quicksketch’s picture

2/ all thumbs are generated as jpg files, even if you upload a (transparent) png.

At least this one is already fixed. I changed this in imagefield_update_6004().

The idea is to make the imagecache part optional so that imagefield can work without imagecache.

Sounds good, but let's make sure that thumbnails still work without ImageCache. Putting all the thumbnail images into a separate directory would fix your first concern. If you're up for writing that fix that would be fantastic, but we should put it in a separate issue.

quicksketch’s picture

1/ get rid of the thumb.jpg files to get rid of the clutter with WYSIWYG editors.

I made some more serious changes to the functionality of ImageField thumbnails in #307503: Move *.thumb.jpg files to a separate directory. Thumbnails are now stored separately, not all mixed in.

I definitely still think this is important to get in though, please post an updated patch when you get the chance.

attiks’s picture

Included a patch against head, following the remarks in #3

One problem remains: If you select preview on the node/x/edit page you'll get the classic thumbnail, so either I missed a function or the ['data'] part doesn't get filled in.

attiks’s picture

Some random thought:

If we provide an /admin/imagefield/settings page we can:
- make 'use imagecache for thumbnails' a global option and remove it from the widget
- add a default imagecache preset to use for all thumbnails
- add options for the custom image thumbnail size #307503: Move *.thumb.jpg files to a separate directory
- allow people to select the directory for the thumbnails #307503: Move *.thumb.jpg files to a separate directory

ari-meetai’s picture

This sounds good. Better than the current way of changing the '100x100' constraint by editing a variable on the settings.php file...

Is this going to 6.x-3.1?

frankcarey’s picture

bump. This would also indirectly solve the issue of not being able to use Imagemagick on Thumbnail creation, right? Though I Know that issue is here #618024: Add support for ImageAPI when creating admin_thumb. But this would add the feature of then being able to use a pre-existing thumbnail size for an image, so probably 1/3 less images if you are using 2 imagecache sizes, and no additional resizing needed when viewing a gallery of thumbnails.

plasticlax’s picture

sub

mani.atico’s picture

Version: 6.x-3.0-alpha4 » 6.x-3.2
FileSize
6.67 KB

This patch is basically the same as #7 except for the following lines which where restored and solved the problem described in the same post, as well as some minor changes to make it compatible with version 3.2

if (isset($element['preview']) && $element['#value']['fid'] != 0) {
   $element['preview']['#value'] = theme('imagefield_widget_preview', $element['#value']);
  }
entendu’s picture

Status: Needs work » Reviewed & tested by the community

Very useful feature. #12 worked for me against HEAD.

quicksketch’s picture

Status: Reviewed & tested by the community » Needs work

The patch needs to follow coding standards. Not sure what's up with this class name:

+    return '<img class="xyzzy' .$item['data']['preset_name'] .'x" src="'. file_create_url($thumb_path) .'" />';
entendu’s picture

Status: Needs work » Needs review
FileSize
6.52 KB

I assume he was trying to ensure a unique class name. Rerolled with something more descriptive.

attiks’s picture

#15, you're right, xyzzy is my debug class ;p

Thanks for cleaning

rjl’s picture

Another reason this would be helpful is that if gd is not installed with jpeg support, thumbnails don't get created form jpg files. The image api allows using ImageMagick for image manipulation.

I don't really know that much about image manipulation, but drupal core appears to have built in capabilities to use other toolkits, not sure why the image api did not build on that infrastructure.

So thanks so much for this patch, as I find myself in a situation where gd is in fact not installed with jpg support, and this patch would make it all work beautifully.

Thanks for the great work.

entendu’s picture

Status: Needs review » Reviewed & tested by the community
quicksketch’s picture

Status: Reviewed & tested by the community » Needs work

This patch isn't ready for committing.

- Needs to following coding standards.
- Instead of adding values into the $item['data'] array we should be simply displaying the appropriate ImageCache preset based on the field setting directly. It would make more sense to add another property to theme_imagefield_admin_thumbnail() instead of sticking things in $item.
- I'm not entirely sure why we've prefixed every ImageCache name with t('Image size'), it seems unnecessarily redundant.

mani.atico’s picture

Built this patch following most of the suggestions made through out this post.

Pending stuff:

-

And we'd need to write an update function that removes all the thumbnails.

(#3) (flushing the preset?)
- I‘m having some issues with my English – texts reviews are needed.
- Depending on the preset's setting, preview images could be wider than the region assigned to them (see attached pic). This is a filefield css problem. Applying max-width to the image and writing some documentation on how to override related css attributes could be a temporary solution.

quicksketch’s picture

I should note that my requirements for writing an update function is only if we decide to make ImageField dependent on ImageCache (which would then make it dependent on ImageAPI also). My preference would be to keep the current functionality intact rather than introduce these dependencies.

juanpox’s picture

Patch works against 6.x-3.2 and works! Well done!

Cyberwolf’s picture

Subscribing.

ryan_courtnage’s picture

FYI, patch fails against 6.x-3.7 (not sure about the process, so not sure if anyone cares)

$ patch -p0 < imagefield-402014_0.patch
(Stripping trailing CRs from patch.)
patching file imagefield.module
Hunk #1 FAILED at 319.
1 out of 1 hunk FAILED -- saving rejects to file imagefield.module.rej
(Stripping trailing CRs from patch.)
patching file imagefield_widget.inc
Hunk #1 succeeded at 126 (offset 3 lines).
Hunk #2 succeeded at 230 (offset 8 lines).
Hunk #3 succeeded at 259 (offset 8 lines).

jenlampton’s picture

Version: 6.x-3.2 » 6.x-3.x-dev
Status: Needs work » Needs review
FileSize
5.92 KB

Rerolled patch, now applies without failing. Tested, it appears to work as intended with a few small syntax changes. I also corrected one spelling error.

jenlampton’s picture

FileSize
6.63 KB

I think it would be a good idea to provide a default preset rather than making the site admin build one. This patch includes a default preset for 'thumbnail', but I think default value of the imagecache preset for imagefield thumbnails is probably still best as "Do not use imagecache". What do you guys think?

quicksketch’s picture

Status: Needs review » Needs work

I'm not sure about the adding a default ImageCache preset, especially at the "thumbnail" location where many users are likely to have a preset already. Maybe something like "imagefield_thumb" would be acceptable?

I do sort of like using a default preset, this makes it so that ImageField doesn't need to generate a thumbnail on-the-fly upon upload, which means less memory errors and fewer HTTP Error 0 problems.

A few issues that need addressing in any case:

- Use sentence case for comments.
- Why is this variable called "$new_parts"? Something like $imagecache_preset would make much more sense.

+    $new_parts = array(
+      '#type' => 'select',
+      '#title' => t('Imagecache Preset'),
+      '#description' => t('This imagecache preset will be used to creeate the thumbnail.'),
+      '#options' => $preview_presets_list, // @str_replace: returns correct node type
+    );
+    $form['preview']['preview_preset'] = array_merge($form['preview']['preview_preset'], $new_parts);

- New line here before "else {". ImageCache should be CamelCased to match its official project name.

+  } else {
+    $form['preview']['imagecache_disabled'] = array(
+      '#value' => '<p>'.t('Enable <a href="http://drupal.org/project/imagecache">imagecache</a> to control the way preview thumbnails should be shown for this field.').'</p>',
+    );

- hook_theme() needs to be updated with the new arguments for all the changed theme functions.

Khalor’s picture

Subscribing, excited to see this in Imagefield

doomed’s picture

Can anyone tell me whats the status on this?

Does imagefield currently have a setting for the size of the thumbnail or is still default at 100x100 px ?

quicksketch’s picture

Does imagefield currently have a setting for the size of the thumbnail or is still default at 100x100 px?

You can always set your own site-specific setting by using the hidden "imagefield_thumb_size" variable in settings.php (just put this line at the bottom of the file):

$conf['imagefield_thumb_size'] = '200x200';

This feature has existed for a long time, but it's not commonly used.

upupax’s picture

Subscribing!
I will test this as soon as I can!
Could this allow uploading of formats other than standard web images?
It would be great!

iantresman’s picture

$conf['imagefield_thumb_size'] = '200x200';

Why not offer this as an override option on an Imagefield Admin page, with the option to use Imagecache if it is installed and working. This would keep everyone happy.

iantresman’s picture

It would also be nice if the Upload thumbnail could optional be linked to Lightbox, as sometimes thumbnails can be difficult to distinguish.

davemaxg’s picture

I'm very interested in using ImageCache for thumbnails. I looked at the patch but it appears that it was for 6.x-3.2 while current release is 3.10. Anyone know why this patch never made it in? Is there a decent workaround? I'm using the autorotate action so my default thumbnails are sometimes oriented wrong. :P

anou’s picture

Subscribing. Also adding an idea :
I'd like to choose the size of the thumbnail for one, but also assign it to a content type in particular. Didn't find anything telling me how to...

anou’s picture

In case my solution helps someone, here what I did to have a special size for a particular content type :

function YOUR-THEME-NAME_imagefield_widget_preview($item = NULL) {
  $preview = '';
  $type = $item['field']['type_name'];
  if($type == 'your-content-type-name'){
  	$preview = '<div class="imagefield-preview">' . theme('imagecache', 'your-preset-name', $item['filepath'], $item['alt'], $item['title']) . '</div>';
  } else { //default behavior
  	$preview = '<div class="imagefield-preview">' . theme('imagefield_admin_thumbnail', $item) . '</div>';
  }
  return $preview;
}

This code is placed in template.php and you must create an imagecache preset before.

*EDIT : Sorry, works only on node creation or when changing the image already uploaded... :-(

qtbeee’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Here's a patch that slightly modifies the patch in comment #26, and implements the comments made in comment #27.

qtbeee’s picture

Oops, messed up the patch. Here's the real one.

gibry21’s picture

I am trying to use imagecache to create a custom thumbnail of a video field, displayed with views.

I want to take the default thumbnail and make it smaller, maybe add an over etc.

Is this a patch for that sort of thing or is there a different way of doing that?

I am using embeddedmedia field with embedded media thumbnail. As far as I can see all this allows you to do is upload ur own thumb...but can I think display in a view and apply imagecache preset to it??

Thanks

jenlampton’s picture

@gibry This issue is not what you want. This issue allows you to use a different ImageCache preset for the preview image that appears in the upload widget on the content type form. It does not affect the images that appear once the node is saved. ImageCache should be able to do what you are asking about, but I'm not sure how it integrates with the emebdded media thumbnail. You may need to override a theme function to get the ImageCache presets to appear with that module (and there may not be a user-interface for choosing which one to use).