Problem/Motivation

The ability to manually set cropping on an image is great. But in some cases the user don't really need/use it at all time. Infact the automated crop tools are suficient in few cases, and user use the manual crop with ImageWidgetCrop or Focalpoint after if the result aren't satisfactory.

Automated crop project target this functionality but more different about classic "Scale & crop". Today the core Scale & crop take an precise crop area in configuration and resize + crop it in good dimenssions by center. But that can be break the original aspect ratio or not really satisfactory because in few case it better to crop automatically your picture with dimensions 800 x 600 onto an real 16:9 (800 x 450) and just crop image without distorsions.

This module can be usefull for Crop API to integrate an Fallback effect if Manual crop are not define and an option on Manual Crop are seleted eg : "Automated Crop fallback" and if any crop is defined by user, Crop api can use automated crop service with crop type basis configuration (min/max/aspect ratio) and tell automated crop to calculate an automatic crop area based on configuration of Crop api. In that idea we can target an combo with Automated crop + google vision fallback to use within plugin an crop more intelligent and by example only crop faces or precise object.

Proposed resolution

For an usage more flexible/easy, I suggest to create an new option in Manual crop effect eg "Fallback effect" and if any crop are define by manual crop, we tell automated crop to crop the best area for user (i prefer thought an crop in other aspect ratio or only width or height not necessary w+h because that is possible for scale & crop).

In all cases Automated crop does be only consumed by Crop api to retrive an crop area and not create the crop entity instead of crop api. Crop api tell automated crop to define the best crop area and crop api received all coordinates to crop & store datas (that micro-services approach).

I need to discuss that with you for best solution to integrate that needed in Crop api / google vision / IWC (potentially) / Focal point (potentially).

Remaining tasks

  • Discuss of Automated cropping tools integration
CommentFileSizeAuthor
#45 2830768-crop-automated_crop_integration-45.patch14.3 KBwoprrr
#45 interdiff-2830768-45.txt2.96 KBwoprrr
#43 2830768-crop-automated_crop_integration-43.patch14.75 KBwoprrr
#43 interdiff-2830768-43.txt11.38 KBwoprrr
#36 2830768-crop-automated_crop_integration-36.patch11.69 KBwoprrr
#36 interdiff-2830768-36.txt9.34 KBwoprrr
#34 interdiff-2830768-34.txt736 byteswoprrr
#34 2830768-crop-automated_crop_integration-34.patch11.56 KBwoprrr
#25 2830768-crop-automated_crop_integration-24.patch11.11 KBwoprrr
#25 interdiff-2830768-24.txt1.59 KBwoprrr
#22 interdiff-2830768-22.txt4 KBwoprrr
#22 2830768-crop-automated_crop_integration-22.patch11.24 KBwoprrr
#19 interdiff-2830768-19.txt970 byteswoprrr
#19 2830768-crop-automated_crop_integration-19.patch10.71 KBwoprrr
#17 2830768-crop-automated_crop_integration-17.patch9.66 KBwoprrr
#17 interdiff-2830768-17.txt319 byteswoprrr
#15 interdiff-2830768-15.txt10.97 KBwoprrr
#15 2830768-crop-automated_crop_integration-15.patch9.27 KBwoprrr
#12 2830768-interdiff-11-12.txt1.44 KBbgilhome
#12 2830768-crop-automated_crop_integration-12.patch3.8 KBbgilhome
#11 2830768-interdiff-10-11.patch2.85 KBbgilhome
#11 2830768-crop-automated_crop_integration-11.patch2.81 KBbgilhome
#10 2830768-crop-automated_crop_integration-10.patch1.18 KBbgilhome
#5 Capture d’écran 2016-11-28 à 17.21.48.png387.7 KBwoprrr
#5 Capture d’écran 2016-11-28 à 17.21.15.png485.5 KBwoprrr
#5 Capture d’écran 2016-11-28 à 17.20.09.png377.22 KBwoprrr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

woprrr created an issue. See original summary.

woprrr’s picture

Issue tags: +D8Media, +Media Initiative
woprrr’s picture

Issue summary: View changes
slashrsm’s picture

