Add an "Interlace" effect

Any contribution is welcome! :)

I had a quick look at the sandbox module. Please make sure a patch here follows same approach as other effects thta were introduced - toolkit specific code in image toolkit operation classes, addition of tests, etc. A good example is #2659676: Add "Contrast" image effect

+++ b/src/Plugin/ImageToolkit/Operation/imagemagick/Interlace.php
@@ -0,0 +1,36 @@
+    $this->getToolkit()->addArgument('-interlace Plane');

At least ImageMagick has different interlacing options - None/Line/Plane/Partition - shouldn't we give users the possibility to choose instead of hardcoding "Plane"? One may also want to revert from Plane to None for example.

Then GD will only set on or off the interlace bit.

Also, I think that this option only applies to some image formats (GIF, JPEG). Shall we check the image format before applying the operation?

Also, please add a line to the README.md file with the description of the effect.

Not sure if this can be tested at all on the image itself, but a test could be made just for adding the effect.

@lebster thanks a lot for explanation.

we could make this setting configurable if ImageMagick toolkit selected, Line or Plane

yes having the configurable option on the effect would make sense. Only thing I would suggest (on the image effect) not to check the active toolkit, but just put in the description of the config option that the GD toolkit does not differentiate. Image effects are (should be :)) toolkit-agnostic - it's the toolkit operation that should decide what to do with the configuration that effects pass to them. Other toolkits implementation may take 'line' or 'plane' and do something with it.

regarding the revert, from the documentation and research, type "none" does not allow us to make the the reverse transformation

Clear, thanks. So no "None" option in the image effect :)

BTW, just found this #1883192: Option for progressive jpeg in ImageMagick's issue queue - even if the approach there is different (setting interlace on the toolkit config level rather than on image effect level), still it suggests supporting both 'line' and 'plane'.

VERY good job @lebster! And very nice test concept.

Most probably test fails on DrupalCI because you are exec'ing 'identify - verbose'. This will work locally, but not on the test bots since they have not ImageMagick installed :(

Have a look at ImageEffectsTestBase::executeTestOnToolkits line 171 to see how to skip the test if identify is not reachable.

Minor nitpicks to follow.

Nitpicks:

1. +++ b/src/Plugin/ImageEffect/InterlaceImageEffect.php
   @@ -0,0 +1,73 @@
   + *   description = @Translation("Add interlace image effect.")
   

   this is shown on the effect form UI - better "Specify the type of interlacing scheme"?

2. +++ b/src/Plugin/ImageEffect/InterlaceImageEffect.php
   @@ -0,0 +1,73 @@
   +      '#description'  => $this->t('Use <strong>Line</strong> or <strong>Plane</strong> to create an interlaced PNG or GIF or progressive JPEG image. <strong>Does not matter if GD toolkit selected.</strong>'),
   

   "Does not matter if GD toolkit selected." not a native English speaker but I think better something like "This setting is not relevant for the GD image toolkit"

3. +++ b/src/Plugin/ImageToolkit/Operation/InterlaceTrait.php
   @@ -0,0 +1,33 @@
   +        'description' => 'The interlace type.',
   

   maybe you can add 'required' => FALSE and 'default' => 'Plane' here too. Up to you.

4. +++ b/src/Plugin/ImageToolkit/Operation/gd/Interlace.php
   @@ -0,0 +1,30 @@
   + *   description = @Translation("Add interlace image effect.")
   

   not really... rather "Create an interlaced PNG or GIF or progressive JPEG image"

5. +++ b/src/Plugin/ImageToolkit/Operation/imagemagick/Interlace.php
   @@ -0,0 +1,31 @@
   + *   description = @Translation("Add interlace image effect.")
   

   same here...

6. +++ b/src/Tests/ImageEffectsInterlaceTest.php
   @@ -0,0 +1,100 @@
   +      $generated_uri = 'public://styles/image_effects_test/public/'. \Drupal::service('file_system')->basename($original_uri);
   

   $this->container->get('file_system')

7. +++ b/src/Tests/ImageEffectsInterlaceTest.php
   @@ -0,0 +1,100 @@
   +      $this->assertTrue($this->isImageInterlaced($image), 'Is image interlaced/progressive?');
   

   Note that assert messages are no longer necessary. Up to you.

8. +++ b/src/Tests/ImageEffectsInterlaceTest.php
   @@ -0,0 +1,100 @@
   +    $real_path = \Drupal::service('file_system')->realpath($source);
   

   $this->container->get('file_system')

+++ b/src/Tests/ImageEffectsInterlaceTest.php
@@ -0,0 +1,98 @@
+   * @param ImageInterface $image
+   * @return bool

please use FQN for ImageInterface (\Drupal\Core\Image\ImageInterface), and add comments to @param and @return according to coding standards. I know, I know...

Used another way that checks the "interlaced" flag - http://stackoverflow.com/questions/14235600/php-test-if-image-is-interla....

Great! Let's credit the source then, add a @see http://stackoverflo... to the method's docblock

With that we're RTBC I think

Hm, I think that

function isInterlaced( $filename ) {
   $handle = fopen($filename, "r");
   $contents = fread($handle, 32);
   fclose($handle);
   return( ord($contents[28]) != 0 );
}

only works for PNG, though. So the passes on JPEG and GIF are false positives. Maybe for the purpose of the test we can only have test for the PNG file and call it a day.

@lebster thanks this looks good. Also tested manually. RTBC.

+++ b/src/Tests/ImageEffectsInterlaceTest.php
@@ -0,0 +1,92 @@
+      // Check that ::applyEffect generates interlaced PNG or GIF or progressive JPEG image.

comment exceeding 80 chars, can be fixed on commit

Committed, thank you @lebster!