Skip to content

Cleanup ifdefs and revise hashing algorithm - #503

Merged
codebude merged 5 commits into
masterfrom
ifdef
Apr 26, 2024
Merged

Cleanup ifdefs and revise hashing algorithm#503
codebude merged 5 commits into
masterfrom
ifdef

Conversation

@Shane32

@Shane32 Shane32 commented Apr 25, 2024

Copy link
Copy Markdown
Owner

Cleans up ifdefs

To the extent possible:

  • SYSTEM_DRAWING is used for when System.Drawing.Common is available
  • NET6_0_OR_GREATER is used to apply SupportedOSPlatformAttribute (maybe in future PR this is polyfilled)
  • !NETSTANDARD1_3 is used to exclude code that requires System.Drawing.Primitives

And for tests:

  • SYSTEM_DRAWING is used for when System.Drawing.Common is available
  • TEST_XAML is used for XAML tests
  • !NETCOREAPP1_1 is used to exclude tests for .NET Standard 1.1, and for hash checks for PNG tests

All other ifdefs are simplified and used as minimally as possible (e.g. NETFRAMEWORK vs NET35 || NET40)

Also, many tests were not performed on .NET 6, probably since the hashes don't match due to a new deflate algorithm. Worse yet, they do not appear to be deterministic, so rerunning the test can produce a different hash. I've changed the hashing algorithm to be deterministic based on the pixel values inside the Bitmap. This only works on platforms with access to Bitmap so the PNG tests still have ifdefs when running on .NET Core 1.1.

@Shane32 Shane32 changed the title [WIP] Cleanup ifdefs Cleanup ifdefs and revise hashing algorithm Apr 25, 2024
Comment thread QRCoder.Xaml/XamlQRCode.cs
Comment thread QRCoder/PayloadGenerator.cs
Comment thread QRCoder/QRCodeData.cs
Comment thread QRCoder/QRCodeData.cs
Comment thread QRCoderTests/ArtQRCodeRendererTests.cs
Comment thread QRCoderTests/Helpers/CategoryDiscoverer.cs
Comment thread QRCoderTests/Helpers/HelperFunctions.cs
Comment thread QRCoderTests/Helpers/HelperFunctions.cs
Comment thread QRCoderTests/QRCoderTests.csproj
@Shane32

Shane32 commented Apr 25, 2024

Copy link
Copy Markdown
Owner Author

lol - looks like I forgot to submit my self review last night!

Comment thread QRCoder.Xaml/XamlQRCode.cs
Comment thread QRCoder/PayloadGenerator.cs Outdated
Comment thread QRCoder/QRCodeData.cs Outdated
@Shane32

Shane32 commented Apr 25, 2024

Copy link
Copy Markdown
Owner Author

I'm heading out now but if you read my comments (which I forgot to post last night), I think it will answer some of your questions. I should be able to answer any other questions by tomorrow.

Comment thread QRCoder/QRCodeGenerator.cs
Comment thread QRCoderTests/ArtQRCodeRendererTests.cs
Comment thread QRCoderTests/Helpers/HelperFunctions.cs
Comment thread QRCoderTests/Helpers/HelperFunctions.cs
Comment thread QRCoderTests/Helpers/HelperFunctions.cs
@codebude

Copy link
Copy Markdown
Collaborator

I'm heading out now but if you read my comments (which I forgot to post last night), I think it will answer some of your questions. I should be able to answer any other questions by tomorrow.

Sure, will check your comments. Thanks again! :-)

@codebude

Copy link
Copy Markdown
Collaborator

One more thing (besides #503 (review)) before merging this, is the compatability matrix: https://github.com/codebude/QRCoder/wiki/Advanced-usage---QR-Code-renderers#2-overview-of-the-different-renderers

Has anything changed due to the updated ifdefs? (I would say no after reviewing the PR, but maybe I've overseen something.)

@codebude

Copy link
Copy Markdown
Collaborator

Having a look onto the compatability matrix, I asked myself why e.g. PostscriptQRCode isn't compatible with .NET Standard 1.3. I checked the history/blames and saw that the exclusion ifdef intially was defined as !PCL" and then !NETSTANDARD1_1`. Since we're targeting 1.3 now, the exclusion is proably not necessary any longer. Same story for SvgQRCode.

I wish I had commented back then which feature/function was missing in PCL when I excluded it, so that I could check now if its available in .NET Standard 1.3. I guess I have to simply try out...

@codebude

Copy link
Copy Markdown
Collaborator

Having a look onto the compatability matrix, I asked myself why e.g. PostscriptQRCode isn't compatible with .NET Standard 1.3. I checked the history/blames and saw that the exclusion ifdef intially was defined as !PCL" and then !NETSTANDARD1_1`. Since we're targeting 1.3 now, the exclusion is proably not necessary any longer. Same story for SvgQRCode.

I wish I had commented back then which feature/function was missing in PCL when I excluded it, so that I could check now if its available in .NET Standard 1.3. I guess I have to simply try out...

Got it. It's System.Drawing not beeing availble for NET_STANDARD1_3. So ignore my last comment.

@Shane32

Shane32 commented Apr 26, 2024

Copy link
Copy Markdown
Owner Author

Has anything changed due to the updated ifdefs?

No; this doesn't change the exposed API.

If you like, I can help write a test that produces a copy of the API so it is easy to tell when it changes. So it will auto generate a file like this:

https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL.ApiTests/net60/GraphQL.approved.txt

In our case at GraphQL.NET, we tailored the routine to be specific per-tfm, which complicates the routine considerably but would be particularly useful here where you have different APIs for various tfms. We also extended the routine to auto update when it fails, so it is simpler for devs.

https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL.ApiTests/ApiApprovalTests.cs

Comment thread QRCoder/QRCodeData.cs Outdated
@Shane32

Shane32 commented Apr 26, 2024

Copy link
Copy Markdown
Owner Author

No; this doesn't change the exposed API.

Correction: with the exception of adding the methods that were accidentally missing from .NET 6

@Shane32

Shane32 commented Apr 26, 2024

Copy link
Copy Markdown
Owner Author

If you like, I can help write a test that produces a copy of the API so it is easy to tell when it changes

@codebude

Copy link
Copy Markdown
Collaborator

If you like, I can help write a test that produces a copy of the API so it is easy to tell when it change

That looks great! I've seen that you already started in #504 - thanks! :-)
Do you mind if I start to pack QRCoder for the release without waiting for 504?

@codebude
codebude merged commit 940e9d8 into Shane32:master Apr 26, 2024
@Shane32

Shane32 commented Apr 26, 2024

Copy link
Copy Markdown
Owner Author

Do you mind if I start to pack QRCoder for the release without waiting for 504?

Certainly! #504 won't change the compiled code.

@Shane32
Shane32 deleted the ifdef branch April 26, 2024 19:55
@Shane32 Shane32 added the refactoring Refactoring of code without changes to functionality label Oct 8, 2025
@Shane32 Shane32 added this to the 1.5.1 milestone Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Refactoring of code without changes to functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants