Problem/Motivation

This should be taken from the 'Textimage Text' effect of the Textimage module.

Proposed resolution

Provide patch and test coverage.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

slashrsm’s picture

Status: Postponed » Active
mondrake’s picture

mondrake’s picture

Status: Postponed » Active

Textimage 8.x-3-0-alpha2 has been released, this can be worked on now.

mondrake’s picture

Status: Active » Needs review
FileSize
89.37 KB

Some initial work, missing tests + several todos still.

Status: Needs review » Needs work

The last submitted patch, 5: 2655044-5.patch, failed testing.

mondrake’s picture

FileSize
93 KB
mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work
mondrake’s picture

Status: Needs work » Needs review
FileSize
18.81 KB
89.2 KB

Some progress, but still lots to do.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 10: 2655044-10.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
92.82 KB

patch with --binary option

mondrake’s picture

Status: Needs review » Needs work
mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
111.28 KB

So, first reviewable patch. Interdiff would be bigger than patch itself, so not attached.

The 'Text overlay' effect allows to overlay text on the image. Text can contain tokens, but only global ones can be processed by core functionalities. Textimage module add some support for node, file and user tokens but requires using its own field formatter. Also, Textimage extends the effect by allowing a preview of the text overlay, like the 'Textimage text' provides. When this goes in, the 'Textimage text' effect can be dropped from Textimage.

ImageMagick support is only partial, like in Textimage - the image toolkit operation actually creates a temporary wrapper for the text overlay as a GD image, flushes to disk, then watermarks it via ImageMagick commands.

mondrake’s picture

FileSize
111.34 KB
slashrsm’s picture

Status: Needs review » Needs work

Few comments:

  1. +++ b/css/image_effects.text_overlay_preview.css
    @@ -0,0 +1,14 @@
    +#text-overlay-preview IMG {
    

    s/IMG/img

  2. +++ b/src/Component/TextUtility.php
    @@ -0,0 +1,74 @@
    +/**
    + * @file
    + * Contains \Drupal\image_effects\Component\TextUtility.
    + */
    

    @file is not required any more in this kind of cases:

    https://www.drupal.org/node/2304909

  3. +++ b/src/Component/TextUtility.php
    @@ -0,0 +1,74 @@
    +    // Convert the offset value from characters to bytes.
    +    // NOTE - strlen is used on purpose here, instead of Unicode::strlen
    +    $offset = strlen(Unicode::substr($subject, 0, $offset));
    

    Explain the reason for this decision.

  4. +++ b/src/Component/TextUtility.php
    @@ -0,0 +1,74 @@
    +    if ($return_value && ($flags & PREG_OFFSET_CAPTURE)) {
    +      foreach ($matches as &$match) {
    +        // Convert the offset returned by preg_match from bytes back to
    +        // characters.
    +        // NOTE - substr is used on purpose here, instead of Unicode::substr
    +        $match[1] = Unicode::strlen(substr($subject, 0, $match[1]));
    +      }
    +    }
    

    $matches it not returned nor it seems we do anything with it after this point.

  5. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +  // $info stores information about image and text wrapper.
    

    This should be converted to proper PHPDoc comment.

  6. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +      array(
    +        'font'          => array(
    +          'name'                  => '',
    

    I prefer short array syntax. But this is more of a personal preference.

  7. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +      // --- Preview effect.
    

    We can remove ---. Applies to all occurrences.

  8. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    +  }
    

    Can we remove this empty function?

  9. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +    if ($image->getWidth() === 1 && $image->getHeight() === 1) {
    +      $image_width = 0;
    +      $image_height = 0;
    +    }
    

    This condition looks strange.

  10. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +      $image_width = $image->getWidth();
    +      $image_height = $image->getHeight();
    +    }
    +
    +    $this->info['image_width'] = $image_width;
    +    $this->info['image_height'] = $image_height;
    +
    

    $image_width and $image_height are not used many times in this function. We could entirely replace them with $this->info counterparts.

  11. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +                'width' => $this->info['image_width'],
    +                'height' => $this->info['image_height'],
    +                'x_pos' => $this->info['image_xpos'],
    +                'y_pos' => $this->info['image_ypos'],
    +              ]
    +            )) {
    +          return FALSE;
    

    Indentation looks strange.

  12. +++ b/src/Plugin/ImageEffect/TextOverlayImageEffect.php
    @@ -0,0 +1,1092 @@
    +      // Convert case, if requested.
    +      switch ($this->configuration['text']['case_format']) {
    +        case 'upper':
    +          $this->configuration['text_string'] = Unicode::strtoupper($this->configuration['text_string']);
    +          break;
    +
    +        case 'lower':
    +          $this->configuration['text_string'] = Unicode::strtolower($this->configuration['text_string']);
    +          break;
    +
    +        case 'ucfirst':
    +          $this->configuration['text_string'] = Unicode::ucfirst($this->configuration['text_string']);
    +          break;
    +
    +        case 'ucwords':
    +          $this->configuration['text_string'] = Unicode::ucwords($this->configuration['text_string']);
    +          break;
    +
    +      }
    

    We could simplify this with something like:

    Unicode::{$this->configuration['text']['case_format']}('text');
    
mondrake’s picture

Assigned: Unassigned » mondrake

Assigning

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
7.57 KB
107.75 KB

Thanks for review @slashrsm!

1. Done
2. AFAICS that's still RTBC, not finally live. In any case the entire module would require to be cleaned up eventually. I suggest to open an issue when the coding standard is finalised, for the module overall.
3. Added comments.
4. Actually $matches is passed in by reference to the method, so it is somehow 'returned'. See the tests in ImageEffectsTextUtilityTest.
5. Done
6. Me too. But here I am trying to move the code from Textimage with as little change as possible. Follow up for converting array() to [] for the entire module in a later stage?
7. Done
8. Done
9. Added a comment to explain why
10. Actually no, because $this->info can be changed by other methods while processing and we need to store starting width/height.
11. Fixed
12. Done

Status: Needs review » Needs work

The last submitted patch, 19: 2655044-19.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
111.37 KB

I keep forgetting --binary option in git diff :(

mondrake’s picture

Created issue #2684181: @file is not required for classes, interfaces and traits re #17.2

If that one lands before this one, this one will require reroll.

mondrake’s picture

FileSize
8.28 KB
109.73 KB

Removed @file docblocks from traits/classes. Patch with --binary option (just to remind myself)

  • mondrake committed 2ef77ed on 8.x-1.x
    Issue #2655044 by mondrake, slashrsm: Add "Text overlay" image effect
    
mondrake’s picture

Status: Needs review » Fixed

So I thought to move ahead and commit this -

a) we're still in alpha
b) little chance to have more reviews for a 100k+ patch, better have it in and address bugs from reports
c) this is not new code (mostly) but the port of the 'Textimage text' effect from the Textimage module, which has already been in alpha for a few months with some use from statistics and no bugs reported

Status: Fixed » Closed (fixed)

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

jo1ene’s picture

"Textimage module add some support for node, file and user tokens but requires using its own field formatter."

How does this work in the context of an Image Style?