flokosiol / kirby-focus

Better image cropping for Kirby CMS
184 stars 14 forks source link

focusSrcset does not update URL on focus point changes #63

Closed renestalder closed 2 years ago

renestalder commented 2 years ago

I tried to figure this out for a while what the problem could be, but I don't really get it.

So, I use focusPreset(<presetName>) to generate the fallback image for the <img> tag while using focusSrcset(<presetName>) for the <sources> of the picture tag.

And it looks like that when I change the focus of an image a second time, this applied to focusPreset but not focusSrcset. They are clearly getting generated on the server, but the src URL to the new cuts are not update except for the focusPreset function.

Hint: if I delete the media folder and refresh the pages, the URL get correctly applied.

flokosiol commented 2 years ago

Hi @renestalder,

I tried to reproduce your issue, but for me everything works fine with Kirby 3.6 Starterkit and GD.

My config.php with the preset and a srcset preset looks like this:

return [
  'debug' => true,
  'flokosiol' => [
    'focus' => [
      'presets' => [
        'square' => [
          'width'=> 555
        ],
      ],
      'srcsets' => [
        'homer' => [
          '600w' => [
            'width' => 600,
            'height' => 600,
          ],
          '1500w' => [
            'width' => 1500,
            'height' => 1500,
          ],
        ],
      ],
    ]
  ]
];

I modified sites/templates/album.php to use focusPreset() and focusSrcset() like this:

<a href="<?= $image->url() ?>" data-lightbox>
  <figure class="img" style="--w:<?= $image->width() ?>;--h:<?= $image->height() ?>">
    <img
      src="<?= $image->focusPreset('square')->url() ?>"
      srcset="<?=
        $image->focusSrcset('homer');
      ?>"
    >
  </figure>
</a>

As soon as I update the focus point in the panel, all urls are updated as well. Did I miss something in your description?

renestalder commented 2 years ago

@flokosiol Thanks a lot that you took the time to actually test it yourself. Well then, I will check further. There are several places where the issue could happen in my case: Using your plugin in another plugin, using Twig to render the results, calling the focusSrcset function in a plugin's pageMethod instead of the template directly,...

However, knowing that this can't be reproduced with the bare minimum setup already helps.

owzim commented 2 years ago

I have the same issue, I am also using twig. @renestalder have you found a workaround? I am guessing clearing the media folder after page save could be a way, but that's overkill.

owzim commented 2 years ago

@flokosiol @renestalder I found the issue. When using focusSrcset in conjunction with crop: true|false the generated filename will always be suffixed with crop-1 or crop- and not with the focus coordinates like e.g. crop-83-50

$image->focusSrcset([
    '800w' => [
        'width' => 800,
        'height' => 800,
        'crop' => true,
    ],
]);

this is the responsible line

https://github.com/flokosiol/kirby-focus/blob/a906e5f7ef4da2dc23dcb4d681f11b3c7e068ee7/src/Focus.php#L195

The $options array gets unionized and thus the crop: true value will not be replaced with the focus crop value.

Solution:

Either don't use the crop option or this special case should either be documented or circumvented with a simple unset($options['crop']) or an override like in in the focusCrop:

https://github.com/flokosiol/kirby-focus/blob/a906e5f7ef4da2dc23dcb4d681f11b3c7e068ee7/src/Focus.php#L150

renestalder commented 2 years ago

@owzim Awesome that you dig deeper into that. Does the removal of the "crop" option create the same result when the focusCrop is used? Or will there be a difference in cut depending on whether the user actually did set a focus point or not then?

I re-open this issue, assuming @flokosiol would might it to work with the crop option too.

For understanding, I've set the crop option to true on my image definitions because I want to ensure, for example, I always get 1:1 cut, no matter whether the focus point was set or not.

owzim commented 2 years ago

@renestalder

Does the removal of the "crop" option create the same result when the focusCrop is used?

From visual testing the results are the same. I think as long as the crop option is anything other than falsey, cropping will take place (within the kirby image processing functions, which are ultimately used by the focus plugin). The focus plugin generates a truthy value by adding those aforementioned coordinates. So I guess any focus plugin related image processing results in cropping. Correct me if I'm wrong @flokosiol.

For understanding, I've set the crop option to true on my image definitions because I want to ensure, for example, I always get 1:1 cut, no matter whether the focus point was set or not.

That's why I did it as well.

flokosiol commented 2 years ago

Hey guys, thanks for investigating!

I think overriding the crop option as mentioned by @renestalder is the way to go. Hopefully, I will find some spare time on the weekend to implement this little fix.

flokosiol commented 2 years ago

I could recreate the bug and used the fix you both mentioned. Thanks again! Version 3.0.8 has just been published.

renestalder commented 2 years ago

@flokosiol Thank you very much.

Seems to work with everything that is JPG, but the generated and cropped webp and avif images now don't go to the correct folders. That's weird. Will have to check if this was already the case in the previous version.

@owzim That release works in your case?

renestalder commented 2 years ago

@flokosiol I downgraded to v3.0.7 and there, the cropping works with webp and avif, so it seems something's off with 3.0.7. But I will create a new issue for that.

flokosiol commented 2 years ago

Damn. Seems like I should learn how to write tests. Thanks for testing, I'll check that out.