Could you explain the algorithm behind the Automated crop a bit?

woprrr’s picture

Sure ! with pleasure !!

After explain the approach in code, I need to explain what cases are covered by Automated Crop in this prototype point.

Case 1: All sizes of crop box are defined (Width AND Height), and do not take into account the aspect ratio of image. That similar to Scale & crop but that only applie an crop with specific sizes by center of image. I ve already think about the possibility to define another points to attach crop area but i think that is another case.

Case 2: Only one size are completed (Width OR Height), the algorithm do calculate the missing value with respect, of aspect ratio. It's important to notice that, if user have define any aspect ratio then the aspect ratio are original image aspect ratio but if user have define an enforced aspect ratio that is used to calculation of missing value to respect new aspect ratio.

Case 3: Any sizes values are defined, the algorythm are based on the maximum widht of original image and calculate height with respect of aspect ratio (in this case too, if user enforce original aspect ratio that take prior).

That is the three principal cases of crop functional in this module ATM. Other usecases "combo" are available by example :

Scale & crop + Automated crop
That can be used to resize an image onto specific size and after automated crop an other specific area onto image resized.
Manual Crop + Automated crop
That is my original case needed by ImageWidgetCrop module, to use Automated crop as a fallback of Manual crop. Elseif any crop instance are defined by user Automated crop can crop these picture. (We need to améliorate this integration to permit to add an option on Manual crop effect and create an automated crop entity if any are defined by user).

All other effects are compatible with this effect because Automated crop use the $image statement and the position of effect can affect the state of image by example if we apply an effect black & white after automated crop your crop zone are in black & white.

Now explaination about algorithm behind Theses 3 cases :

Case 1 : Nothing special, except that

    // Initialize auto crop area & unsure we can't exceed original image sizes.
    $width = min(max($width, $this->cropBox['min_width']), $this->cropBox['max_width']);
    $height = min(max($height, $this->cropBox['min_height']), $this->cropBox['max_height']);

    // The width & height of auto crop area must large than min sizes.
    $this->cropBox['width'] = max($this->cropBox['min_width'], $width * $this->autoCropArea);
    $this->cropBox['height'] = max($this->cropBox['min_height'], $height * $this->autoCropArea);

the min / max imbrications permit to unsure all results can't exceed of original picture and destroy image, The min sizes exist to permit an minimum of we don't need to downgrade the size of image particularly in "totaly" automated mode (with aspect ratio only).

Case 2 : That is an case more spécific to ensure we don't destroy the image and permet to crop without distorsion of image in same aspect ratio or enforced by you.

    elseif ($this->hasSizes() && !empty($width)) {
      $height = round(($width * $ratio['1']) / $ratio['0']);
    }
    elseif ($this->hasSizes() && !empty($height)) {
      $width = round(($height * $ratio['0']) / $ratio['1']);
    }

Predict $height algorithm :
In this case we have $width of image (crop zone define by configuration or maximum possible width of image). The aspect ratio are REQUIRED to predict all sizes and calculate by original sizes of image or enforced.

In the case in we need to predict GCD (Great Common Denominator) these function permit that. (Algorythmic pure).

  /**
   * Calculate the greatest common denominator of two numbers.
   *
   * @param int $a
   *   First number to check.
   * @param int $b
   *   Second number to check.
   *
   * @return int|null
   *   Greatest common denominator of $a and $b.
   */
  private static function calculateGcd($a, $b) {
    if (extension_loaded('gmp_gcd')) {
      $gcd = gmp_intval(gmp_gcd($a, $b));
    }
    else {
      if ($b > $a) {
        $gcd = self::calculateGcd($b, $a);
      }
      else {
        while ($b > 0) {
          $t = $b;
          $b = $a % $b;
          $a = $t;
        }
        $gcd = $a;
      }
    }
    return $gcd;
  }

Logic to determine sizes with the rule of three tells :
($width * $ratio_height) / $ratio_width = $height
($height * $ratio_width) / $ratio_height = $width

That implement only "the rule of three tells" theorem for predict the exact sizes of annonymous value.

