Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Only output existing images and allow upscaling #70

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

manuelmeister
Copy link
Contributor

This is a draft pr to limit srcset output to only existing images.

I'm not sure I feel comfortable merging it like this. Maybe we first need a PR to add tests, so we can ensure that everything still works correctly.

Fixes #33

@mficzel
Copy link
Member

mficzel commented Oct 13, 2023

I like the idea, more precisely i think the biggest generated image should be the largest possible image with the required aspect ratio if there is an aspect ratio defined, otherwise the original one.

@manuelmeister
Copy link
Contributor Author

That's exactly what I've done.

I've also increased the minimum php version to 7.4 as it did not support an extended return type.

@mficzel
Copy link
Member

mficzel commented Oct 27, 2023

@manuelmeister thanks a lot for the pr. Need to find some time and mindset for a proper review as this is non trivial.

@manuelmeister
Copy link
Contributor Author

@mficzel Can you have a look again?

@mficzel
Copy link
Member

mficzel commented Jun 1, 2024

I do not like the allowUpscaling argument that is passed around all over the place and adds quite a bit of complexity while i think that upscaling general is not a good idea. Also this pr does some unrelated things that look valuable but should not be mixed in this one.

How about this approach:

  1. Add a method. supportsUpscaling(): bool to the scalableImageSourceInterface
  2. Asset Image source should return false while DummyImageSource should return true
  3. Skip srcset items that would yield a bigger image if the CurrentImageSource does not supportsUpscaling

That way we have strong defaults and anyone that has a need for upscaling images can still create a custom AssetImageSource

@mficzel
Copy link
Member

mficzel commented Jun 1, 2024

@manuelmeister i talked with @bweinzierl at the contribution day and he is willing to help here aswell.

BTW: A thing that is important to me is that the pr only does what is mentioned. The cleanups you added are nice but make reviewing harder. It would be great to split those topics into separate prs.

# Conflicts:
#	Resources/Private/Fusion/Prototypes/Picture.fusion
#	Resources/Private/Fusion/Prototypes/Source.fusion
@manuelmeister
Copy link
Contributor Author

manuelmeister commented Jun 3, 2024

@mficzel I agree with your comments.

@manuelmeister
Copy link
Contributor Author

@mficzel I removed the aspect ratio part from this PR and opened a new one: #77

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this now. However do we really have to add the "supportUpscaling" flag? I see no real user case for that and would leave this out and always assume allowUpscalinj = false. Just to make the tests for us a little bit easier.

Classes/Domain/AbstractImageSource.php Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't render srcset paths that are bigger than the original image
2 participants