Problem/Motivation

We have some memory issues with our current implementation.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

StatusFileSize
new5.43 KB
chr.fritsch’s picture

Status: Active » Needs review
StatusFileSize
new10.81 KB

Here is the patch to review

chr.fritsch’s picture

Project: Media entity Instagram » Media entity slideshow
Version: 3.x-dev » 8.x-2.x-dev
Status: Needs review » Active

Oh god, damn it. Wrong project...

chr.fritsch’s picture

Status: Active » Needs review
volkerk’s picture

Status: Needs review » Needs work
  1. +++ b/src/ThumbnailGenerator.php
    @@ -0,0 +1,208 @@
    +  public function createImage(array $images) {
    

    Function should be called something more obvious like createThumbnail(), createCollage() or simply create().

  2. +++ b/src/ThumbnailGenerator.php
    @@ -0,0 +1,208 @@
    +    $int = hexdec($this->backgroundColor);
    +    $arr = [
    +      "red" => 0xFF & ($int >> 0x10),
    +      "green" => 0xFF & ($int >> 0x8),
    +      "blue" => 0xFF & $int,
    +    ];
    +
    

    Use Drupal/Component/Utility/Color class

  3. +++ b/src/ThumbnailGenerator.php
    @@ -0,0 +1,208 @@
    +    switch (count($loaded_images)) {
    

    What happens when this is called with an empty array? It seems like 'default' case will fail badly.

chr.fritsch’s picture

Status: Needs work » Needs review
StatusFileSize
new10.81 KB
new1.77 KB

Fixed the remarks.

volkerk’s picture

Status: Needs review » Needs work
  1. +++ b/src/Plugin/media/Source/Slideshow.php
    @@ -197,12 +216,12 @@ class Slideshow extends MediaSourceBase implements MediaSourceEntityConstraintsI
    +      ->setPadding(2)
    

    Add moar padding (10).

  2. Needs tests.
kneek’s picture

Looks very nice !!! Thanks a lot for the quick response on the issue. Agree with #8 that it needs more padding

chr.fritsch’s picture

Status: Needs work » Needs review
StatusFileSize
new4.25 KB
new13.23 KB

Here comes the test.

Status: Needs review » Needs work

The last submitted patch, 10: 3193701-10.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
StatusFileSize
new2.69 KB
new13.87 KB

Try to make the tests more robust.

chr.fritsch’s picture

StatusFileSize
new5.25 KB
new12.3 KB

Code cleanup

chr.fritsch’s picture

#13 is broken. Let's stay with #12

volkerk’s picture

Status: Needs review » Reviewed & tested by the community

#12 seems fine

  • chr.fritsch committed 2415fab on 8.x-2.x
    Issue #3193701 by chr.fritsch, volkerk: Replace collage generation by...
chr.fritsch’s picture

Status: Reviewed & tested by the community » Fixed

  • chr.fritsch committed 3f6ada5 on 8.x-2.x
    Revert "Issue #3193701 by chr.fritsch, volkerk: Replace collage...

Status: Fixed » Closed (fixed)

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