Why not ? At the begin i ve thing add that after too, to ensure the new calculated value respect the aspect ratio, but all correspond in all cases ATM i don't add this now i wait your feedback.

      if (aspectRatio) {
        if ($height * aspectRatio > $width) {
          $height = $width / aspectRatio;
        } else {
          $width = $height * aspectRatio;
        }
      }

Case 3 : The last case the more "complicated" case are infact more simple if we decide to abstract the $width value with one principe. If we assume the $width are ok, we use the maximum width possible of original image and apply "the rule of three tells" theorem for predict the correct height compatible with aspect ratio (enforced or original).

    if (!$this->hasSizes() && !$this->hasHardSizes()) {
      $width = $this->originalImageSizes['width'];
      $height = round(($width * $ratio['1']) / $ratio['0']);
    }

Actually, I ve focused all my effort to render Automated Crop as a service to be consume by other modules (Crop api etc...) and in the final focus to provide an powerfull interface to configure an "crop" plugin to use all possibilities of Google Vision API.

In possibilites targeted :

- Ability to determine face & auto crop on it.
- Ability to crop by density of color / form
- By specific mood by example

All limits are the information retrive by Google vision in the end...

I Hop that explaination are more clear for you, and all my efforts can offer the best module possible to complete Crop functionalities in Drupal.

Other point, I currently in preparation about structure of Automated Crop class and i need your feedbacks on it (it's never perfect).

woprrr’s picture

Priority: Normal » Minor

small Up about this subject to be continue in future or when Automated crop / Google vision developpement continue.

woprrr credited Berdir.

woprrr’s picture

Category: Support request » Feature request
Priority: Minor » Normal
Related issues: +#2627720: Integrate automated cropping tools, +#2750041: Force default crop to be applied

I also add @miro_dietiker & @berdir to this discuss to say if MD system can be interested by this subject. This is strongly linked with choose of which module and which method we use to provide Automated crop feature.

bgilhome’s picture

The patch attached is a start on basic integration - if no manual crop is set, the effect passes to automated_crop (if enabled) to process the image given the relevant aspect ratio. NB currently, the AutomatedCropEffect doesn't seem to alter the image using just an aspect ratio and a min width/height.

Ideally there would be a configuration option added to the CropEffect to fallback on automated_crop.

bgilhome’s picture

Here's a patch which sets the max dimensions for the automated crop, using the original image dimensions (respecting the crop ratio), so that the image is processed.

It also adds a configuration option 'Use fallback' (checkbox), to enable/disable this behaviour.

Interdiff also attached.

This patch currently applies to 1.x and/or 2.x.

bgilhome’s picture

It would be better if there were also a global option to enable automated crop fallback for all manual crop effects - I've added this to the crop.settings configuration (currently no config form UI).

Patch & interdiff from #11 attached.

woprrr’s picture

Very interesting patch :) ! Thank you !!

  1. +++ b/src/Plugin/ImageEffect/CropEffect.php
    @@ -103,6 +103,42 @@ class CropEffect extends ConfigurableImageEffectBase implements ContainerFactory
    +        !empty($this->configuration['fallback'])
    +        || \Drupal::config('crop.settings')->get('fallback')
    +      )
    +      && \Drupal::moduleHandler()->moduleExists('automated_crop')
    

    can we extract this part in a separate method to be more clear like extract method pattern.

  2. +++ b/src/Plugin/ImageEffect/CropEffect.php
    @@ -103,6 +103,42 @@ class CropEffect extends ConfigurableImageEffectBase implements ContainerFactory
    +      // Use image size as max dimensions (respecting aspect ratio).
    +      list($ratio_width, $ratio_height) = explode(':', $crop_type->getAspectRatio());
    +      $ratio = $ratio_width / $ratio_height;
    +      $max_width = $image->getWidth();
    +      $max_height = $image->getHeight();
    +      if ($max_height * $ratio > $max_width) {
    +        $max_height = round($max_width / $ratio);
    +        } else {
    +        $max_width = round($max_height * $ratio);
    +      }
    

    This part are very similar to \Drupal\automated_crop\AutomatedCropFactory::setCropBoxSizes and does not existing here IMHO can we extract that part in a Builder service (builder pattern) to avoid duplication and to be more class specific as possible ?

  3. +++ b/src/Plugin/ImageEffect/CropEffect.php
    @@ -103,6 +103,42 @@ class CropEffect extends ConfigurableImageEffectBase implements ContainerFactory
    +      $automated_crop_effect = \Drupal::service('plugin.manager.image.effect')->createInstance('automated_crop', $configuration);
    +      return $automated_crop_effect->applyEffect($image);
    

    Here we need to implement a Manager or everything to "Discover" what service we should use ATM only automated_crop exist but we can add google vision or custom plugin to apply effects.

