Duplicate preview with same Media image twice on one page

Hi michielkenis,

Can you tell me the version number of your Media module?

Thanks!

As you already figured out, Manual Crop expects the fid to be unique in a form. This is needed to:

• Find the associated thumbnail (the "bug" you're experiencing);
• Determine the required crop styles;
• Properly save/update existing crops and remove the ones that were cleared.

So I would guess you'll encounter some extra issues with this set-up... I don't think this can be fixed easily, isn't it possible to split out the forms?

Hi,

With some luck I will add a patch in a day or two, had the same issue with a site today.

Same issue here with a site that has a node type that has two image fields to be able to select two different images. However the content editor selected the same image for both fields resulting in the reported issue. So , this situation can actually exist on production sites and is not a "non-issue".

Patch is in progress.

Here is the first patch, but it requires more testing.

Tested with:
* Media multiple fields, multiple images

Needs to be tested with:
* no Media fields (just core image field)
* Media + Multiple fields and multiple image styles
* ...?

Let me know what you think!

Hmm, spotted debug code on line 610 of the JS file :(

Without debug code..

Hi,
I'm uploading a slightly modified version of the patch in #13 because I noticed that the JS event handler in manual-style-select that calls ManualCrop.showCropTool was missing the element ID. This change fixes up that event handler.

Nice update Geoffreyr!

Tested the patches some more and here are some points to fix first, before we can call RTBC:
- ManualCrop Thumb list shows the label times the fid.
- Doesn't work with Field Collection

Extra test:
- Field Group (horizontal tabs and vertical tabs)

Argh .. status

I noticed that when uploading a new file through the Media picker, Manual Crop would fail to find an ID for the field and throw an error. The following patch fixes this; I'm yet to determine if it fixes your issues with Field Collection though.

I can confirm the patch in #17 works for horizontal tabs

Patch #17 works in our case.

Any update to this?
Applying the patch from #17 fixed the repeating images in the edit UI... but now, as I see, it doens't work with a field collection...

Also, maybe related.
The manual crops aren't sticking anymore.
The feedback you used to get with (with the parenthesis, saying "cropped") now doesn't show up.

The interface sends you to the crop screen, you can select what you want to crop, hit save... but it doesn't change the status in the drop down.

You can select the crops, and then hit save, but they don't show up on the front end.

Then when you EDIT the node, and look at the crops, they haven't saved.

Thank you.

Found that the previews would disappear, styles wouldn't be marked as "(cropped)", and the crops weren't sticking with the above patches, this should fix that

The patch in #21 introduces a new problem for me. I'm getting this error multiple times:

Notice: Array to string conversion in manualcrop_croptool_validate() (regel 437 van C:\wamp\www\nhl.trunk\sites\all\modules\contrib\manualcrop\manualcrop.module).

Also I still have this error:

[image] must have a cropping selection for the [image_style] image style.

I also had the "Notice: Array to string conversion in manualcrop_croptool_validate()" error from patch #21.

I'm attaching a new patch that fixes this error. It also adds support for the case where one field using the image uses only one style (thus displaying a "crop" button) and the other field using the image uses multiple styles (thus displays a select element).

Rerolled against latest DEV.
One thing I noticed about patch #23 was that changing the #parents array in manualcrop_croptool_process led to a really nasty form processing bug. That array gets fed to drupal_array_get_nested_value, which expects a single-level array of either integers or strings. Sending it an array of arrays made that function throw all kinds of horrible errors, especially when used in conjunction with the Media Browser.

In case anyone is interested, here's an alternative to #24 that I put together that uses data-* attributes to indicate elements that Manual Crop's JS should use. These attributes pass in a JSON-encoded string containing parameters that each element should use. Not only will this keep in line with principles of unobtrusive Javascript, but it should also lower the likelihood of inadvertent typos in the onchange and onmousedown handlers. (I've hit this exact problem several times while working on this issue, and it's been a persistent enough bugbear for me to want to have to make this change.)

One more change to my patch in #25. Added a check to see if the $id was unset - in the case of using this together with Media module, this happened whenever I uploaded a new image to a Manual Crop-enabled field. This change ensures that there is an ID available at all times, regardless of circumstances.

I got the following error from #26 when saving a node with cropped images:

Notice: Undefined index: #id in _manualcrop_add_croptool() (line 419 of sites/all/modules/patched/manualcrop/manualcrop.helpers.inc).

It looks like the intent of the last change to patch #26 was to address this situation, but the empty() function wasn't used in a way that avoids the notice. I'm attaching a #27 that fixes this one problem.

#26 avoids a problem with my #23 that prevented the user from being able to save changes after clicking the Media Browser's edit button on the image. I'm also pleased to see that #26 still handles the case where one field using the image uses only one style (thus displaying a "crop" button) and the other field using the image uses multiple styles (thus displays a select element).

Linking to a possibly related issue.

This patch does not work on the dev version when using the same image ($fid) within a node. I fixed the patch to work with the latest dev version. Also this makes it work with multiple fields with the same style.

Since Tecca stepped back a little bit and others chimed in too, we maybe should encourage others to chime in by removing "assigned to" from this issue atm?

1. +++ sites/all/modules/contrib/manualcrop/manualcrop.helpers.inc	(revision )
   @@ -236,14 +236,22 @@
   +    if(!isset($form_state['manualcrop_data']['images'][$file->fid]))
   +    {
   ...
   +    else
   +    {
   
   @@ -408,6 +416,11 @@
   +  } else {
   
   +++ sites/all/modules/contrib/manualcrop/manualcrop.module	(revision )
   @@ -407,66 +407,76 @@
   +      foreach($image['element_parents'] as $parent)
   +      {
   ...
   +          if(!isset($selections[$fid]))
   +          {
   ...
   +          else
   +          {
   

   Coding standards

2. +++ sites/all/modules/contrib/manualcrop/manualcrop.helpers.inc	(revision )
   @@ -236,14 +236,22 @@
   -      'element_parents' => $container['#parents'],
   +        'element_parents' => array($container['#parents']),
   

   can someone explain this change?

3. +++ sites/all/modules/contrib/manualcrop/manualcrop.js	(revision )
   @@ -15,7 +15,7 @@
   -    $('.manualcrop-identifier-' + identifier, context).once('manualcrop-init', function() {
   +    $('.manualcrop-identifier-' + identifier, context).once('manualcrop-init', function () {
   
   @@ -25,13 +25,13 @@
   -  $('.manualcrop-cropdata', context).once('manualcrop-init', function() {
   +  $('.manualcrop-cropdata', context).once('manualcrop-init', function () {
   ...
   -  $('.manualcrop-button', context).once('manualcrop-init', function() {
   +  $('.manualcrop-button', context).once('manualcrop-init', function () {
   -    $(this).mousedown(function() {
   +    $(this).mousedown(function () {
   
   @@ -40,7 +40,7 @@
   -    $('.ajax-new-content', context).once('manualcrop-init', function() {
   +    $('.ajax-new-content', context).once('manualcrop-init', function () {
   
   @@ -61,13 +61,13 @@
   -  $('.modal-content', context).once('manualcrop-init', function() {
   +  $('.modal-content', context).once('manualcrop-init', function () {
   -    $(this).ready(function() {
   +    $(this).ready(function () {
   ...
   -
   +  ¶
   

   lets remove those changes, it just makes the patch harder to review

4. +++ sites/all/modules/contrib/manualcrop/manualcrop.js	(revision )
   @@ -664,27 +663,44 @@
   +          if ($(this).val().lastIndexOf(' ' + Drupal.t('(cropped)')) == -1
   +              || $(this).val().lastIndexOf(' ' + Drupal.t('(cropped)')) != ($(this).val().length - Drupal.t('(cropped)').length - 1)) {
   +            $(this).val($(this).val() + ' ' + Drupal.t('(cropped)'));
   ...
   +          if ($(this).text().lastIndexOf(' ' + Drupal.t('(cropped)')) == -1
   +              || $(this).text().lastIndexOf(' ' + Drupal.t('(cropped)')) != ($(this).text().length - Drupal.t('(cropped)').length - 1)) {
   +            $(this).text($(this).text() + ' ' + Drupal.t('(cropped)'));
   ...
   +          if ($(this).val().lastIndexOf(' ' + Drupal.t('(cropped)')) != -1
   +              && $(this).val().lastIndexOf(' ' + Drupal.t('(cropped)')) == ($(this).val().length - Drupal.t('(cropped)').length - 1)) {
   +            $(this).val($(this).val().substr(0, ($(this).val().length - Drupal.t('(cropped)').length - 1)));
   ...
   +          if ($(this).text().lastIndexOf(' ' + Drupal.t('(cropped)')) != -1
   +              && $(this).text().lastIndexOf(' ' + Drupal.t('(cropped)')) == ($(this).text().length - Drupal.t('(cropped)').length - 1)) {
   +            $(this).text($(this).text().substr(0, ($(this).text().length - Drupal.t('(cropped)').length - 1)));
   

   Lets store Drupal.t('cropped') result somewhere, and $(this).text(), $(this).val()
   to avoid tens of unneeded function calls?
   Same for $(this)

Hi,
for me #29 is solving this multiple-image-preview problem with the current dev version,

Thanks

With a new release of manualcrop a month ago, would love to see a working patch that apply cleanly to latest dev.

Just confirming #29 works when patching 1.6.

Patch assumes module is in sites/all/modules/contrib which is not always the case. Which makes it non-standard and hard to apply.

#29 seems to work, only it's not a git-standard format, instead it seems to be a product of a phpStorm or something like that - it bears the IDEA git format, which is not always compliant. Drush make for example breaks upon the build.
This #30 patch is simply the same as #29 but in a git-patch format.

Works against version 1.6

Hi,
#30 patch works for me patching 1.6
Thx

The patch in #37 partly works for me with 1.6.

With the patch the preview is not shown at all.

This patch file is broken, use #41
I've changed the id (in patch #39) to a class matching the field name instead. Assumes there's a field_name property in the element, this breaks use cases where manualcrop is used outside the Drupal field elements - which is not ideal.

The Media module (this might only be some version of Media) added an extra --widget to the id, so the toolOpener selector didn't match when multiple crops where enabled.

This also fixes the preview issue mentioned in #39.

My previous patch wasn't created properly, this should fix that.

Patch in #41 seems ok but doesn't resolve the issue in #28 (which is #2711195: If two fields use the same manual crop style and the same image is reused for both, crops are lost on save). Here is a patch that combines the two together which seems to work for me.

#41 still won't allow multiple media fields with the same image file to be cropped, you can crop images, but the cropped style won't be saved, also the preview image is repeated and appended in the amount of the images that are identically used on the node as shown in #1

#42 doesn't apply to dev-branch

@ next patch uploader, please add interdiff.txt to show the changed lines.

I painstakingly applied #42 by hand against current dev. This patch is just a re-roll of #42. I've added an interdiff from #41.

In my testing it certainly seems to fix the issue of multiple thumbnails appearing. It also saved the second instance of a media field with an image crop but not the crop from the first. I haven't thoroughly tested though.

I had missed a couple of important lines from the previous patch. This one now works nicely for me and solves the following two issues:

• Duplicate/multiple previews after applying crop
• Only once file's crop settings saved if multiple fields with the same file used

Hi,

I'm still have issue using Manual Crop (7.x-1.7) after using last patch (manualcrop-duplicatepreview-2503175-45.patch)
I'm using paragraphs.

Fred

I did also experience this issue.

While patch from #45 seems to be a very nice solution in most cases, I did still experience bugs in case field collection items with images where used and especially if multitple field collection items are allowed. Of course only in case the same image (same fid) is used several times as this whole issue is about this case only.

As @freed_paris experienced also issues with the patch from #45 together with paragraphs module, I suppose there would be more work to do to get #45 work with modules like field collection and paragraphs that create "field in field" scenarios. I looked in patch from #45, but find it very hard to get my head around all the changes that were done here and moreover found it difficult to rewrite this patch for the field collection use case.

So I did some work on a new patch that seem to solve the problem in my use case, where regular image fields (with media module) and field collection fields are used. I took 2711195 as the base and added some other changes.

My patch is a bit hacky, but it seems to solve the issue for me. The downside on this patch is that you lose the changing preview in the case when the same image is used several times. Also with this patch there will be no button used for the cropping even if only one style is selected (you will always get the select field). But for me this is fare more acceptable than the completely broken functionality within field collections. Maybe it will also work with paragraphs, but I have not tested this.

The patch from #45 seems to be a more clean solution than my patch. But hey, I needed something to fix this field collection mess ...

Patch #47 works for me at 1.6 version.

Patch #45 works at version 7.x-1.7 when trying to use the same image in two different fields.

If you use the same image in the same field you'll still get the image double up issue, however (screenshot in #46 is what I'm referencing). I'm not sure how much of an issue this is, as most people probably aren't uploading the same image to the same field twice, but I thought it worth noting.

Before I applied the patch, on pages with many images & an instance or multiple instances of this issue it made it impossible to even update the node.

Per #48, I attempted rolling back to 1.6 and applying #47 to no avail, as it causes issues with the UI that render it pretty much unusable.

Site is using paragraphs but this issue was affecting non-paragraphs image fields as well.

It should not be necessary to go back to 1.6 to apply the patch #47, I am using it with 1.7 too. It helped me with field collection and normal images in one node type. But if you have another configuration (i. e. paragraphs) it might have different implications, even for non-paragraph image fields when paragraph image fields are used together in one node type. The whole thing is quite complicated and if you hit one issue, another might pop up behind the next corner, as we can see in the issue history.

If #45 works out fine for you, than you should probably stick with this. I made #47 for a case where #45 did not suit, but tested it only in my specific configuration and therefore it might not suit other configurations.

I tried out the patch from #47 and it works great with paragraphs.

I'm updating the patch file with some small changes on JS file, otherwise, it will keep adding the label text on each cropping selection update.

#51 caused my browser to hang. #45 seems to do the job.