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

Revert compressed file if larger #229

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ARAKHN1D
Copy link
Contributor

@ARAKHN1D ARAKHN1D commented May 14, 2024

Oxipng and jpegoptim already skip over files that'll be larger by default. pngquant has an option to skip them if larger, which I added. cwebp doesn't have any option for this as far as I know, so the file has to be reverted manually. I'm not sure if there's a cleaner way to do it. The attached image shows the result of a file being skipped/reverted.

I'm also not sure if scour has an option to skip files if they'll end up being larger, but if #227 gets fixed, that won't be a problem. The check I added should catch compressed SVG files that end up being larger anyway.

This currently doesn't properly handle reverting files while in overwrite mode. I'll implement that when I have the time. (Done).

result

Closes #172

oxipng and jpegoptim already do this by default. cwebp doesn't have a
skip option, so manually reverting it is necessary.

Closes Huluti#172
@ARAKHN1D ARAKHN1D marked this pull request as draft May 14, 2024 00:50
This is done by creating a copy of the input file that is then used to
copy over onto the output file, effectively reverting it. This was the
best solution I could find.
@ARAKHN1D ARAKHN1D marked this pull request as ready for review May 14, 2024 23:56
@ARAKHN1D
Copy link
Contributor Author

ARAKHN1D commented May 14, 2024

This should work in overwrite mode. I wish the solution was cleaner, but this was the best I could come up with. I should note something that I found on the Python docs, however:

Warning: Even the higher-level file copying functions (shutil.copy(), shutil.copy2()) cannot copy all file metadata.

On POSIX platforms, this means that file owner and group are lost as well as ACLs. On Mac OS, the resource fork and other metadata are not used. This means that resources will be lost and file type and creator codes will not be correct. On Windows, file owners, ACLs and alternate data streams are not copied.

This does make me a little hesitant to use shutil.copy2(), but this was the best solution I could think of. It's also only really bad if safe mode is off, since the file is overwritten. And, it seems like as much metadata as possible is preserved. Ultimately up to you, though.

This was done because the new code is cleaner and still produces the
desired result, at least from my testing.
I forgot to remove this, I'd imported io in attempts to simplify the
code and forgot it was imported.
@Huluti Huluti added this to the 1.11.0 milestone Jun 7, 2024
@ARAKHN1D
Copy link
Contributor Author

ARAKHN1D commented Jul 2, 2024

Is there anything I could/should do to improve this? I'm still not super enthusiastic about the solution in overwrite mode, but ideally there would just be an option upstream for the WebP and SVG(?) compressors to skip files if larger anyway.

@Huluti
Copy link
Owner

Huluti commented Jul 11, 2024

I will try to review the PR in the next few days, I put it on my TODO :)

@ARAKHN1D
Copy link
Contributor Author

Alright, thank you 👍

@Huluti
Copy link
Owner

Huluti commented Aug 13, 2024

To summarize:

  • jpegoptim seems to already skip larger files (because it has an --force option)
  • pngquant has --skip-if-larger that you added
  • oxinpng already skip larger files (because it has an --force option)

So shutil.copy2() will be useful for libwebp and scour.
But for SVGs I doubt that they can be larger.

So only really useful for libwebp.
Could you add a condition to only apply this feature to webp images?

Sorry for the long review...

if not self.do_new_file:
# Creates a copy of the input file
# This is done in case the output file is larger than the input file
temp_filename = result_item.filename + ".temp"
Copy link
Owner

@Huluti Huluti Aug 13, 2024

Choose a reason for hiding this comment

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

Maybe a '.' in front of the filename could be useful to "hide" the file in file managers? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I added this.

@ARAKHN1D
Copy link
Contributor Author

ARAKHN1D commented Aug 13, 2024

Could you add a condition to only apply this feature to webp images?

I can, but this should only run if the output file is larger. It won't run for SVGs if SVGs aren't going to be larger. Up to you though.

And don't worry about the long review, it's fine :)

Adds a "." to the beginning of the temporary file to hide it from file
managers.
Accidentally forgot to remove this. It was only needed for testing.
Some code that was only supposed to run in overwrite mode was running in
safe mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Revert Image if Bigger
2 participants