Support passing PNG data directly to SvgLogo #491
Conversation
| @@ -285,7 +285,7 @@ public SvgLogo(Bitmap iconRasterized, int iconSizePercent = 15, bool fillLogoBac | |||
| using (var bitmap = new Bitmap(iconRasterized)) | |||
| { | |||
| bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); | |||
| _logoData = Convert.ToBase64String(ms.GetBuffer(), Base64FormattingOptions.None); | |||
| _logoData = Convert.ToBase64String(ms.GetBuffer(), 0, (int)ms.Length, Base64FormattingOptions.None); | |||
There was a problem hiding this comment.
GetBuffer() returns the current working memory buffer allocated by MemoryStream, including extra padding that is reserved but not yet used by the MemoryStream. This is unlike ToArray which copies the working data to a new array of the correct length. To properly use GetBuffer, the length must be passed to Convert.ToBase64String so that only the working data is encoded.
| @@ -119,7 +119,26 @@ public void can_render_svg_qrcode_with_png_logo() | |||
| var svg = new SvgQRCode(data).GetGraphic(10, Color.DarkGray, Color.White, logo: logoObj); | |||
|
|
|||
| var result = HelperFunctions.StringToHash(svg); | |||
| result.ShouldBe("78e02e8ba415f15817d5ed88c4afca31"); | |||
| result.ShouldBe("7d53f25af04e52b20550deb2e3589e96"); | |||
There was a problem hiding this comment.
Since the extra padding was removed from the SVG, the hash has changed.
| var svg = new SvgQRCode(data).GetGraphic(10, Color.DarkGray, Color.White, logo: logoObj); | ||
|
|
||
| var result = HelperFunctions.StringToHash(svg); | ||
| result.ShouldBe("7d53f25af04e52b20550deb2e3589e96"); |
There was a problem hiding this comment.
We can see here that the new constructor results in the same hash as using the System.Drawing.Bitmap constructor.
There was a problem hiding this comment.
Hmm, this is false. Well, I was sorta surprised; .NET should re-encode the PNG with the Bitmap constructor. Not sure why I thought otherwise.
Shane32
changed the title
|
Can you please re-check the test-cases? It seem like the changes are breaking one of the test cases @Shane32 |
|
@codebude Done. FYI, I did save and open the outputs of the two affected tests to be sure they still look correct. |
|
@codebude Let me know if there's anything else you want me to look at or help review. Some of the outstanding PRs look to be pretty simple fixes and worth reviewing if you plan to issue a new release (please!). For instance, I could write a PR consolidating these bug fixes and ensuring tests pass, etc -- if you want. Bug fixes that should probably be reviewed:
New features; would take more time to review: |
|
@Shane32 thanks for your support. I really appreciate it! My plan is as follows:
Rough time plan: 4-7 days Does that work for you? If you want to use the QRCoder with your changes in advance, you could also use the current package from the Github packages: https://github.com/codebude/QRCoder/pkgs/nuget/QRCoder |
|
Certainly that works for me and I appreciate your attention. Thanks again, and let me know if I can be of assistance. Just so you know, as maintainer you can push additional commits to any PR where the contributor checked the “allow edits from maintainers” checkbox, which most people do. It’s great for preserving authorship and due credit towards those contributors, but it’s a bit of pain to actually do. You need to configure another source within Git for the repo, which can be done from within VS, and then you can merge across repos, fix tests, etc. (But me as a contributor the most I could do is to create a new PR.) |
|
@codebude I reviewed a number of the outstanding PRs and left comments. Some can be merged as-is; some need tests updated. Let me know if I can be of assistance. Thanks! |
|
Hi @Shane32 , |
|
@codebude I also went through all of the outstanding issues and added a couple PRs: Adding support for Can I ask what your plans are for the remaining bugfix PRs and/or publishing a new release to NuGet? Thanks again, and let me know if I can be of further assistance. |
|
Hi @Shane32 , my plan is still to go through the open PRs and merge as many of them as possible. (Ideally all that can be merged with manageable effort. I would like to reserve larger PR for a later release). Unfortunately, I've already missed the 4-7 days to the next release that I announced in the comments above, but I hope you can see that I'm making (slow but) steady progress. Unfortunately, things are not going as quickly as I would like at the moment. (My little daughter and my main job are currently taking up a lot of my time and they both have priority over my leisure projects). Nevertheless, I hope to be able to release version 1.5.0 soon. I would still like to look at PRs #495, #411, #474 as well as #430 /#438 and (if possible) merge them. After that, I plan to push 1.5.0 to Nuget before continuing with the other issues and PRs. |
|
Sounds great and thanks for the update! |
Summary
This PR fixes/implements the following bugs and features:
What existing problem does the pull request solve?
When using a third-party library to encode image data, or when reading PNG directly from a stream, this additional constructor allows for it to be passed directly to the created SVG file, without passing through the System.Drawing library.
Test plan
Tests have been added to the codebase