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

Only resize an image if it results in a smaller image size #32787

Closed
wants to merge 3 commits into from

Conversation

jcjveraa
Copy link

@jcjveraa jcjveraa commented Jun 9, 2022

Made to fix issue 32779.

Current default behaviour (for the last 2 years at least, or more) has been that any background file that is 'not an SVG or a GIF'
will be resized and stored as a PNG. When uploading a large compressed (e.g. webp or jpg) image this default results in (relatively) huge (1 MB+) background images, as per the issue I raised, as it then recompresses this e.g. 151kb jpg file to a 1MB png file. This is counterproductive as (per the original comments in the code) the intent of this function is to reduce file size.

With this change, only if the resulting image has a smaller file size than the original file, the new file is used.

Signed-off-by: JelleV 3942301+jcjveraa@users.noreply.github.com

Current default behaviour (for the last 2 years at least, or more) has been that any background file that is 'not an SVG or a GIF' will be resized *and stored as a PNG*. When uploading a large compressed (e.g. webp or jpg) image this default results in huge (1 MB+) background images. This is counterproductive as the intent of this function is to reduce file size.

With this change, only if the resulting image has a smaller
file size than the original file, the new file is used.

Signed-off-by: JelleV <3942301+jcjveraa@users.noreply.github.com>
Docstring was copy-pasted and not updated.

Signed-off-by: JelleV <3942301+jcjveraa@users.noreply.github.com>
@szaimen szaimen added bug 3. to review Waiting for reviews labels Jun 9, 2022
@szaimen szaimen added this to the Nextcloud 25 milestone Jun 9, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 12 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 12 potential problems in the proposed changes. Check the Files changed tab for more details.

Realized after having a think about my previous commits that the intent
of the original function was also to ensure images are stored in a
progressively loading format. With these further changes, I think the
original intern is captured more closely.

Signed-off-by: JelleV <3942301+jcjveraa@users.noreply.github.com>
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.

@PVince81 PVince81 requested a review from CarlSchwan June 10, 2022 13:09
This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@mrAceT
Copy link

mrAceT commented Oct 7, 2022

Is it OK, that I suggest here that switching to WEBP for all previews might be the best way to go?
(besides the check that the new file should be smaler than the original.. that is always smart..)

PS: WEBP has been around for many years now and is perfect for the purpose we're talking about here..

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@skjnldsv
Copy link
Member

skjnldsv commented Mar 1, 2023

Hey @jcjveraa thank you for this PR.
Sorry it takes such a long time!

Could you address all the psalm issues first?

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 1, 2023
@skjnldsv skjnldsv modified the milestones: Nextcloud 26, Nextcloud 27 Mar 1, 2023
@skjnldsv
Copy link
Member

skjnldsv commented Mar 1, 2023

Superseded by #36471

@skjnldsv skjnldsv closed this Mar 1, 2023
@jcjveraa
Copy link
Author

jcjveraa commented Mar 1, 2023 via email

@skjnldsv
Copy link
Member

skjnldsv commented Mar 1, 2023

@jcjveraa the other one was pretty close/inspired from this one. But it also had tests, so huge bonus ;)
Thank you for pushing this over the last months, really helped moving forward!

@skjnldsv skjnldsv removed this from the Nextcloud 27 milestone Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Custom login page background image size increases x7
5 participants