endroid / qr-code

QR Code Generator
https://endroid.nl
MIT License
4.33k stars 721 forks source link

Support for WebP and Gif output added (also quality option for PNG) #398

Closed erkens closed 1 year ago

erkens commented 1 year ago

I have made some changes to support the WebP image format. Changed the PngWriter to a generic GdWriter with the possibility to set dynamicly the result with PngResult, WebpResult and GifResult. The first two also have support for an additional quality option.

I have made it that it will not break any existing code.

erkens commented 1 year ago

@endroid That was quick, just comitted a fix for the code quality checks and it was already merged, thanks. Should I create a new pull-request for those changes? (it also includes a small fix for the options to the results that wheren't used when using Label and/or Image)

endroid commented 1 year ago

Hi @erkens thank you, you have a clean coding style. Nice addition, this was something that was on the wish list for a while. I changed some things that I personally want to work differently. For instance I like users to work with concrete PngWriter, GifWriter and WebPWriter classes (and hide implementation details like GD as much as possible). Also added a check to see if GD supports webp (this is not always the case) and updated the Symfony bundle accordingly. Please check the latest version :)

Thanks again!

erkens commented 1 year ago

Everyone has their own code style and preferences. I tried to be as close to your other code style. If it was up to me, I would use enums or consts (because of PHP 8.0, but again if it was me I wouldn't support that version). And create an "ImageWriter" (which is better than GdWriter I think) with the option of the type of output ImageType::PNG instead of what I did earlier. With that setup it would also mean that we have less duplicate code and also just one Result class. But again, everyone is different and this is of course.

Anyway, the best thing is that we now have support for other GD generated images! But in my last change (https://github.com/lelyfoto/qr-code/commit/7232ac13bf7fa95e5a91f71d8fc45fed9dd495b5) I have also fixed an issue with the extra quality option I added, because I didn't used Logo or Image it didn't popup.

I will make a new pull request for that based on your newest version.

erkens commented 1 year ago

I will make a new pull request for that based on your newest version.

I now see that you fixed that by making those internal functions static (I woudln't have done that). But you should also change this line: https://github.com/endroid/qr-code/blob/ff1f769ca6e65f4eb45405b2d54f7b34bf3d3135/src/Writer/AbstractGdWriter.php#L113 to self::addLogo() and https://github.com/endroid/qr-code/blob/ff1f769ca6e65f4eb45405b2d54f7b34bf3d3135/src/Writer/AbstractGdWriter.php#L117 to self::addLabel()

endroid commented 1 year ago

The static should be removed there, missed that, I don't like them too be static too ;)