Image Rotation
trogie - September 6, 2007 - 12:43
| Project: | ImageField |
| Version: | 6.x-3.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
Hello,
I'm currently building a gallery system for one of my portal sites and I'm using CCK 5.x-1.6-1 and imagefield 5.x-1.1.
With the current image viewers and auto-rotate features (virtual rotation in viewing, or real rotation in files,...) in the software, users don't really know anymore what their viewers do with the orientation of their pictures until they uploaded the files. That's why I want my users to be able to rotate their pictures on the site.
As Drupals image.inc already has the function image_rotate, I could simply add a radioboxes-form to every picture and get the imagecache flushed.
Patch included
| Attachment | Size |
|---|---|
| imagefield_6.patch | 1.13 KB |

#1
I've been trying this patch against 2.x-dev version but I seem to have problems with the cache flush of imagecache...
#2
I think this will be a great feature for 2.1... I may want to implement it slightly differently and use a work around for GD2 without imagerotate support. I'm not sure that image.inc tests for this. But you can create a new image and copy it over pixel for pixel if imagerotate isn't compiled into gd.
#3
just FYI.. I'm still totally in love with this feature... I would like to see it use ImageAPI instead... Cuz I don't have ImageRotate support in my GD.
#4
The attached patch uses imageapi with a fallback to image.inc. I wasn't sure if it needed to do some imageapi error handling, so I didn't add any.
There were some problems with both the GD2 and ImageMagick toolkits when rotating images. I'll file issues and reference them here. After applying those patches, rotation should work fine with both toolkits.
#5
I did notice some problems with imagecache after rotating the images. I usually had to reload the page a couple times before the correctly rotated image was displayed for each preset. I'm not sure if the call to imagecache is wrong or there's a problem with imagecache. I'm using imagecache 5.x-2.0.
#6
The rotation only happens when you're updating a node, not on the initial insert.
#7
Updated patch so it works on insert, too.
#8
Works excellent for me! THX!
#9
Awesome! This is just what I was looking for. This is a critical feature.
Thank you much.
#10
For usability, I think a better Labeling, should be:
90 clockwise
90 counter-clockwise
180 degrees
I don't know how to do patching, but the changes are:
Change lines starting 807 at imagefield.module, from:
$form[$fieldname][$delta]['rotate'] = array('#type' => 'radios',
'#title' => t('Rotate clockwise'),
'#options' => array(
0 => t('0°'),
90 => t('90°'),
180 => t('180°'),
270 => t('270°'),
),
'#default_value' => 0,
);
To:
$form[$fieldname][$delta]['rotate'] = array('#type' => 'radios',
'#title' => t('Rotate'),
'#options' => array(
0 => t('0°'),
90 => t('90° clockwise'),
270 => t('90° counter-clockwise'),
180 => t('180°'),
),
'#default_value' => 0,
);
Also, why the float: left in the radio buttons. Doesn't look good good, IMHO.
And one more: can the image be displayed in the rotated mode while editing the image? Because now, when editing the picture and changing the rotation will rotate the already rotated image. So either:
1) display the currently rotated image in the editing, or
2) display the original rotation while editing, but also default select the current rotation (and if changed, apply it to the original)
Thank you!
#11
Update:
I noticed that the image is actually rotated in the edit, but because of the imagecache problem Junyor described, it doesn't show up until refreshing one or two times.
The imagecache flush is now located at the _imagefield_rotate -function. Is this a good place for it or should there be more places where to flush the cache?
#12
Whats the status on this patch. Any chance it will put into the 6.x version? I am not really a coder, but I can help test it out.
#13
IMHO alternation of the uploaded image in imagefield isn't optimal. Rotation will always lead to loss of data and it is always nice to keep the original file in one piece. If I understand things correctly this patch rotates the original image before storing. Depending on the rotation toolkit you also loose EXIF information. Rotation should be presentation layer work, i.e. imagecache.
I'd rather see f.ex. an imagecache module that serves as an 'editor' for images/files, with interface in the imagefield widget.
I've just written a module with an imagecache action that rotates images according to the EXIF orientation field, and this has led me to thinking in more general terms. Would it not be nice to provide the imagecache actions in some way to imagefields individual, i.e. in the node edit form?
Ok, I'm beginning to mumble, but my point is that to keep the imagefield as general as possible you should leave the image rotation apart from the original image.
#14
@kaare: i agree 100% here regarding the pristine condition of imagefields but we might have to consider a few concessions when the image is serveed on on the node edit form, and other otherwise "non presentational" conditions. Again implementing imagecache support for these displays might be a good idea but it might add too much clutter for too little gain.
#15
I don't think this will make it into the D5 version (sorry everyone that worked hard on this) since I'm putting it into "maintenance only" and only D6 is getting new features. Due to the significant differences between D5 CCK and D6 CCK, maintaining both versions is like maintaining two separate modules.
A few more things I'd like to see in this:
- The rotation should be two buttons (preferably an image with little rotate icons) for clockwise and counter clockwise, rather than radio buttons. Though we can still use the same logic for generating the final image since we don't want the change actually saved until node save. With both the current approach and this new approach though, we have the problem that preview does not reflect the rotation settings.
- The ability to rotate needs to be configurable when setting up the widget.
- Needs to be ported to Drupal 6. :)
#16
#17
subscribing
#18
Ok, here's a start on a D6 port of this patch. This should 'work' but some of the implementation is wrong and it doesn't address all the requirements from 15.
1. Using hook_nodapi to process the fields. Wrong, wrong, wrong, but I'm not sure of any other way to intercept the elements until http://drupal.org/node/417122 happens.
2. Admin previews, imagecached files, and prob the raw image are cached by the browser so you don't see the updated rotated image unless you refresh the browser. Not sure how to force the browser to request new versions of files. Could be solved by renaming/moving the filepath and saving the rotated image as a new file?
3. Widget not configurable per field. Should be easy to add if anyone has the time?
4. Uses a dropdown to select the rotation, sorry just thought it was neater and didn't have time to do buttons and images etc.
5 Could do with a class on the widget, not sure how to add that.
#19
The Image Cache Action module already handles rotation
#20
But only on a per imagecache basis. This feature request is to allow rotation for individual files by content editors.
In the OS exif data is usually used to rotate an image for display, so you get users confused as to why their images are wrongly rotated once they're uploaded to the server, when they look fine to them in Finder, Explorer etc.
#21
Thanks muhlender for your work on this. I think you should be able to eliminate the use of hook_nodeapi() by using hook_elements() instead, then you won't have to find the individual fields in hook_nodeapi() or hook_form_alter(). See FileField Insert for an example of how to modify the field on a per-field basis rather than using hook_nodeapi().
But as for settings (which would definitely be nice to have), that definitely requires #417122: Allow drupal_alter() on Field and Widget Settings, which looks like it's making progress.
#22
Ok, here's an updated patch. This should take care of most of the requirements, so tentatively marking as needs review.
Changes from last patch.
1. Added ability to set rotation option on a per field basis, using #417122: Allow drupal_alter() on Field and Widget Settings. Followed code from one of the other modules using this now.
2. Fields processed using imagefield_widget_value() function, where imagefield data items are already processed.
3. Added javascript rotation to the images using the jquery rotate library from google code. Worth renaming the library function in case another module uses it? or use Libraries API, but then you have another dependency?
4. Changed dropdown to left and right buttons where javascript is enabled. JS to calculate rotation is slightly clunky, but there's a tradeoff between making the js simple and the php form processing simple. Went for making the php simple.
5. To force the browser to display the rotated file, I've saved the rotated file to the temp folder, and then created a new file/object using filefield API. The file rename here seems to be a bit wonky, if you rotate a file several times you get filenames like muhleder_3_0_0_0_0_0_0_1_0_0_0_0.jpg. Need to take a look at this.
So prob not quite ready, but it should be quite a lot better than the last version in UX and functional terms so worth letting people have a look at.
#23
subscribing
#24
very interesting. subscribing :)
#25
+1 Very awesome patch!
#26
This patch fixes some bugginess I found in the javascript: It kept adding the rotate buttons over and over.
Also fixed some code/comment style issues.
I didn't understand why we needed a imagefield_widget_settings_alter() when we are defining the imagefield widget settings in the same file. So I moved this stuff up into the main widget settings functions.
#27
Hiya mfb,
been a while since I looked at this, but off the top of my head I think the widget_settings_alter was to allow site builders to choose whether or not to have the rotate widget on per field basis.
#28
Yes, the widget_settings_alter was used to provide this setting for each field. But why not create this setting in the main form definition a few lines above. the widget_settings_alter seemed superfluous.