@berdir @miro what you think about these implementation of automatic crop tool ? Specially for Berdir I think we can use as fallback automated_crop factory when any crop is defined and purpose a "crop_plugins" to "discover" what kind of services using in fallback. @slashrsm if you have any idea suggestion I'm totally open :) !

jcnventura’s picture

Status: Active » Needs work
woprrr’s picture

Status: Needs work » Needs review
FileSize
9.27 KB
10.97 KB

Here the patch continued to follow futur structure of Automated crop changes.

The main change is therefore the delegation of the `apply` method so that the CropEffect class can choose between an existing crop 'manual' made from the interface and the delegation of the crop area to a service (automated crop) for the moment it is strongly coupled with Automated crop and the fact that it uses a factory but a `provider` system would be welcome! Crop API could define this plugin and automated crop use this plugin to define how to manage the crop according to a logic also based on plugins. After reflection it is the most flexible and efficient approach. The idea of ​​the `provider` as a fallback is a solution that seems to answer all the requests and use cases, but in the conception of automated crop the idea was to decouple the IS effect of the factory in ways an effect like CropEffect can use it without calling itself another effect. And especially because Automated crop is not necessarily related Crop API and can live without. Crop API relies on the logic of automated crop to provide a fallback (provider) able to manage the existing of a crop Entity or if it is not the case calculate his own crop without creating a Crop entity which might be a possibility, but the idea of ​​creating N crop entity via automatic crop makes me think that we are going to have problems with crop API number and the possibility of having collisions.

This is an open question, but the fact that CropEffect is labeled "Manual Crop" should probably not have to do automatic crop and so maybe the idea would be to provide a new effect that extends CropEffect and this would be called Complex Crop with integration CROP And / XOR Automated cropping tools.

My current strategy is therefore to provide a stable means `MVP` to provide the crop automation and then re-work better with possible two styles.

