Problem

When you have a node with 2 image fields, with the same image (fid) but different required cropping styles, after cropping MC gets confused and marks all the images CROPPED, even though only 1 style was cropped. Saving then, results in avalidation error, because only 1 was cropped, but 2 were marked as "Crop (cropped)".

Reproduce

  1. Create node type with 2 required image fields, with FileField Sources, without any cropping
  2. Create 2 croppable image styles
  3. Create a node with both image fields filled with the same image (hence FileField Sources or alike)
  4. Update the node type fields to have different required cropping styles
  5. Edit the node, add cropping etc

Solution

In theory it's very simple: only update buttons/options that MATCH the just created cropping (fid + style), not just the cropped image (fid).

CommentFileSizeAuthor
#13 manualcrop-image-reuse--2508773-13.patch11.75 KBdouggreen
#9 manualcrop-image-reuse-2508773-9.patch11.54 KBlennard westerveld
#7 manualcrop-image-reuse-2508773-7.patch11.12 KBlennard westerveld
#4 interdiff.txt847 bytesDavid_Rothstein
#4 manualcrop-image-reuse-2508773-4.patch11.77 KBDavid_Rothstein
#3 manualcrop-image-reuse-2508773-3.patch11.05 KBDavid_Rothstein
#2 manualcrop-2508773-1.patch2.11 KBrudiedirkx

Comments

rudiedirkx’s picture

rudiedirkx commented
Related issues: +#2043029: Cropping the same image in two different fields on the same node erases crop information, +#1977606: Crop selection on same image in two nodes of two different node types (with difference in selectable styles) unsets selection

Might be related to #2043029: Cropping the same image in two different fields on the same node erases crop information but that mentions #1977606: Crop selection on same image in two nodes of two different node types (with difference in selectable styles) unsets selection as a fix, and that definitely does not fix this issue.

rudiedirkx’s picture

rudiedirkx commented
Version: 7.x-1.5 » 7.x-1.x-dev
StatusFileSize
new manualcrop-2508773-1.patch2.11 KB

Well, that was even easier than expected.

Patch changes a few classes.

David_Rothstein’s picture

David_Rothstein commented
Status: Active » Needs review
StatusFileSize
new manualcrop-image-reuse-2508773-3.patch11.05 KB

The above didn't work for me when using the Media module's media library to get the same image to appear more than once on the same form. I had to make some additional changes in the attached:

  1. Make the JavaScript that finds images for a widget grab only the one associated with the widget.
  2. On form submit, handle the possibility that more than one image style can be submitted for the same file.

This is not as big of a patch as it looks like - a lot of it is indentation changes due to a new foreach() loop.

David_Rothstein’s picture

David_Rothstein commented
StatusFileSize
new manualcrop-image-reuse-2508773-4.patch11.77 KB
new interdiff.txt847 bytes

Added a small fix to prevent duplicate images from appearing in the widget sometimes.

rudiedirkx’s picture

rudiedirkx commented

I don't do or know Media, so I can't test it...

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me.

lennard westerveld’s picture

lennard westerveld
🤖
Primary language Dutch
Location Dordrecht, Netherlands
commented
StatusFileSize
new manualcrop-image-reuse-2508773-7.patch11.12 KB

Edited: oops i broke it :) ignore my patch

lennard westerveld’s picture

lennard westerveld
🤖
Primary language Dutch
Location Dordrecht, Netherlands
commented
lennard westerveld’s picture

lennard westerveld
🤖
Primary language Dutch
Location Dordrecht, Netherlands
commented
StatusFileSize
new manualcrop-image-reuse-2508773-9.patch11.54 KB

Added new patch this one is working with the latest dev version

fabianx’s picture

fabianx commented
Status: Reviewed & tested by the community » Needs work 
+++ b/manualcrop.helpers.inc
@@ -236,7 +236,7 @@ function manualcrop_croptool_process(&$form, &$form_state, &$element, $file, $se
-    $form_state['manualcrop_data']['images'][$file->fid] = array(
+    $form_state['manualcrop_data']['images'][$file->fid][] = array(

We lost a comment here compared to David's patch.

dqd’s picture

dqd
Location London | N.Y.C | Paris | Hamburg | Berlin
commented

Can someone confirm that the module has supported this ...

a node with 2 image fields, with the same image (fid) but different required cropping styles

... firmly? Otherwise we should rather tag this a feature request instead of a bug? Because this can bring up a module version discussion, depending on if this will have surprising impact on already installed modules and sites.

lennard westerveld’s picture

lennard westerveld
🤖
Primary language Dutch
Location Dordrecht, Netherlands
commented

@Diqidoq nope this isn't a feature right now, so it needs to be a new one.

See manualcrop.helpers.inc
'id' => array('manualcrop-' . $crop_type . '-' . $fid),
'id' => 'manualcrop-area-' . $fid . '-' . $style_name,

And

manualcrop.js
ManualCrop.output = $('#manualcrop-area-' + fid + '-' + styleName);

Its right now not mode for it because of the html ids.

douggreen’s picture

douggreen commented
StatusFileSize
new manualcrop-image-reuse--2508773-13.patch11.75 KB

Re-rolled and added missing comment mentioned in #10 by @Fabianx