Cropping coordinate determination

I agree. Anything that has to do with display is something that should be handled by code that is responsible for display. Crop information itself doesn't and shouldn't care about this details.

IMO JS should contain as few calculation logic as possible. All of the crop calculation happens on the PHP side already. So why should we have it in JS here?

Do we have that much calculation on the JS side? the only thing we do is rounding the values Cropper outputs:

var cropperOptions = {
    background: false,
    zoomable: false,
    viewMode: 3,
    autoCropArea: 1,
    cropend: function (e) {
      var $this = $(this);
      var $values = $this.siblings(cropperValuesSelector);
      var data = $this.cropper('getData');
      $values.find('.crop-x').val(Math.round(data.x));
      $values.find('.crop-y').val(Math.round(data.y));
      $values.find('.crop-width').val(Math.round(data.width));
      $values.find('.crop-height').val(Math.round(data.height));
      $values.find('.crop-applied').val(1);
      Drupal.imageWidgetCrop.updateCropSummaries($this);
    }
  };

Before the cropend callback is being executed, Cropper calculates the coordinates based on the downscaling of the image style.

Is this solution that bad?

Another option would be to only calculate relative (percentages) of the cropping data in JS and then do the rest of the calculations in PHP. I guess this would be more accurate since we round only once.

Assigning to me.

I removed thumb-w and thumb-h input elements and eval() from JS and precalculated aspect ratio in PHP, currently I am working on scaling determination in JS during initialisation of the cropper, I am not sure if I understood that part of the issue properly, I will use getImageData() function from the cropper to determine width and height of the preview image that is used to initialize cropper and will send data attribute with original image width and height, and with those data I will calculate the delta, that is image scaling.

I think we need at first to agree. I highly Lukas joined in the sense that we must not calculate dimenssions via JS because the whole system is based on images_style. Let me explain to correctly pass the user select area we need more data that is not available through JS interogeant but in drupal. To reflect the selected area on a ui style picture we need to know the dimmensions thumbnail image to determine the enlargement or reduction rate of the thumbnail image to affect the original image

The data manipulated by JS are primarily intended to pass on a crop area on the thumbnail precise manner.

@sanja_m : The ratio must not be it calculated automatically, this was one of the old functionality of the module and it has been proved that often the user or graphic uses ratios "as non-standard 95:35.

This is my unfinished patch, what I started to change, now I am not sure should we continue with this issue, and if we should is this the right direction?

Yes, I agree. It totally makes sense to have all calculation logic in one place. All PHP should be interested in is original image and crop information in it's context. This also makes code useful for other usages where presentation might be different. We are already trying to bring IWC to file_entity edit page. True, presentation will be very similar, but anyway. I still think it is a valid point.

Fixed all issues from comment #9 except the first one. @woprrr should I add two data attributes ratio_x and ratio_y and move the devision in JS like Miro suggested?
Also removed some unnecessary functions from PHP, because now coordinate calculations are moved to JS.

I test & review it today but seem very GOOD :) @sanja_m thanks you verry much & @miro too for your help :)

I found a bug that this patch introduces. I tested on 8.x-1.x I wasn't able to reproduce. Steps:

- Enable image_widget_crop_example module
- Navigate to node/add/crop_simple_example
- Upload image and apply crop
- Save node
- Expected result: cropped image appears on view page
- Actual result: image is not cropped
- Edit node
- Re-apply crop
- Save
- Image is cropped now

And few more other comments:

1. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -207,11 +206,16 @@ class ImageCropWidget extends ImageWidget {
                // Add compatibility to PHP 5.3.
   -            $has_ratio = $crop_type->getAspectRatio();
   -            $ratio = !empty($has_ratio) ? $has_ratio : t('NaN');
   

   D8 requires 5.5. Let's remove this comment as it makes no sense.

2. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -229,7 +233,12 @@ class ImageCropWidget extends ImageWidget {
   -              '#attributes' => ['data-ratio' => [$ratio], 'data-name' => [$crop_type_id]],
   +              '#attributes' => [
   +                  'data-ratio' => [$ratio],
   +                  'data-name' => [$crop_type_id],
   +                  'data-original-width' => [$element['#default_value']['width']],
   +                  'data-original-height' => [$element['#default_value']['height']]
   +              ],
   

   Why do we need to pass attribute values as arrays?

3. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -426,42 +431,25 @@ class ImageCropWidget extends ImageWidget {
   +    // Get the Crop selection Size (into Uploaded image).
   +    $crop_thumbnail['height'] = round($original_crop['size']['height']);
   +    $crop_thumbnail['width'] = round($original_crop['size']['width']);
   

   Rounding is not needed since we removed division with delta.

   This also makes this function just a wrapper that copies array values to another array. We may not need it any more.

4. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -426,42 +431,25 @@ class ImageCropWidget extends ImageWidget {
   +    // Get the Top-Left corner for Thumbnail.
   +    $crop_thumbnail['x'] = round($original_crop['anchor']['x']);
   +    $crop_thumbnail['y'] = round($original_crop['anchor']['y']);
   

   .

The comments above make sense... I've also tested it and can confirm that the cropping is not saved correctly. Let's debug and fix it.

But the approach is good and nice to have a proper cleanup!

Fixed all bugs from comment #15.

I ve re-create patch with last version of repo, there was a problem to apply the patch on the most recent version of the module. (Line 209)

I am under review thank you for the work.

Great job !!!
Your patch fix another problem too (the image appears more completely in the thumbnail).
There is just one aspect that is not available is the respect of the ratio, the crop area is always free now since I applied the patch.
But this is good way !

Step to reproduce :

Enable image_widget_crop_example module
- Navigate to node/add/crop_simple_example
- Upload image and apply crop in 16:9 tabs

Your crop area is free and not fix to 16:9 ratio.

@see

I was able to reproduce this problem too.

Is eval() deletion.... :( I try to find a way not to do that ...

I think cropper accept only an number value with this option. We sould calculate this 16 : 9 = 1.7777777777777777 and if we take this cropper work fine.

eval() is evil. We should find a different solution.

Yes this is evil but the solution is under our eye O-O, the ratio is formated in good format like (16/9) and we need to just added () in ratio variable to proced the division W/H = aspect ratio number.

All seems good now for me aspect ratio are applied now we can confirm if is good for you too ?

True. It is formatted correctly when module does it. But what if someone changes that somehow?

I'd feel much better if we'd actually make sure ratio is what we expect it to be. Maybe split string, make sure there are integers on both sides and do division.

+++ b/js/imageWidgetCrop.js
@@ -93,8 +95,7 @@
     options.data = data;
-    // @TODO: eval() is evil.
-    options.aspectRatio = eval(ratio);
+    options.aspectRatio = (ratio);
 
     $element.cropper(cropperOptions);

Another strange thing. Above we assign values of cropperOptions to options. Then we add some stuff to options and pass cropperOptions to the cropper initialization.

Why do we need to create new variable (options) at all?

Ok i ve seen and add some test to ensure data ratio isNumeric to procced to division.

For me all it's all Ok fonctionaly.

@slashrsm : I think we add some stuff in option when user not have aready clic on vertical tab, when user open details element js trigger we need to get an options with data empty but with aspectRatio set.

Just removed ImageWidgetCropManager::getThumbnailCalculatedProperties() since we don't need it anymore.

Looks good.

Thank a lot all :) @sanja_m Great job !!