I come back to the idea of ​​Provider! == Fallback. Why provider? because the previous idea behind automated crop is that it can provide a plugin instead of the existing PHP Factory and provide the possibility to have at least three types of `native` plugins (Automated crop factory based on configuration given (w, h, ratio, max / min sizes).
'Google Vision' (Automated Crop Plant based on Google vision). Logic based on your specifications.

Status: Needs review » Needs work

The last submitted patch, 15: 2830768-crop-automated_crop_integration-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

woprrr’s picture

Ooops missed one schema file :D

Status: Needs review » Needs work

The last submitted patch, 17: 2830768-crop-automated_crop_integration-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

woprrr’s picture

Status: Needs work » Needs review
FileSize
10.71 KB
970 bytes

We need to think about add tests for this usecase separately.

I just has adapter existing functionTest to add provider (empty) by default as configuration of IS.

woprrr’s picture

Status: Needs review » Needs work
  1. +++ b/src/Plugin/ImageEffect/CropEffect.php
    @@ -54,13 +56,24 @@ class CropEffect extends ConfigurableImageEffectBase implements ContainerFactory
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, LoggerInterface $logger, CropStorageInterface $crop_storage, ConfigEntityStorageInterface $crop_type_storage, ImageFactory $image_factory, ModuleHandlerInterface $module_handler) {
    

    We will add pluginManagerService when Automated crop are transformed from plugin system.

  2. +++ b/src/Plugin/ImageEffect/CropEffect.php
    @@ -155,29 +181,97 @@ class CropEffect extends ConfigurableImageEffectBase implements ContainerFactory
    +      $options = [
    +        'automated_crop' => $this->t('Automated crop (native)'),
    +      ];
    

    Recover AutomatedCrop plugins.

  3. +++ b/src/Plugin/ImageEffect/CropEffect.php
    @@ -155,29 +181,97 @@ class CropEffect extends ConfigurableImageEffectBase implements ContainerFactory
    +    return \Drupal::service($this->configuration['provider'] . '.factory')
    +      ->initCropBox($image, $configuration, $crop_type->getAspectRatio());
    

    Use futur plugin instead of Static call.

  4. +++ b/templates/crop-crop-summary.html.twig
    @@ -18,4 +18,8 @@
    +    <br>
    +    {% if data.provider|length %}
    +        <em>Automated Crop activated</em>
    +    {%- endif %}
    

    See for better wording. If anyone have ideas :D ?

  5. +++ b/tests/src/Functional/CropFunctionalTest.php
    +++ b/tests/src/Functional/CropFunctionalTest.php
    @@ -120,7 +120,7 @@ class CropFunctionalTest extends BrowserTestBase {
    

    Add Automated crop usecases to FunctionalTests.

woprrr’s picture

We can now use the new plugin provider system from AutomatedCrop.

@see #2984803: Transform PHP Native class from Plugin Factory

Basically we can just use same logic as AutomatedCropEffect here or why not define a sub-module 'AutomatedCropApi' for provide a plugin with ability to create automatically a CropEntity instead of calculate dynamically an automatedCrop area like 'Native' Approach.

Here we just need to add $form['provider'] like AutomatedCropEffect and enjoy :D ! We just need to add more plugins now to match with evry usecase making sense !

woprrr’s picture

Here the patch with Automated Crop plugins.

HongPong’s picture

I have been using patch #22 and it seems to not be causing any problems. (Note that in the UI to activate this, you have to edit inside the Image Style, the new automatic crop fallback is an option to enable inside the regular manual crop step of the image style).

woprrr’s picture

Hi @HongPong,

Thank for your review. Yes we have somes usecases possible with automated crop And Crop API. This patch provide a way to use Automated crop factory as fallback provider for automated crop based on Crop Types configuration.

For the moment the technical choice is to provide the fallback in a transparent way that to say that in the absence of Crop entity we check if a provider is to inform and allow to use this service specialize in order to provide a crop by default. It should be noted that this automatic crop achieves the generation of the image by the effect (IS) so it does not generate a crop entity for you. I think that it is absolutely necessary to separate this need from the others and to develop an effect apart for this kind of need because it will potentially create a lot of crop entity and thus increase the base and decrease the perfs (see the issue of berdir at subject of perfs).

The RC of automated crop is now out. We need to make more tests (functional/unit) to be stable but the module will not change before 8.1.x now :)

If you can re-test with RC of automated crop it's perfect. I will write doc about that and adding tests onto crop api (functional) to test provider usecases.

woprrr’s picture

HongPong’s picture

My goal is to simply have the images be the right proportions automatically with no interaction, as a fallback behavior. I will try patch 24 and see what I encounter + with the RC/dev version of automated crop module. Thank you!

candelas’s picture

Hello

I have just tested the patch in #25 and works :)
Thanks a lot for so much good work!

woprrr’s picture

Status: Needs review » Reviewed & tested by the community

Time to RTBCING !

The last submitted patch, 11: 2830768-crop-automated_crop_integration-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 11: 2830768-interdiff-10-11.patch, failed testing. View results

The last submitted patch, 12: 2830768-crop-automated_crop_integration-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Kingdutch’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for all the work done so far! Really excited to get this working as the automated cropping as a fallback is something we're waiting for. I do have a few questions/comments about the current patch :)

1. config/install/crop.settings.yml seems to be losing its newline in this patch (unless I'm misreading the patch). Files should end in newlines per Drupal coding standards.

2. I really dislike the fact that $this->crop can be multiple interfaces.

@var \Drupal\crop\CropInterface|AutomatedCropInterface|false

I don't think I've seen anything like it before and I think it's dangerous because you can only ever use functions that both Interfaces implement without causing bugs. Either a shared Interface that both CropInterface and AutomatedCropInterface rely on should be created and used (and extended by both interfaces) so it's clear what is relied on. Or AutomatedCropInterface should extend CropInterface. e.g. currently causing setAspectRatio seems valid because it's in AutomatedCropInterface but it would cause an error when an object implementing CropInterface is used instead.

3. I'm not sure why the internal method getCrop is changed to setCrop even though its behaviour is not changed. I think it's convention to have setX take argument X but here it's setX(Y) so using "get" still makes more sense because you're getting a crop for an image. Reverting this change also reverts the change in indentation which makes the patch easier to review. If this should be changed then it can be done in a follow-up issue as it's an internal method.

4. We should possibly move the check for the automated_crop module into the setAutomatedCrop method so that someone else can't shoot themselves in the foot when adjusting this class in half a year.

Finally we should possibly look into creating an automated crop service or plugin that can be sought for instead of creating an optional dependency on the automated_crop module. That would open up this module to use other types of cropping/automated cropping in the future besides the automated crop module : )

Thanks for all the work so far!

woprrr’s picture

@Kingdutch When I can spent more time into this I reply you (probably this week).

Here a patch to solve Transform dimension when AutomatedCrop have calculated sizes.

woprrr’s picture

Hi all,

For #1
Miss deleting ! thanks

For #2
Yes, it's true if we admit any crop system can't exist without CROP API. But For Automated crop this is not the reallity, Automated Crop was designed to be totally agnostic regardless Crop API and we does be functional with or without Crop api. In few cases Crop api isn't needed and user only need to define automatically without data persisting.

Another solution can be the following scenario :
setCrop -> if crop api have a crop findCrop() return data and that it
setCrop -> any crop found -> automatic provider are defined -> use it to calculate cropBox -> create a crop entity and return it

With last scenario we have only a CropInterface or False and we respect both approach for automated_crop and possible combo with ImageWidgetCrop can be interesting for caches too. The counter argument we lost flexibility because automatic crop are calculated once (that look good to me but not sure that match with all usecases).

For #3
This method isn't a getter at all. This method set crop property for given image ressource. We can call it applyCrop possibly or getCrop if #2 are applied then setAutomatedCrop() method will create crop instance and persist if we are again in scenario of getter.

For #4
No, this method is extracted from setCrop and exist only to create an automatedCrop plugin instance if that method are in automated_crop that a duplicate of plugin createInstance work.

Finally we should possibly look into creating an automated crop service or plugin that can be sought for instead of creating an optional dependency on the automated_crop module. That would open up this module to use other types of cropping/automated cropping in the future besides the automated crop module : )

