This patch is an introductory patch to add all the functionality of ImageAPI and most of ImageCache into Drupal core. Considering better media handling is the #1 requested feature for Drupal core, I think this counts as critical for the release of Drupal 7.

This patch implements the following portions of ImageAPI:

  • Abstracted image "actions", allowing any module to provide new actions.
  • New core actions in addition to the current scale, crop, and resize: scale and crop, desaturate, and rotate.
  • Batch processing of image actions by passing image resources around instead of files.

In addition this patch implements the following portions of ImageCache:

  • The ability to define "presets" for images, combining any number of available actions.
  • Administrative section for configuring image presets and actions.
  • Nearly "on-the-fly" image generation as images are requested.

This patch differs from current approaches in the following ways:

  • Images are generated by calls to image_preset_generate() rather than through a menu callback, so you can't just request an image derivative by visiting it's URL.
  • Images are generated by separate HTTP requests called by the server, rather than the client. This way we get the ability for each image to get it's own PHP instance (and memory limit), while not needing the requirement of Clean URLs.
  • If you don't like the default mechanism for generating images (then we should improve it, but) you can swap out the mechanism entirely to provide the current mechanism that ImageCache currently uses by setting the variable "image_preset_generation_method".
  • Works with or without both Clean URLs and Private Downloads.
  • Thanks to the new hook_file(), image derivatives are automatically deleted whenever the original file is deleted.

And as a sample implementation, this patch adds the following features:

  • Integrates with user.module to allow administrators to select an image preset for user profile pictures.
  • Increases the default user variables for profile image dimensions and size from 85x85 to 1024x1024 and from 30Kb to 800Kb respectively.
  • Enables user profile images and sets up a default image preset for "thumbnail_square" (at 85x85 pixels) in the default install profile.

There are still things to do, but this is a fully working patch. On the docket: write new simpletests for all the new APIs, improve the UI for setting up image presets, and do some performance testing. Not everything needs to be done in this one issue, but the image parts so far are rather cohesive in terms of functionality. The user picture implementation is only a dozen lines or so and is good as an example.

CommentFileSizeAuthor
12.jpg70 bytesJANGNAM_IT
#182 image_ui.patch56.52 KBquicksketch
#181 image_ui.patch56.65 KBquicksketch
#180 iamge_ui.patch50.75 KBquicksketch
#177 iamge_ui.patch50.75 KBquicksketch
#171 balloons.svg_.zip48.1 KBquicksketch
#169 image_ui.patch50.94 KBquicksketch
#168 sample.png174.11 KBquicksketch
#164 371374-164-eojthebrave-imagcache_ui.patch51.36 KBeojthebrave
#161 sample_6-bikeshed.jpg210.87 KBaaron
#160 sample.jpg192.4 KBBojhan
#159 sample.png184.45 KBquicksketch
#158 sample.png176.33 KBquicksketch
#158 sample-screenshot.png49.95 KBquicksketch
#157 371374-157-eojthebrave-imagcache_ui.patch51.51 KBeojthebrave
#157 ic-ui-157.png95.15 KBeojthebrave
#154 ic-ui-154.png102.14 KBeojthebrave
#154 371374-156-eojthebrave-imagcache_ui.patch51.84 KBeojthebrave
#128 image-cache-ui-1-1.jpg283.79 KBDries
#127 d7_imagecache_ui.patch49.95 KBdrewish
#122 image_ui.patch49.68 KBquicksketch
#121 d7_imagecache_ui.patch48.71 KBdrewish
#120 image_ui.patch44.9 KBquicksketch
#119 imagecache_ui.patch44.52 KBquicksketch
#114 d7_imagecache_ui.patch45.93 KBdrewish
#110 d7_imagecache_ui.patch45.92 KBdrewish
#109 imagecache_ui.patch48.88 KBquicksketch
#108 d7_imagecache_ui.patch43.52 KBdrewish
#107 d7_imagecache_ui.patch53.39 KBdrewish
#105 drupal_image_cache_ui.patch54.83 KBquicksketch
#102 drupal_image_cache_ui.patch53.99 KBquicksketch
#98 offset-photoshop.png21.84 KBquicksketch
#97 offset-options.png61.27 KBquicksketch
#97 offset-options.png61.27 KBquicksketch
#94 drupal_image_cache_ui.patch51.37 KBquicksketch
#94 imagecache-new-overview.png110.78 KBquicksketch
#90 drupal_image_cache_ui.patch48.23 KBquicksketch
#87 d7_imagecache_371374.patch40.74 KBdrewish
#76 imagecache_ui-add-preset.jpg22.69 KBeojthebrave
#76 imagecache_ui-edit-preset-action.jpg32.58 KBeojthebrave
#76 imagecache_ui-edit-preset.jpg23.06 KBeojthebrave
#76 imagecache_ui-list-preset-actions.jpg65.01 KBeojthebrave
#76 imagecache_ui-list-presets.jpg22.71 KBeojthebrave
#76 imagecache_ui-preset-add-action.jpg23.18 KBeojthebrave
#76 imagecache_ui-preset-edit-preview.jpg34.41 KBeojthebrave
#76 imagecache_ui-select-preset.jpg28.55 KBeojthebrave
#63 imagecache-in-core.patch88.11 KBdrewish
#62 imagecache-in-core.patch52.91 KBdrewish
#47 371374-46-eojthebrave_imagecache_in_core.patch88.87 KBeojthebrave
#41 371374-40-eojthebrave-imcache_in_core-alternate-menus_02.patch84.66 KBtstoeckler
#40 371374-40-eojthebrave-imcache_in_core-alternate-menus.patch84.71 KBeojthebrave
#40 371374-40-eojthebrave-imcache_in_core.patch83.23 KBeojthebrave
#40 sample.png8.55 KBeojthebrave
#36 371374-36-eojthebrave-imagecahe_in_core.patch81.05 KBeojthebrave
#34 371374-34-eojthebrave-imagecache_in_core.patch79.73 KBeojthebrave
#26 d7_imagecache.patch79.95 KBdrewish
#21 core_image_371374.patch118.2 KBquicksketch
#19 core_image_371374.patch95.03 KBdrewish
#18 core_image_371374.patch96.11 KBdrewish
#17 core_image_371374.patch64.46 KBdrewish
#16 core_image_371374.patch64.55 KBdrewish
#6 drupal_image_handling.patch94.87 KBquicksketch
#3 drupal_image_handling.patch94.86 KBquicksketch
drupal_image_handling.patch94.25 KBquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Senpai’s picture

Subscribe for testing this week.

drewish’s picture

i'd love to see this split into separate patches. because 90k is a lot to poor over and there still aren't unit tests. once those get added it'll easily double.

some quick comments from a skimming the first half:

+ * @param $image
+ *   An image object.
...
+function image_gd_open($image) {

We should explain more about how to get an $image object.

+  $res = imagecreateTRUEcolor($width, $height);

Should be all lower case.

+/**
+ * Load a preset by preset name or ID.
+ *
+ * @param $name
+ *   The name of the preset.
+ * @param $pid
+ *   Optional. The numeric id of a preset if the name is not known.
+ *
+ * @return
+ *   An image preset with the format of
+ *   array('name' => string, 'id' => integer).
+ *   If the preset name or ID is not valid, an empty array is returned.
+ */
+function image_preset_load($name = NULL, $pid = NULL, $reset = FALSE) {

I'm assuming this also serves as a menu loader so we should document that.

+function image_preset_url($preset_name, $path) {
...
+function image_preset_path($preset_name, $path, $file_system = TRUE) {
...
+function image_preset_generate($preset_name, $path) {

needs to document a @return values.

quicksketch’s picture

FileSize
94.86 KB

Thanks drewish. I'll definitely look into splitting up as the tests are created. In addition we also need to document the new hooks in system.api.php (though I think there's just hook_image_actions() which is already documented in image.actions.inc).

This version fixes the notes drewish made above and removes a few remaining references to imagecache.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

FileSize
94.87 KB

Shut it testing bot. Need to pass in NULL instead of '' when calling theme('picture').

Edit: Sorry testing bot I didn't mean it. Please test my patch. ;)

webchick’s picture

Status: Needs work » Needs review

Marking back to "needs review" so it will. :)

moshe weitzman’s picture

Hells yeah. Serscribe ... Multiple patches that depend on each can be just as annoying.

Wim Leers’s picture

Subscribing! Damn you've been churning out a lot of awesome patches quicksketch!

MatthewS’s picture

+1 This is a fabulous idea!

Zarabadoo’s picture

Subscribing to this. This feature seems a very valuable add to the system.

catch’s picture

Absolute no-brainer that this should be in core, no point having image uploads without it. Not looked at patch properly yet.

scroogie’s picture

Really a killer feature. Would love to see this go into core.

chx’s picture

Nate, this is fabulous but 90K+ patches are really hard to fit into the Drupal core development workflow. Reviewers and committers would need to set aside hour or probably hours sized blocks each time they want to review this. Please split this into smaller chunks. As it's mostly new functionality, there is absolutely new problem to submit partial functionality just to get something in.

drewish’s picture

Issue tags: +ImageCacheInCore

I'm going to work on piecing this patch out into smaller issues. I think it makes sense to leave this open as a meta issue to drive attention to the others. Also using the ImageCacheInCore tag to track them.

To kick this off I've just opened #373502: Add function to delete unmanaged files recursively which has tests and lets us remove code from SimpleTest. Two wins that would have been hard to spot in a mega patch.

drewish’s picture

Status: Needs review » Needs work
FileSize
64.55 KB

rerolled dropping the changes that are now part of #373502: Add function to delete unmanaged files recursively so you'll need to apply that to test this. marking as CNW to save the test bot the trouble

drewish’s picture

FileSize
64.46 KB

forgot to fakeadd the new files.

drewish’s picture

FileSize
96.11 KB

grrr.... last try.

drewish’s picture

FileSize
95.03 KB

so the worst part was that three tries later... i posted a version that didn't drop the file_unmanaged_delete_recursive()... this'll be right, i swear.

quicksketch’s picture

Thanks Drewish! An excellent place to start. I'm currently working on writing a new image.test file to unit test our image manipulations. I'll be posting the patch at #373613: Create "Image" Objects, Operate on Images By Resource. I also like the idea of having this thread continuously updated with the complete patch of the remaining parts to implemented. It's much easier to see what we're trying to accomplish when it's all put together.

quicksketch’s picture

FileSize
118.2 KB

Re-roll with the latest version of #373613: Create "Image" Objects, Operate on Images By Resource. Cleaned up code in a few places, better help text, adding extensive UI testing of adding/deleting/editing image presets and actions.

aaron’s picture

I haven't looked at the full version of the patch yet. But I'm wondering if thought has gone into making the (core) imagecache more generic so that transformation settings could in the future be applied to other media types, particularly video? I was already looking into this for emfield and media. I know dopry did some work in this direction some time back with the Transformation module.

RobLoach’s picture

Don't mean to be a scope creep, but using handlers for the toolkits would be an awesome application of the handler system: #363787: Plugins: Swappable subsystems for core.

drewish’s picture

Rob Loach, I'd actually moved the toolkits over to the handlers framework but it doesn't look like it's going to get in any time soon and it doesn't seem like a good idea to tie this to a sinking ship.

Update: I wasn't very clear here. What I meant to say was that I've got an unposted patch that I wrote to test out the handlers patch. I think it'd be a much better way of doing this but since the future of the handlers patch is uncertain we shouldn't make it a prerequisite.

quicksketch’s picture

I'm definitely in the same boat here (continuing with the nautical theme): I'd like to do one thing at a time and get this patch done, then we can convert it to handlers afterwards. Especially not knowing when handlers is going to be finished and our patch is already adequate the way it is now.

drewish’s picture

FileSize
79.95 KB

here's a stacked patch that excludes all the work going on over in #373613: Create "Image" Objects, Operate on Images By Resource.

drewish’s picture

Spent a little time with this and came up with a list of things I'd like to see in this patch:

  1. add the preset preview that's in imagecache in D6
  2. more actions from imagecache. the "deprecated scale" that allows inner and outer dimensions is notably missing. at the least we'll need a way to upgrade the previous actions to the new format.
  3. rename the "New actions" fieldset to "Add actions"
  4. the settings listed on the preset listing form should probably be drupal_ucfirst()ed
  5. rotation background color made optional and defaulting to an empty string. also could we integrate the color.module as a picker if it's enabled?
  6. when adding the desaturate action you're helpfully notified that the action has no configuration and to confirm the addition but once it's been added and you click configure the same message appears and it's misleading.
  7. proper bread crumbs on the preset pages.
  8. the help on the presets should make it clear that x is horizontal and y is vertical
  9. convert all the queries in image.inc into DBTNG.
  10. could we move all the image_actions/preset_*() functions into their own .inc file? image-cache.inc perhaps?
drewish’s picture

Title: Integrate ImageAPI/ImageCache into Core » ImageCache into Core

lets shorten that title up.

quicksketch’s picture

Even without the FileAPI and ImageAPI parts of the patch, we're still weighing in at 80K, probably 100K after adding more tests and docs. For a lot of these items in #27, I'd like to hold off on and polish after the basics go in.

Things that could wait:
- add the preset preview (since I'd like to make this prettier and more configurable than it is in D6)
- more actions from imagecache
- color.module picker (especially since color.module is still completely worthless in this regard, we'd have to do our own Farbtastic implementation).

I'm also iffy about "proper bread crumbs on the preset pages". I had an implementation for this in original patches (#6 and earlier) but it involved nasty hacks to work around breadcrumb limitations. We really need to fix breadcrumbs separately, right now they cease to work whenever you're in any menu item deeper than a MENU_LOCAL_TASK.

I'd vote for "image-actions.inc" as the name of a new .inc file. I agree it'd be good to split out, most of the time you won't need any of those functions unless you're generating an image.

I agree that all the other areas need fixing or improving. In addition to that list, we also need a better mechanism for image generation, as chx informed me that you can't reliably make server HTTP requests to itself (see #245990: HTTP request testing unreliable and may be undesirable). However I have some definite ideas that will help with this.

quicksketch’s picture

New Image Generation Method Proposal:

Currently in this patch, images are generated when theme('image_preset', ...) is called. This is a good thing, but the way the images are generated currently won't work on all servers due the inability of some servers to make HTTP requests to themselves. Here's the new proposal:

  • When theme('image_preset') is called, it fires image_preset_generate() which returns a path to the scaled down image (same as the current approach).
  • Our default implementation inside of image_preset_generate is to check to see if the image already exists for the requested preset. If it exists, it returns the path to the real file and no further work is necessary.
  • If the image does not exist, a Drupal callback menu path is returned instead which will generate the image when it is requested by the browser. This path will include a token "authenticating" the image to be created.
  • The user's browser reads the <img> tag containing the Drupal menu callback and makes a second request to Drupal.
  • Drupal is bootstrapped at this special path (say system/image). The callback sets an entry in a new cache_image table to report that the image is being generated. Drupal attempts to create the image and returns the image directly to the browser upon completion.

If an anonymous user hits a page that contains images that have not been generated, we temporarily disable page caching for that single request. This prevents the page cache from containing the path to our special Drupal image generation callback. Any other pages that are served while the image is being generated (which we'll know by checking the cache table) will point directly to the final image location, even though the image might not yet exist, since we don't want to spawn multiple processes all attempting to generate the image.

This all sounds rather complicated, but I'm fairly certain it'll work out quite nicely. I'll post an updated patch sometime this week.

Stefan Nagtegaal’s picture

Hmmm... We are making progress on the image handling capabilities in D7, right? :-)
/me is going to be back in Drupal business in 5 minutes from now, get up to speed and review this patch...

eojthebrave’s picture

Some thoughts after playing around with the patch in #26.

Current CVS HEAD.

#373613: Create "Image" Objects, Operate on Images By Resource - I didn't do anything with this issue since it looks like the patch was committed to HEAD on 3/10

When adding a new action to a preset it would be nice if you had some indication of what preset you were adding this action to. After clicking to add a new action I was momentarily confused. Perhaps change "Add Scale action" to, "Add Scale action to @preset_name" This could also apply on the Edit action pages, "Edit Scale action for @preset_name"

Maybe the Add/Edit actions should be MENU_LOCAL_TASK items so you don't loose your place in the navigation structure. I found the whole experience of adding/editing actions disorienting.

Not 100% sure what to expect, but theme('image_preset', ...) returns a valid img tag, and what appears to be the correct URL for the image, but the derivative image itself is never created. Seems like I should be able to view images this way.

Question.

What should I be doing to test image generation? Reading through the code it looks like the theme_image_preset function is currently the way to go, but since that doesn't seem to be working I just want to make sure that I'm not missing something.

EDIT: Just realized some of the text from my post above was missing since it was being stripped out by the filter.

quicksketch’s picture

theme('image_preset', ...) returns a valid tag, and what appears to be the correct URL for the image, but the derivative image itself is never created.

That's right, depending on your server configuration, your server might not work with the current image generation method. That's the problem described in #245990: HTTP request testing unreliable and may be undesirable and why I proposed the new method in #30.

eojthebrave’s picture

Okay, after further inspection it looks like the patch in #26 is using image_open() and image_close(), but the code from here #373613: Create "Image" Objects, Operate on Images By Resource that made it into core implemented image_load() and image_save() functions. Changed to these functions in the function image_preset_create_derivative() and everything is working as expected for me now.

The attached patch includes #26, with these two simple changes.

Though, I do think that it is still a good idea to implement it as suggested in #30.

quicksketch’s picture

Ah, excellent, thanks for the reroll eojthebrave, I had forgotten about needing a reroll after that change. :)

eojthebrave’s picture

So here is my rough draft of implementing what quicksketch outlined in #30.

system_image_preset_request() now returns a callback path when an image derivative does not already exist. This is passed to theme_image_preset, which outputs an img tag with this path. No more drupal_http_request().

When the users browser reads the tag and makes a request for this path (system/image/path_to/image.jpg?token= ...) system_image_preset_generate() is called and generates a derivative of this image and then outputs the image and appropriate headers to the browser. Result, the image is displayed for the user after being generated.

In order to accomplish this I had to add two new functions.

image_render() in /includes/image.inc which is a wrapper for image toolkit functions to output an image directly to the browser.

image_gd_render() in /system/image.gd.inc which outputs an image by calling the GD image output functions. imagejpeg(), imagepng() etc.

We still need a way to disable the page cache for a single page request. I'm not really sure how to go about this so any ideas would be appreciated. We also need to create a cache_image table as mentioned in quicksketch's original outline so that we are not trying to create the same derivative more than once.

Question. Is there any reason to be concerned about printing someone's session id in the callback url? (system/image/100/argument.jpg?token=9912fbe24c8dd3d597b9dbfb50fb7ce3&session=3311d6b8a4bd567a1a9896ce958fcfad) I'm not really sure if this raises any issues with security or not?

quicksketch’s picture

Thanks eojthebrave, I appreciate the reroll! Here are some suggestions:

In order to accomplish this I had to add two new functions...

Why not just use file_transfer() after the image is saved to disk?

We still need a way to disable the page cache for a single page request.

Well, the tricky way to do this:

// Disable page caching only during this request.
$GLOBALS['init']['cache'] = CACHE_DISABLED;

Of course this seems like a terrible hack. Perhaps if we were to put it in a function like page_temporary_cache(). I have a bad feeling about this becoming a quagmire. :\

Is there any reason to be concerned about printing someone's session id in the callback url?

Yes, let's not do this. The original implementation needed the session variable because the server was making the request to itself. This is no longer necessary since we'll already know the user's session since they're making the request themselves now. So all we need to keep now is the "token" parameter and we can use the normal drupal_get_token() function to validate it.

catch’s picture

Disabling the page cache for a single request is also an issue at #339958: Cached pages returned in wrong language when browser language used (which needs a backport) and maybe elsewhere - we need a generic way to do this, and not in this issue.

dman’s picture

Hi guys!
Re #27, you don't need color.module to use the farbtastic color picker. It was really easy to hijack into any form!
code from imagecache_actions here

function theme_imagecacheactions_rgb_form(&$form) {

  // Add a farb element
  drupal_add_css('misc/farbtastic/farbtastic.css', 'module', 'all', FALSE);
  drupal_add_js('misc/farbtastic/farbtastic.js');

  $hex_id = $form['HEX']['#id'];
  $form['farb'] = array('#value' => "<div id=\"$hex_id-farb\" style=\"float:right\"></div>", '#weight' => -1 );

  // Adds the JS that binds the textarea with the farb element
  $js = "
  $(document).ready(function() {
    farbify($('#$hex_id'), '#$hex_id-farb');
  });

  function farbify(elt, wrapper) {
    var farb = $.farbtastic(wrapper);
    farb.linkTo(function(color) {
        elt
          .css('background-color', color)
          .css('color', this.hsl[2] > 0.5 ? '#000' : '#fff')
          .val(color.substring(1));
      });
    farb.setColor('#' + elt.val());
    elt.bind('keyup', function(){ updateColor(elt, farb); });
  }
  function updateColor(elt, farb) {
    var text = elt.val();
    if (text.length == 6)
      farb.setColor('#' + text);
  }

  ";
  drupal_add_js($js, 'inline');  
  $output = drupal_render($form);
  return $output;
}

I'm not doing a lot of D7 myself at the moment, but am quietly encouraging the guys doing signwriter, menuwriter (and a few others) to learn from the imageAPI image-handle method of getting things done in the hope that we can end up with a common pipeline for text-image formatting. I may be able to throw together a proof of concept rough syntax demo for that sometime soon.
I really like the imageAPI approach and am looking forward to it being common ground. (although it caused me much swearing last week when I found ->res changed to ->resource !! )

eojthebrave’s picture

Alright, here we go again.

This patch now uses the file_transfer function to send images to the users browser when requested via the image generation callback path. Much cleaner than the way I was doing it before. Thanks quicksketch.

There is now a cache_image table so that Drupal is not trying to create the same derivative more than once at the same time. Currently this check is implemented in system_image_preset_generate(), we might want to consider moving it to image_preset_create_derivative() so that alternative image toolkits make use of this by default. Thoughts?

As per quicksketch's recommendation in #37 the session id variable has been removed from image generation callback paths.

Addressing issues from drewish's comments in #27

Added a preview for presets with one or more defined action. This requires the attached sample.png file to be placed in misc/sample.png. I'm not sure how (or if it is possible) to do this with a patch file. So, in order to test this you'll have to download the attached sample.png file and place it into your misc/ directory.

"New actions" fieldset renamed to "Add actions"

Uppercased words like "Height", and "Width" in action settings summaries.

Removed "configure" link from the table listing a preset's actions if $action['data'] is empty. Replaced with with "n/a". So for actions like desaturate that do not have any configuration options you are never given the option to configure them.

Changed "X offest", and "Y offest", to "Horizontal offset", and "Vertical offset" respectively.

With regards to the rotation actions background color settings. The imagerotate() function requires a background color argument. I played around with it a bit and passing NULL, or 0 to it always gave me a black background. Since I'm guessing that the majority of people would want a white background as the default I didn't change this at all. Suggestions or thoughts?

Since the overall menu/navigation structure for the image presets seems to be an issue that is bothering both drewish and myself I've tried to fix that as well. This required some pretty significant changes to the way the menu/navigation stuff was being handled in this patch previously so I've attached a second patch which implements the following changes along with all of those mentioned above.

First I moved the image toolkit settings to their own menu item at admin/settings/image-toolkit

In order to get more intuitive navigation with proper breadcrumbs and active trails I've ditched the wild card menu loaders and implemented the menus more like cck does when dealing with node types and fields.

admin/settings/image-presets lists all the currently configured presets and a tab for adding new ones.
admin/settings/image-presets/

allows users to edit the presets name and view a preview image. admin/settings/image-prests//actions (MENU_LOCAL_TASK) - shows the table for viewing/re-ordering actions and options for adding new actions to a preset. Each configured action is rendered as a secondary tab of the actions tab. This seems to be working really well, makes the navigation of the whole image preset creation and editing much more intuitive, and means both breadcrumbs, and active-trail are set properly. It does however mean a number of calls to menu_rebuild(), and a bit messier implementation of system_menu(). This is the best way of handling this that I was able to come up with at this point. If someone has better ideas I'm open. I really think figuring this out is crucial for this patch. Things that are still left to do. Figure out how to disable page caching for a single page request. DBTNG for image.inc Better help/documentation both user facing and in the code. Breaking this up into smaller patches so it is easier for people to test/review. I'm willing to work on this, but don't really see any logical places to split it up? Suggestions?
tstoeckler’s picture

Patch didn't apply for me because the paths in the patchfile were broken. I fixed that so that the patch applies cleanly for me now.

I also changed the help text, when there are no actions defined, to reference the Add action fieldset, not the New action fieldset (which doesn't exist anymore...).

No changes other than that. Great patch!

eojthebrave’s picture

tstoeckler. Thanks for updating that paths in my patch. I'm still learning my way around git and creating patches.

I've done a bit more poking around and figured out another way to deal with the active trail/breadcrumb trail issue. We could leave the menu stuff in system_menu alone and add something like this to system_image_preset_form.

   // Set appropriate menu trail.
  $item = menu_get_item();
  $item['href'] = 'admin/settings/images';
  menu_set_item(NULL, $item);
  drupal_set_title($item['title']);
  // Set appropriate breadcrumb trail.
  drupal_set_breadcrumb(array_merge(drupal_get_breadcrumb(), array(l(t('Image handling'), 'admin/settings/images/presets')))); 

This would get us usable breadcrumbs, and keep the admin > site configuration > image handling menu item active.

Personally, I still favor approach in #40, I think it makes the whole thing look and feel cleaner. Would love to get some more opinions on this.

SeanBannister’s picture

sub

axyjo’s picture

Subscribing.

Stefan Nagtegaal’s picture

Most interesting, testing things at the moment...

drewish’s picture

I've been working with this patch a bit trying to remove the $static variables and $reset parameters in favor of drupal_static() and drupal_static_reset(). All I can say is that it seems like we're doing a lot of unnecessary caching at too many levels. There are 2 cache tables and 5 static variables. I think this might be some premature optimization that adds more complexity than it gains in performance. I'd like to have quicksketch weigh in on this because I'm tempted to drop most of the statics in favor of the cache tables.

eojthebrave’s picture

Spent some time going through all the documentation for new functions etc included in this patch. Here's a summary of what I changed. There were a number of small typos that I didn't document below along with a few minor changes here and there.

image.inc

comments for image.inc image_preset_options()

document $reset on image_preset_load
modified return value for image_preset_load

unnecessary space in image_preset_save comments

added return info for image_preset_delete

added return info for image_preset_actions

image_preset_path final @param should be @return

missing @param $toolkit for image_action_definitions

added return info to image_action_save

added @params and @return to image_action_apply

image.actions.inc

improved documentation for system_image_preset_request

added documentation for system_image_resize_image, system_image_resize_image, system_image_crop_image, system_image_scale_and_crop_image, system_image_desaturate_image and system_image_rotate_image

system.admin_image.inc

system_image_resize_form had an argument of $action, but documentation for $data. Changed argument to $data, and updated all references in function so that it is consistent with other similar functions.

added documentation for system_image_desaturate_form

standardized documentation for theme_system_image__summary functions

Question:

There are a few functions like this function system_image_action_delete_form($form_state, $preset, $action) throughout the code, is it necessary to document $form_state for these or is that just assumed knowledge?

drewish’s picture

eojthebrave and I'd talked on IRC and I'd mentioned that there were still a mixture of links to settings/image-presets and settings/images/presets that still need to be fixed. there's also a couple of DELETE queries that need to be converted to DBTNG:

db_query('DELETE FROM {image_actions} where ipid = %d', $preset['ipid']);
db_query('DELETE FROM {image_presets} where ipid = %d', $preset['ipid']);

becomes:

db_delete('image_actions')->condition('ipid', $preset['ipid'])->execute();
db_delete('image_presets')->condition('ipid', $preset['ipid'])->execute();

and:
db_query('DELETE FROM {image_actions} WHERE iaid = %d', $action['iaid']);
becomes:
db_delete('image_actions')->condition('iaid', $action['iaid'])->execute();

In regards to his question in #47, I'd say for functions that clearly implement forms it's not necessary to document each parameter, in the same way we don't document the parameters of hook implementations.

We still need to deal with the static caching but that's another issue.

webchick’s picture

Since it's too late for me to do a thorough patch review, I'm going to be annoying (better to do this sooner than later :P) and just point out that I hate, hate, HATE hate, hate hate hate HATE, HATE *HATE* the word "preset." Can we please use a word that means anything at all to normal humans (since it is exposed both in the UI and at the theme layer), such as:

- Image Transformation
- Image Effect
- Image Format
- Other?

dman’s picture

Can't be those three you suggested, as they are each more atomic, and refer better to the individual effects or filters.
'rotate' is a transformation.
'dropshadow' is an effect
'blur' is an effect (or filter)

the imagecache presets are recipes that string one or more effect together. "resize to 200x100, rotate by 20' and add text caption"

I'd call it a 'process' or a 'pipeline' but I don't think either term is any more explicable.
'Macro' or 'script' describe the internals, maybe, but also not useful words.
"Instruction set'? Ug.

catch’s picture

Out of those, process seems best, but I agree it's not really any better than preset.

dman’s picture

'profile' is in use elsewhere for similar presets, but overloaded already, and doesn't help.
Thinking about it, 'transformation' isn't too bad, as long as we recognise the difference between that and 'transform' (which is what I really meant above)
:-/
That just illustrates the problem with the word really!

Process is a fine word, but doesn't noun properly. To my dialect. That is, it's already mostly a verb. To process an imagecache process is not a good function call.

catch’s picture

transformation means we'd have URLs like files/imagecache/transformation/thumb doesn't it?

This is the same problem we have with filters and text formats.

eojthebrave’s picture

What about operations, or conversions.

Image operations are a collection of actions that are applied to an image in order to create a derivative of the base image.

image_operations_load, image_operations_apply ... etc

Image conversions are a set of actions that convert a base image creating a derivative of the original ....

image_conversions_load, image_conversions_apply, image_convert ... etc ...

Of the two I think I like operations better.

quicksketch’s picture

Keep in mind that this word will need to be used throughout Drupal where you need to select a preset.

For example, this patch already includes a preset selection for the display of user pictures. Do these words make sense when pulled out of context and used as the label for a select list? I personally still think "Image preset" is the best word we have versus any of the suggestions.

webchick’s picture

Ok, so as to not further derail this issue (but I appreciate the folks putting their heads together on this!) let's bikeshed this over here: #440468: Bikeshed the word image "preset" . Once we come to a consensus we can come back to this patch and do a "simple" name change, and in the meantime drewish and others can work on the technical stuff.

Stefan Nagtegaal’s picture

I rather use the word "Image actions" or "Image task list", how about that?
We apply (different|an) (actions|action) or (tasks|task) (rotate, crop, resize, regenerate, copy, rename, ) to an image afterall...

drewish’s picture

Bah quicksketch... why did you have to have bug fixes in here that didn't make it into patches against ImageCache. I've found some stuff over there that I've fixed but I have no idea how to start merging the changes.

Stefan Nagtegaal’s picture

@quicksketch, drewish: How can I help in this area to get this in? I lost track between all the issue, and do not quite understand which issue needs to be solved first before any other..

drewish’s picture

personally, i'd prefer to port imagecache to d7 in contrib and figure out the new derivative creation system there then work on adjusting the naming to get it into core.

drewish’s picture

Status: Needs work » Postponed

okay, talked to quicksketch and came up with a game plan for getting this going again.

we're going to open a new 6.x-3.x branch for imagecache and set it up as a backport of the core code. the include files will just get moved and the functions will get an imagecache_ prefix. imagecache_ui.module will disappear since the code from system.module will go into imagecache.module.

drewish’s picture

FileSize
52.91 KB

Decided I'd do a re-roll to keep this current and fix a couple things in the process:
* Converted the delete queries to DBTNG.
* Brought some updated logic for image_preset_create_derivative() in from ImageCache and was able to drop image_filter_value() as a result.

drewish’s picture

FileSize
88.11 KB

forgot a few files.

i'm having second thoughts about the plan quicksketch and i came up with. i'm tempted to fork off the preset code to its own issue and leave the UI parts here...

quicksketch’s picture

I'm fine with forking the patch and getting the API in first. Webchick and I chatted earlier and she was saying that she'd be happy with just the API in core. I do not think this is a good idea. Since I can't imagine it being much use at all being in core without a UI to change the values of presets (such as the thumbnail size or deciding whether or not to crop). Having the API in core isn't going to increase adoption any further than ImageCache already is accepted, since every module that needs thumbnailing (other than Image) already depends on ImageCache anyway. I'd much, much rather have all or nothing, though getting in the API first then the UI later is fine with me.

webchick’s picture

Well, more accurately, I think that the imagecache UI needs a whole lot of work to be "core worthy," and is likely to hold up the patch if they're not separated. I also think it'd be interesting to see how other people might envision a UI for creating presets in contrib once the API parts are in core. And finally, the argument could be made that ImageCache UI doesn't really fit in core at all since defining image presets falls pretty squarely in the "power user" tools.

My ultimate preference of course would be to replace user pictures with an imagefield that uses a 75x75 square crop formatter, not get in a big, complex preset definition UI just to support a feature that's implemented in a legacy/deprecated way. I understand that this is a lot of work, however.

I'm not really 'hard line' on any of this; it's just my personal preference.

moshe weitzman’s picture

IMO, the fields system is sufferring greatly because there is no core UI for it. Thats a main reason that few are playing with it. Playing is a precursor to improvement. Lets not repeat this mistake with imagecache ui.

yoroy’s picture

I'm reading here that lack of UI is holding back important core stuff? I'd be happy to get some UI designers eyes on this if someone could do a little brief on what's needed. Find me in IRC, just saying.

kika’s picture

Agreed, bring the requirements on!

webchick’s picture

Would probably be helpful to post some screenshots here of the current imagecache UI, including both the administrative back-end to define a preset and actions, as well as an example of it being applied as a CCK formatter or within Views.

If someone has time to do this, that'd be awesome! Else I can try and do so this weekend.

scroogie’s picture

What about the imagecache documentation? It contains screenshots:

http://drupal.org/node/163561

eojthebrave’s picture

The existing imagecache module's UI is similar, but still different then the way we are currently doing things in this patch. Certainly good reference material though.

I would also like to chime in and say that I agree with those above who have said that getting the API into core and no UI is really limiting the usefulness of this patch. I think that if we're requiring users to install a module to get a UI for this functionality we're really not going to reach out to anyone that doesn't already use ImageCache, and the whole idea is to bring image manipulation to a larger audience.

I've got nothing against breaking it into to patches though and getting the API stuff committed first as long as we're feeling pretty confident that the UI will follow shortly. Actually, in the end, even if we do only get the API in it's still a step in the right direction.

Better media handling in Drupal++

tstoeckler’s picture

Concerning #65: Image preset functionality is not necessarily "power user" stuff. What you can currently do with ImageCache if you leverage its full potential without doubt falls into that category. But even if you have a simple blog-site, and you want to have the images in your posts all styled in a certain way (without a billion hours of workload) you already need something like that. And then maybe you want the user pictures to all have a different size and maybe to crop them a little so that the face really shows. Then you need it even more. And it doesn't stop there. And imagine the power of Drupal if we were to satisfy all those specific needs just like that with our core package.
(If you didn't notice: Read this as "+1 for having a UI in core" (eventually...))

scroogie’s picture

I second that thought tstoeckler. At least the 'scale to custom dimension' should somehow be accessible.

drewish’s picture

First let me say I'm all for having a UI in core. But if the choice comes down to just the API into core or nothing, then I want the API in. Personally--and speaking as the current ImageCache maintainer--I think the current UI is crap. I'd be very excited to fork it off and have the UI folks spend some time on it and come up with something great rather than trying to have the API and UI discussions in the same thread.

So I'm going to leave this thread to the UI folks and create a new thread for the API discussion.

tstoeckler’s picture

Totally agree. I just read #65 as "Maybe it's not worthwhile to have a UI in core at all" and wanted to oppose anything going in that direction :-)

eojthebrave’s picture

Screenshots of what we've got working currently as a UI for this stuff. This is reflective of what you would be seeing if you applied the most recent patch and started playing around.

This list should also help to define most of the possible tasks a user would need to accomplish.

1. List all currently defined presets, and options to add new ones and edit/delete existing ones as well as flush all previously generated derivatives for a specific preset. see imagecache_ui-list-presets.jpg

2. Add a new preset. see imagecache_ui-add-preset.jpg

3. Edit an existing preset. see imagecache_ui-edit-preset.jpg - The current edit page also shows a preview of what an image will look like after this preset has been applied. see imagecache_ui-preset-edit-preview.jpg

4. List all actions currently defined for a preset, along with options to re-order add, edit, and delete actions. see imagecache_ui-list-preset-actions.jpg

5. Add an action to a preset. see imagecache_ui-preset-add-action.jpg, note that the fields provided on this form vary depending on the action being added and what options that action provides.

6. Edit a preset action. see imagecache_ui-edit-preset-action.jpg

7. An example use case where a user would need to choose a preset to use to format an image. see imagecache_ui-select-preset.jpg - Currently the only place where this is implemented is choosing formatting options for a user's profile picture. However, this will eventually be used in a variaty of places. CCK field formatting, views field formatting, any module that allows image uploads and allows you to choose what format they should be displayed in.

I think that covers everything, though if it seems like anything is missing let me know and I'll either confirm that it is in fact missing from the current UI, or provide a screen shot.

webchick’s picture

@eojthebrave: Rock! Thanks so much!

tstoeckler’s picture

If I read the last few posts correctly, here is the place to start UI discussion, so here I go:
The numbers refer to #76:

1. The list of presets pretty much resemble e.g. the role list at admin/user/user, which also has "Add foo" as a local task, so nothing to complain here. The only thing that's different is that there are no filters, but those wouldn't make much sense here. What is missing though, is a help text at the top. The Edit, Delete and Flush options are also nothing new in the Drupal admin. The word "Flush" though could be confusing to new users, this could be explained in the help text above.

2. Similar to admin/user/roles/edit, so nothing to complain really. I would like the part about the URLs to have an example, though, because right now it is unclear what is meant. Maybe: "The name is used in URLs for generated images. If http://example.com/image is the URL of an image on your site, http://example.com/image/presetname will be the URL of the corresponding image preset, where "presetname" is the the name of the preset you entered above." Actually that's a horrible example because it is way to long and way to complex but maybe someone can come up with something more intuitive along the same lines.

3. The same as for 2. applies here. The "Actions" local task actually makes sense, although I'm not sure, we have something like that in core yet. Fields UI has it with content types and fields, but it is also not very intuitively solved ther. Roles and permissions which have a similar hierarchy/relationship solve it differently, although it would be a horrible idea to make the actions page a huge table. I think if we could solve this problem (the problem of having to edit two different types of objects and edit the hierarchy of the two) nicely we would automatically be 10 steps further in having a nice Fields UI.
I didn't get that preview part, I'll have to try that out, so no comment on that.

4. There is nothing like that in core yet (see 3.), but I like this actually pretty much. The list is similar to all other lists in Drupal, except for the Settings column, but that is fine IMO. I also like the fieldset below very much. That is what I have always wished for admin/content/node, except that it doesn't scale for many nodes. Here it does, because you won't have that many actions assigned to one preset. What is confusing, though, to my mind is the second level of local tasks. Without having tried it out, I actually can't see what is different in clicking them or the "configure" link in the list.

5. On its own this page is fine. Very similar to 2. The problem is how you get there intuitively, see 3.

6. The same as for 5., except the local tasks. These could be a little confusing, because they are really more of a breadcrumb in this situation. This is the problem of intuitively displaying the hierarchy of presets and actions as described in 3.

7. I love it! If I were to see this in Drupal 7, I would sing and dance for weeks. That's exactly what makes this patch so awesome, to be able to configure for instance the user pictures in such detail.

tstoeckler’s picture

Reading my post again, I just realized we do have something like that (two different object types, both configurable, and their relationship configurable) in core: Actions and triggers. The only difference is that you can't add or edit triggers, but that is a marginal problem, I would say.
All this would need is to have image actions defined seperately at a different place. And then you could assign actions to presets, just like you assign actions to triggers, which is pretty intuitive IMO.
A potential problem, lies in the separation of actions and presets, because it will maybe be hard to grasp what image actions are for, if you define them just like that, because they don't do anything just like that. But the same goes for the current actions / triggers, so I'm not sure if that speaks for my idea or against actions and triggers.

yoroy’s picture

Great.

I'll take advice on where to do the initial round of UI sketching. Mixing UI work with actual functional (API?) changes proved to be disastrous for the Help system patch. Let's be certain to prevent that from happening here. Open a new issue?

tstoeckler’s picture

From #74 by drewish:

So I'm going to leave this thread to the UI folks and create a new thread for the API discussion.

webchick’s picture

Yes. Still though; 80 replies in is usually not the best time to start a major UI re-working initiative. I'd suggest spinning off into a new issue as well, and probably either set this one to postponed on the other two or won't fix it since it eats baby kittens.

moshe weitzman’s picture

So, where is this new discussion? I'd hate to see this stall after so much work.

webchick’s picture

I think the discussion is focus on getting the API parts in first, since that needs to happen either way. After that point we can discuss whether we move in the UI parts or not.

eojthebrave’s picture

I'm booked through next week, but will take a stab at splitting the API/UI stuff in this patch when I return if no one else has done so.

@drewish, have you started on this already?

Anonymous’s picture

subscribe, i've cloned drewish's git repo.

drewish’s picture

Status: Active » Needs work
FileSize
40.74 KB

okay I've forked the API parts of this out to #491456: ImageCache preset API so here's a patch with the remaining UI bits for use once the api is committed.

mcrittenden’s picture

Subscribe.

egfrith’s picture

Subscribing.

quicksketch’s picture

FileSize
48.23 KB

Here's a patch that matches #24 from #491456: ImageCache preset API.

dopry’s picture

Speaking as the original imagecache maintainer... The UI stinks... better than the original UI, but I'll pretend that never happened. Get the API in core, then contrib can fight for the best UI.

btw awesome work to everyone who has contributed to imagecache. I like when I revisit to see that people still find it useful. :)

Bojhan’s picture

subscribe

quicksketch’s picture

Marked #60987: User picture thumbnails as duplicate.

quicksketch’s picture

Title: ImageCache into Core » Add ImageCache UI Core
FileSize
110.78 KB
51.37 KB

This patch corresponds to #34 in #491456: ImageCache preset API.

This patch completely revises the overview for editing a preset's actions. See the attached screenshot. It provides a way to give the user feedback about the scaling actions that occur without taking up a massive amount of screen real-estate. I merged back together the Actions and Edit tabs into a single interface. Breadcrumbs are now horribly mangled, but that is being corrected in #483614: Better breadcrumbs for callbacks, dynamic paths, tabs.

Bojhan’s picture

Starting to look good, wondering if you can remove the text preview by making it more explicit what is clickable.

quicksketch’s picture

Patch to match #38 in #491456: ImageCache preset API.

Adding support for renaming/deletion of image presets.

quicksketch’s picture

FileSize
61.27 KB
61.27 KB

Here's a request for opinion, which approach is better in regards to "Image anchors" (or offsets):

- A grid of 9 checkboxes (a la Photoshop crop)
- Textfields for offset values (either integers or special keywords like "center", "top", "bottom", "right", "left").

Granted Photoshop also has nice little arrows, but that involves adding more images (an odd restraint I know...) meaning it might come down to (gasp) an images directory inside a core module to hold extra things that probably don't belong in misc.

quicksketch’s picture

FileSize
21.84 KB

Ah crap, here's the Photoshop image. Didn't mean to attach the same thing twice.

axyjo’s picture

I would have to go with the grid of 9 radio boxes. That way, there's no ambiguity for things such as top_left vs top left vs topleft and other variations.

eojthebrave’s picture

Re #94. When someone selects to add a new action to the preset can the form for adding the action show up right below the table, rather than having to go to another page to add the action?

How about something similar with editing actions?

Since all of the configuration forms are just a few fields I think it would be nice to try and figure out how to accomplish as much of the adding/editing/configuring on one page as we can.

I also like the 3x3 grid option. I do find those things a bit confusing, even after having used photoshop for years I have to stop and think about it. But, it is much better than the text fields for sure.

Looking good.

yched’s picture

re #100: this looks like a nice use case for #492834: Hover operations for flooded state screens. I think such refinements could go in followup patches, though, depending on what actual UI patterns and tools actually make it in D7.

quicksketch’s picture

FileSize
53.99 KB

yched: Nice! #492834: Hover operations for flooded state screens would be fantastic in this situation. This would make it so that we could keep consistency with other UIs throughout Drupal (like the filter and role configurations). Either way we'll need these configuration forms in place initially no matter what we end up doing with them (modals, accordions, etc.).

This patch tidies up a lot of the configuration forms, setting appropriate maxlength and size properties for example. It also adds validation to all our integer values (width, height, rotate) and validation on hexadecimal colors. Lastly this patch removes the form entirely for the Desaturate action, since it didn't have any configuration anyway.

SeanBannister’s picture

Re #97: I'm a big fan of the grid of 9 checkboxes, there's no way I would ever figure out "Textfields for offset values" and would have to read the documentation each time. Where as with the grid of 9 checkboxes I'd have a visual image of what it means and what it did last time I used to.

catch’s picture

I also like the radios grid there. I use imagecache, and get the text field, but still have to read the docs to remember what to put there (avoid putting UK English spelling for 'centre', center_top or top_center etc. etc.).

Really like the screenshots in general too.

quicksketch’s picture

FileSize
54.83 KB

This patch matches #41 in #491456: ImageCache preset API.

This version implements the grid-style selection for cropping.

meba’s picture

Problem with patching:

jimmy@galatea:~/www/l10nserver$ patch -p0 --dry-run < drupal_image_cache_ui_2.patch
patching file modules/system/system.module
patching file modules/system/admin.css
patching file modules/system/system.info
patch: **** malformed patch at line 235: Index: modules/system/system.test

drewish’s picture

FileSize
53.39 KB

here's a re-roll since i had the same problem with quicksketch's patch.

i've put this code into the github repository that i'm using to track this and the preset api patch: http://github.com/drewish/imagecache_d7/tree/ui

if people want to try it out:

git clone git://github.com/drewish/imagecache_d7.git 
git checkout --track -b ui origin/ui

the patch is generated from the difference between the ui and master branches:

git diff --no-prefix master ui
drewish’s picture

FileSize
43.52 KB

quicksketch had merged the patches again in git but I got them separated back out. Leaving this as needs work since the test bot won't have applied the other patch.

Over on #491456-53: ImageCache preset API eojthebrave had a lot of good comments that I'd addressed. There were some that I'd ignored that I've moved over here since they are UI specific.

Scale effect should validate that either height OR width is filled in.

This should probably be fixed so that we don't end up with something like.

function theme_image_scale_summary($data) {
  return theme('image_resize_summary', $data) . ' ' . ($data['upscale'] ? '(' . t('upscaling allowed') . ')' : '');
}

x100 or 100x or x

Include Anchor information in theme_image_crop_summary.

100x100 Top Left or something like that.

The rotation summary should also explain that random rotations will be between negative and positive angle.

quicksketch’s picture

FileSize
48.88 KB

This patch builds on the changes drewish completed above and adds significantly more help text and a hook_help() implementation. It renames the "description" property of effects to "help", and then uses that text when editing/adding that effect.

drewish’s picture

FileSize
45.92 KB

re-roll after i moved some of quicksketch's changes over to the other branch/patch.

Bojhan’s picture

I am finding it hard to review this from a UI perspective, is there a demo somewhere I could try or updated screenshots?

eojthebrave’s picture

Demo site based off what is in the ui branch of drewish's github repository.

http://dreamformula.net/d7sandbox/imagecache_d7/
demo / ic-demo

I will try and keep it up-to-date.

Things to check out are Administer > Site Configuration > Image Styles

I believe there is also supposed to be a widget that allows you to choose which image style to use when rendering a user's profile picture at Administer > Site Configuration > Users, don't have time to poke around at the moment though. (drewish? quicksketch?)

Discovered a bug in the code while trying to get this working. image_perm() should be image_permission()

tstoeckler’s picture

Very clean interface, I especially love the preview functionality.
One thing I noticed. People that have never installed imagecache before, might not know what is meant by "flushing" a style, and what the difference is to deleting it. Maybe add a description to hook_help.

drewish’s picture

FileSize
45.93 KB
I believe there is also supposed to be a widget that allows you to choose which image style to use when rendering a user's profile picture at Administer > Site Configuration > Users, don't have time to poke around at the moment though. (drewish? quicksketch?)

Yeah there should be—though it'll should show up in the API patch. I'll make a note on that issue.

Discovered a bug in the code while trying to get this working. image_perm() should be image_permission()

Fixed that.

drewish’s picture

tstoeckler, Yeah I started to do some help on the whole flushing but and then realized that we might be better off to just see if we can avoid displaying it entirely. How often do people use it? It seems like it would be better to focus on making sure that the flushing happens automatically at the correct times.

In the process of testing when the flushing happens I found myself a bit confused but the interactions of the Add and Update buttons on a style. Adding effects happens immediately and flushes the cache. Renaming and reordering require you to click the Update button and also flush the cache. So it seems like if you know you need to flush the cache you can just resave the style and we avoid confusing people about flushing.

tstoeckler’s picture

Re #115: I have always wondered what a use-case would be for that! ...Other than the mythical "debugging". So if we can find a way around that, very good!

quicksketch’s picture

So it seems like if you know you need to flush the cache you can just resave the style and we avoid confusing people about flushing.

Very true, let's drop the "Flush" option from the UI. I actually like to set up a Rotate 360° (random) action, then repeatedly click the "Update preset" button to see the preview image change with different rotations. :-)

NancyDru’s picture

subscribe

quicksketch’s picture

FileSize
44.52 KB

Matches #491456-58: ImageCache preset API.

- Removed "Flush" operation, since images are flushed regularly anyway just be editing any property of a style.

quicksketch’s picture

FileSize
44.9 KB

Matches #491456-60: ImageCache preset API

- Code style cleanup
- Better PHPdoc

drewish’s picture

FileSize
48.71 KB

moved some more stuff into this patch from the api patch. rerolled against #491456-69: ImageCache preset API.

quicksketch’s picture

FileSize
49.68 KB

This patch makes some more of the changes as suggested by Webchick/Dries:

- Renames $effect['effect'] to $effect['name']
- Renames 'form' and 'summary' to 'form callback' and 'summary theme', respectively

And it also:
- Cleans up the help text
- Revises the user.module picture configuration for clarity between "upload" settings and "display" settings

tstoeckler’s picture

If I've read the issue correctly (haven't applied patch), the current behaviour is:
If image module is disabled, users cannot select an image to style for their user pictures, but they can still enable user pictures.
I would suggest, that instead of hardcoding some fallback mechanism into user.module, it would be easier to simply disallow user pictures if image.module is disabled. That makes sense on a conceptual level as well: If you don't have image module enabled, you can't have images on the site. Period.

NancyDru’s picture

Nope, that makes core dependent on a contrib.

NaheemSays’s picture

?

image.module would be in core.

quicksketch’s picture

This issue combined with #491456: ImageCache preset API claims the namespace "image.module" for core. There's more information in the Image module queue: #513096: The Future of Image in Drupal 7.

tstoeckler, I think that's a very tempting option. Ultimately what we might do is make User pictures a field that is attached to users, which would have the effect your describing. However we don't yet have ImageField (or FileField for that matter) in core, so making them dependent on each other may come soon, but at the current state, it's not yet necessary. A very good idea to keep at hand though.

drewish’s picture

Status: Needs work » Needs review
FileSize
49.95 KB

here's a re-roll now that the api got committed.

Dries’s picture

FileSize
283.79 KB

Some feedback:

quicksketch’s picture

Thanks Dries! Responses:
- Captialization: good call.
- Rotating randomly can sometime produce images that aren't hardly rotated at all, looks like it's rotated about 2-4° in your screenshot.
- Preview labels: I tried things like "Before" and "After", or removing entirely, but after trying different things I found having the name of the style to be most informative and least confusing as to what's happening. Maybe there's a better approach, but it was the best I could think of.

Dries’s picture

Ah, then I'm confused. The screenshot is taken from your test site. The text on the screenshot reads: "Rotate 30° (random)" -- I interpreted that as, rotate 30 degrees in random direction. That seems to be at odd with the settings though. How can it rotate 2-4° when it is set to 30°?

Bojhan’s picture

@quicksketch I think its a good call, to use the name a person has just filled in. You recognize the name, hence making the connection with how it will actually look - I am trying to grasp how to see the alteration image in full size though. It doesn't seem clear its clickable.

drewish’s picture

Dries, the random checkbox does a random angle from -30° through 30°. The current help text for that option is bit vague:

Randomize the rotation angle for each image. The angle specified above is used as a maximum.

drewish’s picture

@bojhan, you don't think people will actually read the bit that says:

Preview images are scaled. Actual image sizes may be viewed by clicking on the preview.

Dries’s picture

- I overlooked that help text. I think it illustrates a usability problem though because many people will overlook it. In fact, given that there will be a default thumbnail style, most people will never actually see the form descriptions. As such, fixing the label in the overview page seems more important than fixing the description on the settings form. Maybe the label on the overview page could be changed to: "Rotate: random between -30° and 30°"? It might be a bit long but it would have avoided my confusion -- I'd give that a try and report back.

- I think 'Before' and 'After' would be more clear than the current labels. Alternatively, there could be no labels but an arrow image that visually indicates the transformation. This is not a requirement for the patch to be committed -- it's polish.

NaheemSays’s picture

For the strings, how about:

1. "Rotate: randomly up to 30°"
2. "Original" and "Preview" (or "preview changes" or "preview effects")

yoroy’s picture

drewish: obviously not! :-)

- 'original image' could just be 'original'.
- Do we really need a random rotation feature? Any reason I'd want my sunset rotated 'somewhat' in 'this or that' direction? Seems like a rather useless feature to me, Hmmm, maaaaybe for that 'make it look like a bunch of polaroids thrown on the table'-style, but still
- re clickable for actual image maybe add a small (view actual image) to the style name or (click to view actual image)directly below the thumbnail.

Bojhan’s picture

@drewish not really.

Still not sure on the before, after is that more descriptive then the name of the actual image name - that one has filled in above. I dont think most people will name it like thumbmail_square - but thumbmail.

I suggested : Rotate random (1°-30°)

dmitrig01’s picture

Status: Needs review » Needs work

applied the patch, installed head, went to look at the thumbnail style, this is the first thing I get.
* Warning: Division by zero in theme_image_style_preview() (line 662 of /Applications/MAMP/htdocs/head_2/modules/image/image.admin.inc).
* Warning: array_intersect_key(): Argument #1 is not an array in theme_image_style_preview() (line 664 of /Applications/MAMP/htdocs/head_2/modules/image/image.admin.inc).
* Warning: Division by zero in theme_image_style_preview() (line 680 of /Applications/MAMP/htdocs/head_2/modules/image/image.admin.inc).
* Warning: array_intersect_key(): Argument #1 is not an array in theme_image_style_preview() (line 682 of /Applications/MAMP/htdocs/head_2/modules/image/image.admin.inc).

http://img.skitch.com/20090712-1we8c8t6ki1stcjwqreq73p72q.jpg is not optimal and neither is http://img.skitch.com/20090712-gx1554fbei98sxn11cr6a65k2q.jpg

can I resize to units other than pixels? how about percents?

That's all I can review for now, the code looks OK, but I'll review more once this actually works

NaheemSays’s picture

can I resize to units other than pixels? how about percents?

I think this option was taken out as "15% of WHAT?" would be a valid question - the original images would not always be of the same size.

NancyDru’s picture

I have to agree with Nate, showing the preset name and "original" make a lot of sense and was perfectly clear to me. However, keep in mind that I am an English-only. But I agree with the others that "Rotate 30 (random)" is quite confusing; I would think it clearer if it was something like "Rotate random (30° maximum)" (let's be realistic, how many people are really going to know at quick glance what + or - means, or even that it matters).

quicksketch’s picture

Status: Needs work » Needs review

dmitrig01, you need to place an image in "modules/image/images/sample.jpg". There isn't one included yet because I'm trying to work out licensing with the original illustrator. However it might come down to getting a different image than the one I'm using on the demo site.

kika’s picture

A quick UX review:

1. User-facing effect names should be in more "human-readable" format. Can we add a bit of metadata for effects so "image_crop" would display as "Crop image", "image_desaturate" displays as "Desaturate image" etc -- while keeping machinereadable names for internal usage.

2. image_resize and image_scale -- the difference between those is not clear, especially when using such plain terms. What about keeping internal names but being more verbose in human-readable names (brain dump):

image_resize = Resize image
image_scale = Resize image, maintain proportions

...thinking about it a little more. Why not integrate these two, just providing a checkbox "[x] Preserve aspect ratio" what is checked by default? What is the use case of not preserving aspect ratio in real-life situations? Opt out of it is an edge case, no?

(this can be pushed to followup issue of course)

3. There is no real value for the user to be able drag-and-drop "select a new effect" dropdown. Move it out, below of the table.

4. +1 for yoroys idea of actual image link. I'd use "View in actual size" link added to the image title.

5. Could we use already existing modules/simpletest/files/image-test.png for preview? Or is it too obscure for users to face?

6. Is there a technical reason why new style creation has to be done in 2 steps? For the user, it would be simpler just to get straight to full add/edit form of the style

7. I do not see a value for random rotation in core. What is the use case? Why can not it be handled via contributed module "Collage / Pile of Polaroids" with proper frames, drop shadows etc?

drewish’s picture

kika, thanks for the feedback.

1-2. I'll look at adding better naming. There's actually another scaling mode that should probably make it into core just so there's a clear upgrade path from ImageCache. You can read about it here #275497: Remove the deprecated status from the outside dimensions scaling but that might be best handled in a separate issue.

3. on the contrary if you want to put an action at the beginning of the transformation you can move it to the desired position and add it all at once.

4. I'll get the linked text in there.

5. I think we need something larger and more representative of actual images.

6. No idea. We'll have to get some of the forms ninjas to weight in. I tend to think it'll be a bit difficult since there's not any other places in core where we do that.

7. I'd like to keep the random rotation in, both to simplify the upgrade path and because I think it's a nice effect with a small (10°) rotation.

kika’s picture

> 3. on the contrary if you want to put an action at the beginning of the transformation you can move it to the desired position and add
> it all at once.

Still not convinced. You can achieve 100% equal result with 100% equal amount of mouse interaction by adding the action to the end of the list and then move it to the appropriate place.

The proper way to create such a flexible filter-chain UI is to mimic ITunes filter builder http://kika.trip.ee/sites/kika.trip.ee/files/images/osx_smart_filter_0.png -- and we should do it in several places in Drupal. -1 for a weird UI precedenece.

yched’s picture

@kika: note that CCK and webform D6 also uses the 'drag new element in place before creating it' pattern, and it is quite useful there.
I do think that's one click less, but more importantly IMO, once you get out of the 'configure new element' pages you don't have to step back and say 'oh, I'm not finished, I need to put it in the correct order.

I do think it's a cool feature, and users are not forced to use it.

mcrittenden’s picture

image_resize = Resize image
image_scale = Resize image, maintain proportions

...thinking about it a little more. Why not integrate these two, just providing a checkbox "[x] Preserve aspect ratio" what is checked by default? What is the use case of not preserving aspect ratio in real-life situations? Opt out of it is an edge case, no?

+1 for combining them and adding a checkbox. Seems like the separate scaling issue drewish just posted could be handled in much the same way, no?

drewish’s picture

kika,

The proper way to create such a flexible filter-chain UI is to mimic ITunes filter builder http://kika.trip.ee/sites/kika.trip.ee/files/images/osx_smart_filter_0.png -- and we should do it in several places in Drupal. -1 for a weird UI precedenece.

it's not clear to me that the itunes example is reorderable or that the order of the items affects the operations. in this case the order is makes a big difference compare rotating 45° then cropping to cropping then rotating.

Dries’s picture

While I think the UI/UX can be improved, I'm comfortable committing what we have now. I especially like the checkbox-idea, and the label/link suggestions for the images. I'll wait for the next re-roll though, and I'd hope to see some of the suggestions implemented. I don't think the UX issues are a show-stopper though, and happy to make additional UI improvements in follow-up issues.

drewish’s picture

dries, great to hear. i'll post a new patch after the nyc drupal meeting tonite... or during depending on how interesting it is ;)

kika’s picture

Sure, UI tweaks can be added later but current patch does not solve the sample image problem. We need a decent placeholder image what:

1. is placed to a logical place (oh now, another thing /misc !)
2. serves us in future for any real-image-placeholder scenario
3. is not too big in size
4. has GPL licence

Something from http://buytaert.net/photos ? ;)

quicksketch’s picture

Thanks kika, you're quite right. I was trying really, really hard to have a suitable picture ready so we didn't need to discuss it extensively, but I haven't been able to contact the artist that did the illustration that I hoped to use.

At least item 1 is already solved, it goes in the very logical location of "modules/image/images/sample.jpg" (or preferably png really). Being "image" module, I expect that we'll need more images in the future for graphics in the UI (such as the arrows for cropping per #98).

I'd also like to say that the sample image is replaceable in two different ways: via a variable_get('image_style_preview_image') and through the theme layer theme_image_style_preview(). So it's possible that we can add a "upload your own sample image" option at a later point, or a contributed module could easily provide the functionality. However the default is clearly important, since most sites won't bother changing it.

Dries’s picture

Happy to license my pictures as GPL if that is what we want -- any other GPL picture would be good too.

eojthebrave’s picture

New patch with the following changes.

Rewrite theme_image_resize_summary so that it can deal with situations where there is no user supplied height or width. This also applies to the scale summary and scale/crop summary.

Wrap image effect summaries in <div class="description">

Fixes capitalization issues from #128

Add validation callback to image_scale_form that ensures the user does not leave both the height and width options empty.

Include anchor information in theme_image_crop_summary

Adjust summary of rotation effect so that it now reads either. "15", or "random between -15 and 15" as appropriate.

Change label of original image in style preview to read "original" instead of "original image".

Add "view actual size" links to preview images.

Fix issue with machine-readable names being used where human readable names should be. This issue cropped up as a result of "Renames $effect['effect'] to $effect['name']" from #122. The result of this renaming is that we were using the $effect['name'] as both the unique identifier for an effect and as the human readable name of the effect. I changed the definition of hook_image_effects_info so that the 'name' parameter is now 'label', and then used label throughout wherever we should be displaying the human readable name.

Also cleaned up a couple of function documentation issues.

All of these changes are based off of what is currently in drewish's git repo with current CVS pulled in before making the patch.

Bojhan’s picture

Why where the summaries added? I dont think it makes a lot of sense, to be honest.

quicksketch’s picture

Thanks for the reroll eojthebrave. I didn't mean to break the human-readable names when I changed "effect" to "name" in the API patch.

I agree with Bojhan. The summaries were meant to be as concise (yet informative) as possible. There's no need to say "85 width - 85 height", it takes longer to scan than just "85x85". Separating them from the action doesn't help much either, previously they read much more like sentences "Scale and crop 85x85", that's all I need to know. Including *all* the information like "center - center" probably isn't necessary either.

Maybe we can drop the "view actual size" and just make both the preview image and the word above it links.

eojthebrave’s picture

Summaries are back to being displayed on the same line. I had previously gone to the "longer" summaries while trying to figure out how to deal with situations where 100x100 will not work. For example if you've have a scale action with just a height, you end up with "x100" which doesn't make any sense. So now if there is both a width and a height you get 100x100, and if there is just a height you get "height 100". See attached screenshot. I do like it better with everything on one line.

I left the "view actual size" links in place. I'm not sure that just making the label a link deals with the issue people were having figuring out that you could click on the preview images to view the full size one. Does making the label into a link make it clear enough what is going on?

quicksketch’s picture

FileSize
49.95 KB
176.33 KB

Thanks, putting everything on one line definitely helps. I think that's a good solution for "width 50" when a height isn't entered too.

I'm now working on a sample image that we can include in Drupal core. I personally would really like this to be an illustration rather than a photo, because it's more apparent that this is a sample image and not a real uploaded image. In addition it's easier to control composition and get a variety of colors in an illustration. Attached is the image I have so far. It's based off of the illustration I wanted to use (but completely independent) and one of my Flickr photos of a vineyard at Skywalker Ranch. It's done in Inkscape and obviously it's going to be GPL so we can include it.

quicksketch’s picture

FileSize
184.45 KB

Hm, sorry somehow I attached a version that had ridiculously bright colors. Attached is the version I meant to include.

Bojhan’s picture

FileSize
192.4 KB

Never thought I would review sample images :P, but I think you should move the road to the left side of the bushes. Because it acts as a recognition point, more then the vineyard - and it now goes behind the bushes.

aaron’s picture

FileSize
210.87 KB

Sorry, this is just too good to resist...

Dries’s picture

I think Bojhan has a point. The vineyard is only so useful as a reference point. Also, I think the bikeshed should be Druplicon blue not regular plain blue. ;-)

frank0987’s picture

Submitted the patch for retesting

eojthebrave’s picture

Status: Needs review » Needs work
FileSize
51.36 KB

Fix two instances where '!name' was being used in the title callback for effect related paths. Should be !label

All UI tests should work now with one exception which I could use some direction on.

The current test code uses drupalPost() to add one of each of the different effects to a style for testing. This works fine for all of them except the desaturate effect. The reason that this test fails currently is beacuse the drupalPost() function expects there to be a form returned, which it will then fill out, and submit. The desaturate effect does not have any configuration options and we are therefore redirecting right back to the style edit page after adding a desaturate effect. Hence the failure.

So the question is how to write a test that POSTs to a path and then evaluates the results which in this case is a return to the style edit page with a message that says something like "Desaturate effect added."

Also, I'm not entirely sure how the previous patch could have possibly passed all the tests that are included in that patch since a lot of them are just plain wrong. Weird.

Marking as needs work until we get the desaturate test figured out.

eojthebrave’s picture

New issue for further discussion of merging scale and resize effects. #524562: Combine image scale and resize effects and add preserve aspect ratio option to scale

catch’s picture

Doesn't fix the test issue, and haven't tried out this patch yet, but what about a confirm form on desaturate? Also, subscribing.

drewish’s picture

catch, the desaturate form was removed specifically because it did nothing and just added an unnecessary step. i don't think we should be required to have a settings form for effects.

quicksketch’s picture

FileSize
174.11 KB

Doesn't fix the test issue, and haven't tried out this patch yet, but what about a confirm form on desaturate? Also, subscribing.

We actually explicitly remove the confirm form for Desaturate and any other action that doesn't have any options to configure. Previously the confirmation was actually confusing as to why it was happening, since this isn't really a "destructive" operation. It's a piece of cake to remove an action, so there doesn't seem to be any benefit to making the user click two buttons instead of one.

eojthebrave, you're too quick for me, I was making the same changes. ;-)

Anyway, here's an updated image that moves the road (it does help), further lowers some of the color levels, and increases the size of the balloons since they'll usually be viewed at 160px. eojthebrave, we should switch the required image from being "sample.jpg" to "sample.png". Using png has the advantage of displaying transparency when using the "rotate" effect with no background.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
50.94 KB

This version fixes all the tests, renames our sample from a .jpg to a .png file, and removes the TODO regarding #373201: Enhance drupal_render() themeing. Return renderable array on tracker page.. For the time being I think it makes sense to not make up a second parameter for theme_image_style_list() just to make the rendering system happy, since that will likely change before D7 is released.

Note that the testing bot probably won't pass because we need to copy an image into modules/image/images/sample.png.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

FileSize
48.1 KB

If anyone would like to improve the sample image, here's the Inkscape SVG file. I think this patch is ready to go, but we're now at a stand-still until a sample image is added so that our tests can start passing.

webchick’s picture

Status: Needs work » Needs review

I committed the image. We can always change it after if we want to move things around a bit more, but for now this should get the tests passing. Let's see...

webchick’s picture

Also, I would just like to point out how complete awesome it is that you worked around the problem of not having a GPL image of balloons by DRAWING ONE IN INSKCAPE! quicksketch++

Dries’s picture

Any reason the image is under images? Given that there is only one file, we should move it up one directory and delete the new images-directory.

webchick’s picture

The idea was that when this is eventually committed "for real", other images might be required, such as arrow graphics for the border around the picture. We can move it to the root module directory for now though if you want.

eojthebrave’s picture

quicksketch’s picture

FileSize
50.75 KB

Webchick moved the sample image to the image root directory (the "images" directory may have been preemptive on my part), but I don't think it'll make much difference either way. I have some concerns about #525716 anyway about how much sense arrows make to begin with. If we need several more images we can recreate the directory without much trouble, there's not much benefit to keeping the version history on binary files anyway.

Attached is a new patch to accommodate for the new location.

webchick’s picture

Overall this patch looks great. I love that it wraps so nicely around the imagecache API patch that went in. Lots of stuff like this in form submit handlers:

+    image_effect_save($effect);
+    drupal_set_message(t('The effect was successfully applied.'));

YAY for crud functions! :D

+      '#markup' => isset($effect['form callback']) ? l(t('configure'), 'admin/settings/image-styles/edit/'. $style['name'] .'/effects/'. $effect['ieid'] ) : '',

Concatenation is off in a few places, like here.

While we're at it, we could use a few inline coments in image_style_form just to break up the code... that's a LONG function!

Also, this is pedantic, but could we make sure we properly call these things _image_ styles and _image_ effects in PHPDoc?

+function  image_style_add_form_submit($form, &$form_state) {

(minor) there are two spaces here

+  if ($element['#value'] != '' && !ctype_digit($value)) {

We had committed ctype_digit to core before, but had removed it after finding out it wasn't quite as universal as we thought... see #72865: ctype_digit() not available on drupal.org. I assume since Dries said +1 to this patch, and didn't to that one, that this situation has changed since 2006.

[23:43]  <webchick> +    if (empty($element['#allow_negative'])) {
[23:43]  <webchick> +      form_error($element, t('!name must be an integer.', array('!name' => $element['#title'])));
[23:43]  <webchick> +    }
[23:43]  <webchick> +    else {
[23:43]  <webchick> +      form_error($element, t('!name must be a positive integer.', array('!name' => $element['#title'])));
[23:43]  <webchick> +    }

Those look like they should be check_plained(). However, quicksketch pointed out that they're not in http://api.lullabot.com/_form_validate/7.

+ * form for configuring the resize options. Therefor it does not not need to 

"Therefore" spelled incorrectly in several places.

[00:03]  <webchick> +  if ($data['width'] && $data['height']) {
[00:03]  <webchick> +    return $data['width'] . 'x' . $data['height'];
[00:03]  <webchick> +  }

This looks like it also needs to be escaped..?

+ function getInfo() { needs to not have t() around the elements.

+ function testStyle() { needs PHPDoc

/* Add style form. */ and so on in the testStyle() function should not be /* comments but // comments (I know it's a meta comment but it makes commenting out parts of the function whiel debugging really annoying when there are block comments like that inside)

Trying the patch from a user perspective now.

webchick’s picture

Status: Needs review » Needs work

User-facing review:

When deleting an effect, I get the message:

"Deleting this effect will regenerate all images for the thumbnail style. "

Great! What.. the heck does that mean? :) What are the consequences for my site if I proceed?

On the help page:

"Image styles may be updated after they have been created and all images using that style will be automatically updated."

Maybe "and all images using that style will automatically reflect the changes"?

"Besides the ones included with Image, " -> "In addition to the effects.."

Once inside an image style, there are no breadcrumbs, nor is the "Image styles" menu link active, so I end up getting lost. :( However, #483614: Better breadcrumbs for callbacks, dynamic paths, tabs will fix that problem, both here and in other places in the admin panel.

Other than that, I've run out of stuff to complain about. One more re-roll and I think this is good to go.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
50.75 KB

Okay, here's the reroll that takes into account all of webchick's requests. It's a little bit heavier because it changes several existing references to "effects" and "styles" to "image effects" and "image styles". But the changes are easy to review.

A few particular notes: I removed ctype_digit() just in case Dries didn't mean to make that exception. And I removed the warning entirely from the delete effect confirmation, since we don't (and I don't think we should) warn the user when editing, reordering, or adding effects, all of which also flush the image cache.

quicksketch’s picture

FileSize
56.65 KB

Oh crud, that's the same as last patch. Here's the new one.

quicksketch’s picture

FileSize
56.52 KB

A few more comments from webchick:

- Removes the "scary" sqrt() call from theme_image_anchor() ;-)
- Removes ctype_xdigit().
- A few minor code style fixes.

webchick’s picture

Status: Needs review » Fixed

It is with great pleasure that I pronounce this issue FIXED.

Committed to HEAD with a CHANGELOG.txt entry. YEAH!!!

quicksketch’s picture

W0000t! Another Image patch down. Any interested attendees to the Kill-Image-ImageAPI-ImageCache-FileField-ImageField-Upload-All-In-One-Fell-Swoop Party, please see the following issues:

#391330: File Field for Core
#483614: Better breadcrumbs for callbacks, dynamic paths, tabs
#370537: Allow suppress of form errors (hack to make "more" buttons work properly)

Yay!

yoroy’s picture

Congrats quicksketch, all and every future D7 newbie!

Status: Fixed » Closed (fixed)
Issue tags: -user pictures, -Killer End-User Features, -images, -ImageCacheInCore

Automatically closed -- issue fixed for 2 weeks with no activity.

Status: Closed (fixed) » Needs review
Issue tags: +user pictures, +Killer End-User Features, +images, +ImageCacheInCore

plastilin requested that failed test be re-tested.

mcrittenden’s picture

Status: Needs review » Closed (fixed)
webservant316’s picture

I've been learning more about the power of imagecache to embed photos on the node, automatically create tons of user configured variations of the image, and offer lots of ways to present the photo on the page or whereever. This module is wonderful.

Any thoughts about extending it to include other file types such as PDFs? Man it would be create to upload a PDF in the same way as an image and be able to create thumbs and images of the 1st page and then link the image to the PDF file. Maybe there are other modules that do that, but I researched it enought to know which is best for me. However, if imagecache did it as well as even more media types such as video, audio (in the case of audio perhaps upload two files, one for the image, and then the audio file), docs, or whatever THEN I can build a gallery of media using views, etc. because all kinds of media are linked to the node on the same field.

Man that would be wonderful and to have that feature tried and tested because it is part of core! I this module a hopeful for that kind of functionaility or should I be looking at another module?

dman’s picture

@webmaster_prw This thread was closed half a year ago.
That's an entirely different feature request, off-topic for this big issue.
If you researched, you should have come across the history of Create PDF thumbnails with imagecache and ImageMagick and eventually to the dedicated solution PDF to ImageField which is technically a better approach than trying to shoehorn this extra-special idea into core or even to imagecache.

webservant316’s picture

ok, thanks for that.

scott.whittaker’s picture

Is there any progress on getting the deprecated scale to outer dimensions working in D7? I see that crop has the ability to choose the alignment to crop to, but no way to scale the image that won't result in the image being either cropped dead center or scaled smaller than the crop.

Basically all I want to do is have an image scaled and cropped to a square and let the user choose how the image is cropped. I could do that in D5 and D6 by first scaling to the outer dimensions (so that a rectangular image is larger than the final size on one axis) and then a crop action to crop the image top, bottom, left, right, center or middle. This means I had 6 profiles for each crop option, but it was a simple way to give editorial control over how images should be processed for output.

fineartist99’s picture

Okay, stupid question but I've never had to work with patches before. How exactly would I get image cache implemented on my drupal 7 site?

Step by step instructions please.

apaderno’s picture