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

feat(theming): Only convert a background image if it benefits from it #36471

Merged
merged 3 commits into from
Mar 1, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Feb 1, 2023

Summary

  • WebP images are generally quite small, converting to pngs would increase the filesize a lot.
  • Small JPEG and PNG images do not benefit from any conversion, so skip it.
  • JPEG images will get quite bigger when converted to PNG so instead convert to progressive JPEG

This also fixes the wrong mime type sent to the browser, as before this every non gif, non svg would have been converted to a png. But the server sent the file extension as the mime type. Now the file type is preserved and only converted to the same file type.

Checklist

apps/theming/lib/ImageManager.php Fixed Show fixed Hide fixed
apps/theming/lib/ImageManager.php Fixed Show fixed Hide fixed
@szaimen szaimen added the 3. to review Waiting for reviews label Feb 1, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Feb 1, 2023
* WebP images are generally quite small, converting to pngs would increase
  the filesize a lot.
* Small JPEG and PNG images do not benefit from any conversion, so skip it.
* JPEG images will get quite bigger when converted to PNG so instead convert to progressive JPEG

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
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.

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@szaimen szaimen requested review from a team, ArtificialOwl and icewind1991 and removed request for a team February 20, 2023 21:05
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@szaimen
Copy link
Contributor

szaimen commented Mar 1, 2023

@skjnldsv therr is also this attempt since you replied in #32787

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 1, 2023
@skjnldsv skjnldsv merged commit 0b966ab into master Mar 1, 2023
@skjnldsv skjnldsv deleted the fix/theming-keep-images branch March 1, 2023 08:54
@blizzz blizzz mentioned this pull request Mar 2, 2023
@Ornanovitch
Copy link
Contributor

Hey @skjnldsv, could this be backported to 25? That would be very useful. Thanks!

@artonge
Copy link
Contributor

artonge commented Jan 18, 2024

/backport to stable25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: theming
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Uploading Theme Login Image inexplicably increases image size significantly
6 participants