For Crop API automated crop feature is only a fallback because that a manual crop effect. The fallback exist to permit user to create a crop entity from automated crop calculated datas. You can also combine effects manual crop and automated crop :) or use automated crop as fallback. Automated crop already permit to add provider plugins for the moment we only have automated crop default provider with mathematic calculation based on center of images and datas added, all interest of crop api fallback is to re-use crop types datas to define an automatic crop realistic for that datas. In all cases Crop API are only a storage module and doesn't assume any calculation like automated crop. Automated crop will prepare all data from it's provider plugins and take it to fallback. This combinaison permit all possible combo IMHO.

Finally Very thank you for your review and questions !!! That patch was written so fastly ... and your idea make futur feature better !!!!

woprrr’s picture

Here lot of optimisations and @Kingdutch feedbacks.

All are functional but possibly we can improve more this patch :) Please purpose and make this feature better :)

Status: Needs review » Needs work

The last submitted patch, 36: 2830768-crop-automated_crop_integration-36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rosemaryreilman’s picture

In testing Patch 36 - I'm getting a Error: Call to a member function size() on null in Drupal\crop\Plugin\ImageEffect\CropEffect->transformDimensions() (line 287 of /modules/contrib/crop/src/Plugin/ImageEffect/CropEffect.php)

woprrr’s picture

@rosemarystanley 🥺 sorry maybe I have missed one thing in my patch because to me all work as a charm :O I will reroll patch fast and test again

Can you describe your reproduction patch please ?

Kingdutch’s picture

Hi Woprrr,

