skip non png files, if --recursive is used

[LuckyTurtleDev]

Fix #547

Files with non png extensions are skipped now, if the --recursive flag is used.
If --recursive is not used non png files "can" still be compressed.

How @TPS has suggested I have also add an flag, to keep the current behavior.
I am not sure if --include-non-png is the best name for this.

Please squash merge.

TODO:

• allow explicitly specified non pngs (see skip non png files, if --recursive is used #548 (comment))
• check for apng extension
• case-insensitive comparison on the extension
• remove --include-non-png flag ?

[andrews05]

Thanks for this!
I don't think it's quite correct yet though. Currently this will skip non-png files even if they were explicitly specified, which is probably not something we should do. I think the correct place to check the extension would be within the dir.filter_map (obviously you'd also need to check is_dir here too).

Again, I would prefer not to add the extra option for including non-png, it seems like unnecessary clutter. It's true that globbing was problematic on Windows, but this has been resolved now.

[edit] It may also be best to do case-insensitive comparison on the extension, to include .PNG files.

[LuckyTurtleDev]

I don't think it's quite correct yet though. Currently this will skip non-png files even if they were explicitly specified,

I am not sure, what you mean with this. If the user does not use the the --recursive flag, non png, will be still be process.
oxipng image.jpg does still try to compress image.jpg.

see https://github.com/shssoichiro/oxipng/pull/548/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR405

Again, I would prefer not to add the extra option for including non-png,

I do not think that the current behavior is expected from the user.
I would not expect that an png compression tool, which run recursive at a folder, would tries to compress non png files.

[andrews05]

I am not sure, what you mean with this. If the user does not use the the --recursive flag, non png, will be still be process. oxipng image.jpg does still try to compress image.jpg.

Right, but if I were to run oxipng --recursive image.jpg (say I have habit of including that flag even when I don't need it) I would expect it to still try optimise image.jpg because I explicitly specified it. Or if I run oxipng --recursive myimagesdir image.jpg it should recursively optimise .png files within myimagesdir but also include image.jpg.

I do not think that the current behavior is expected from the user. I would not expect that an png compression tool, which run recursive at a folder, would tries to compress non png files.

Yes, I agree 🙂. I'm referring to the new option --include-non-png that you added. I don't think we need to keep the old behaviour at all.

[LuckyTurtleDev]

oxipng --recursive image.jpg

Yes you are right, I did not pay attention to the fact that the user can still pass files instead of folders.
This should definitely be fixed.

I don't think we need to keep the old behaviour at all.

I agree to this. It was suggested by @TPS. @shssoichiro should decide if he wants this flag or not.

[andrews05]

I agree to this. It was suggested by @TPS. @shssoichiro should decide if he wants this flag or not.

Cool. I'd suggest you keep it out for now and only add it back in if @shssoichiro asks.

[TPS]

The advantage to the add'l option (& current behavior) is that a PNG-format file, whatever the extension, will still be analyzed & potentially optimized. I agree this may be a rare event nowadays (though, AFAIK, *nixs never enforced extensions on their native filesystems/-types, so I expect more problems there), but that's why it'd be a suboption.

Globbing on Windows is a whole other whack-a-mole problem. It's fine until an edge case is found where the included glob library fails. Those on Windows are used to having options (in place of globbing) for anything that's not selecting a subset of files (& sometimes even then, just to avoid glob).

I actually missed the reasoning why the current behavior should be changed, but leaving an suboption to keep it shouldn't hurt anything, & is currently useful.

[andrews05]

@TPS Appreciate the input!

Having endless options, even very simple ones, ultimately does end up hurting though. It can become a maintenance burden if options need to interact in some way, or simply cause more work during a refactor. It can also just be a visual clutter when working with the code, not to mention clutter for the users reading the docs and trying to understand all the options. It may seem harmless adding an option or two here or there but they do build up over time and can end up becoming a confusing mess.

I believe it's important to carefully consider new options and avoid adding any unnecessarily, such as where there's an easy alternative (which is the case here) or where there's been no demonstration of an actual need (also the case here - you've mentioned a hypothetical but this isn't actually a use case you're personally encountering, is that right?).

Again though, these are just my opinions. @AlexTMjugador or @shssoichiro may think differently 🙂

(@TPS off topic, but I'd be interested to know what is your use case with oxipng 😄)

[TPS]

@andrews05 By this argument, this whole RFE & discussion is irrelevant: leave things as they are, because there's no evidence there's currently a problem, or any benefit to this change.

(OT: As you might've read elsewhere, I'm a huge proponent of FileOptimizer. But it's often overkill & takes so much time. Much of the time, I can get away w/ Compress-or-Die.com. If that's not good enough for PNG, I generally convert there to lossless WebP/JxL. But, if I need PNG, better than CoD, & faster than FO, OxiPNG is my current goto.)

[andrews05]

@LuckyTurtleDev I'll convert this to a draft for now, you can mark it for review when ready.

[AlexTMjugador]

I think that a --include-non-png option is not necessary. I expect its usage to be so niche and better taken care of by shell scripts and the like.

By this argument, this whole RFE & discussion is irrelevant: leave things as they are, because there's no evidence there's currently a problem, or any benefit to this change.

In my view, what @andrews05 was trying to communicate is that, while some users may get some minor benefit from adding this option, in the grand scheme of things, when considering detailed design, maintenance and project scope, that minor benefit may be offset by costs. This is not advocating for "leaving things as they are because there's no evidence", but admitting that some tasks are better dealt with by the users themselves, which know exactly what they are up to. And in this case, I agree that a --include-non-png option falls under the category of "oddly specific switch that is better dealt with users themselves".

[andrews05]

@LuckyTurtleDev We should also include files with apng extensions.

[TPS]

Who knows how many more extensions?

[andrews05]

Wikipedia knows 🙂
"Filename extension | .png .apng"

[AlexTMjugador]

Hi @LuckyTurtleDev, thank you a lot for this PR! ❤️

We'd like to move forward with the next OxiPNG release and I think this PR could make it into such a release, but I notice it's on a draft status and there are some pending concerns.

In my view, the removal of the --include-non-png option is acceptable, and I would merge this PR when the rest of the items on the TODO list are resolved. Do you have any other concerns or doubts that are hindering the progress of this PR? Ask away 😄

[LuckyTurtleDev]

@AlexTMjugador I think everythink should be ready now. Some should review the changes. I have not test this yet.

[andrews05]

Looking good @LuckyTurtleDev! Just left a couple of comments.
I'd also suggest updating the help for the flag. Something like:
Recurse into subdirectories and optimize all *.png/*.apng files

[AlexTMjugador]

I think this is good to go, thank you!