Thank you for your detailed response! I may have gone wrong in my last review to point out specific code instances while there is a more fundamental issue in this patch that I disagree with. I'll try to outline my thoughts below. Please do let me know if I made assumptions that you think are incorrect because my knowledge of the Crop API and Automated Crop module is limited.

From what I understand the Crop API module is basically what it says on the box. It's an API that allows other modules to provide ways of cropping images in a uniform way so that a user can easily switch between different cropping modules.

In this model of the Crop API providing an API to be module agnostic (module independent) then it should be so that the API itself has no knowledge of individual cropping modules. This is what allows it to remain flexible and ensures that users can switch out cropping modules easily.

What happens in this patch is that the Crop API module gets an optional dependency on the automated Crop module. Basically if that module is enabled then the behaviour of the API changes. I don't think this should happen because it cause the API to lose its main feature of being module agnostic.

I believe that the Automated Crop module should do it's work at some other point so that no change in the Crop API is needed. If that's not possible then we should outline what is missing so we can solve it in a way that does not require the Crop API to know about the Automated Crop module. That would provide a more generic solution that can also be used by other modules.

Thanks!

~ Kingdutch

rosemaryreilman’s picture

@woprrr Hey - no problem - Sorry for not leaving further details. I get the error after I tried to make a request to the image URI of the style that automated crop was supposed to be generating. Hope that helps.

woprrr’s picture

Hii @Kingdutch

I totally understand what you mean now and you totally right crop api as project already describe are a storage module and an api to provide unified tools to manage crops without concret implementation like iwc focal point etc...

The relations between crop api and automated crop are more crop api a real api and automated crop a machup module. Automated crop can live alone and do it alone because that fondamentaly different than manual crop approach (logic). But the feature and flexibility offert by automated crop interest few people and I need more people to thing more the futur of crop ecosystem like you !

I admit in that patch I mix the concept and break abstraction of crop api but this start of poc do permit to conduce that discuss and that great ! I already have changed lot of things in patch now crop api have a generic element « automatic_crop_procider » and that induce we have to listen an event to recover all provider and call the chosen provider after submit. In that patch I already have isolate all automated crop logic to be extracted in automated crop sub module in future ;) we have to propagate an event to subscribe in others modules and implement the method getAutomatedCrop with this we can work without this optional dependency and automated crop can purpose a sub module optional very simple to subscribe this event and apply his logic.

Anyone can purpose that or I do continue but more slowly because I’m pretty busy atm ... i do my best without lot of time ;)

But I really appreciate this discuss and your pragmatic approach I love it !!

Rosemary I go to test it with a clean env with only that patch applied. Thanks all

woprrr’s picture

Hi all :) Happy to having small time to purpose a new patch and clean solution.

Here the patch to use new approach to assure Crop api haven't any dependency and can be agnostic as needed. Two new events are now dispatched when we need to calculate crops, if any crops are defined and manual_crop effect set to use a specific Fallback provider then getCrop() method dispatch the second event and collect automatic crop from chosen crop provider in image effect configuration. To show how this works you will need this patch #3020826: Crop api fallback provider from automated crop module that subscribe in crop api events her plugins as automatic crop provider. This logic is separated in sub-module (warning enable it to test all automated crop fallback with crop api). In others words all peoples can now offert her own logic as automatic crop provider. Automated crop just use events provided by crop api now in his new sub-module.

Please can you review it do not hesitate if you see anything because I was written this patch fastly and with my poor english ;P To have a better experience with automated crop subscribers please use this patch too #3017217: Automated Crop applied reversed aspect ratio to already existing media

We probably need to write tests !!! but if anyone can help that's would be GREAT :)

Thanks,
Best regards.

Status: Needs review » Needs work

The last submitted patch, 43: 2830768-crop-automated_crop_integration-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

woprrr’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
14.3 KB

Oops small miss hunk ;) sorry for disturb this patch will be good :)

woprrr’s picture

Status: Needs review » Reviewed & tested by the community

Ready to RTBC now :D

The last submitted patch, 34: 2830768-crop-automated_crop_integration-34.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • woprrr committed 506e976 on 8.x-2.x
    Issue #2830768 by woprrr, bgilhome, Kingdutch, HongPong, rosemarystanley...
woprrr’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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