🆕 Planning QRCoder 2.0 - let's do it together #512
Comments
|
This second post serves as a placeholder to record decisions/results that we have made as a community for version 2.0. For version 2.0 of the QRCoder we have decided on the following:
New feature that we want to implement in QRCoder 2:
Things that were discussed for QRCoder 2, but made it to production before
|
|
Thanks @codebude for all the effort on this project and seeking the advice of the community here! Below I provide my suggestions and reasoning:
I suggest that all code that can operate with "pure .NET" code should be in the main package. This would include the generation code along with SVG, PNG and other common output formats that do not rely on external dependencies. Now that trimming is available with .NET 6+, and NativeAOT in .NET 8, splitting the project into smaller pieces just makes the project more difficult to use.
See item 3 below.
I think these questions 2 and 3 are the same. If all NuGet packages are released together, having a single repository makes testing and publishing much easier. Most specifically if they are separate, then it's a real pain to utilize unreleased features in dependent repositories. Rather, you have to release the parent package, then test and update the dependent packages, then release them, hoping that you didn't miss something and have to go back and change the parent package. Having a single repository ensures that all tests are run on the same unified version of QRCoder. As for versioning, I think we can learn from MS's example, where with .NET Core 3.x and prior, each package had unique versioning, and with .NET 5+ every package has the same version number. Having the same version number for all related packages makes it very easy for a developer to identify which version is the latest, and which version was tested with what dependency. As a practical example why this is useful, if you pull a 'child' package, say QRCoder.SkiaSharp, and then an update is pushed for the base QRCoder library, developers will see an update to QRCoder.SkiaSharp and pull it, whereas if each package had individual versioning, they would not know that QRCoder had an update without specifically checking. And then to pull it now they would have two references in their project file. This issue occurs today with ASP.NET Core 2.1 applications running on .NET Framework 4.8 (a still-supported scenario), where all user-referenced dependencies are up-to-date, but yet some of the indirectly referenced dependencies are out of date, and worse, have a security-related flaw. This rarely happens with .NET 5+ where most packages are released together, each version referencing the same version of its dependencies.
I don't have a great suggestion here. At GraphQL.NET the documentation is stored within the main repository, and published automatically upon merge to a separate website; we do not use the wiki. It allows any contributor to suggest a change via PR, while providing for review by maintainers. It also keeps the documentation tied to a specific version/commit. It's most beneficial when working on new versions or features, as it allows contributors to update the documentation alongside their feature changes. Maintainers can request that contributors update the necessary documentation before merging the PR. However, we currently have no way of allowing users to view old documentation (besides browsing the repository at a old version). I would suggest that the readme file be maintained and the same file be published in the NuGet package, rather than two separate readme. For the GraphQL.NET Server repository, all the documentation is in the readme file, and as such it is all available on NuGet for any version. If the amount of documentation in aggregate is sufficiently small, then this works well, I believe.
I think the answer depends on the quantity of breaking changes (and ease of developers to upgrade). Due to the popularity of the QRCoder project (27 million downloads and counting), I personally would attempt to release 2.0 with only removal of System.Drawing dependencies, as 1.4.3 and newer do not contain these missing classes anyway. Essentially, I would attempt to have version 2.0 be 100% backwards compatible for the On the other hand, if a complete rewrite of the exposed API is desired, then I would maintain bugfixes for 1.x if/when requested by a contributor. It is quite easy to set up GitHub to allow for this. At GraphQL.NET we can easily release bugfixes for old versions if needed, although we very rarely receive a request to do so; typically users need help upgrading if anything. Having excellent migration documentation really helps in these cases.
I typically push new major version changes into a
I believe this question may be more applicable when there is a larger development team. But in general, I'd suggest that contributors just review goals in a GitHub issue before spending a lot of time on a PR.
I don't think it's necessary. Having all questions posted publicly is likely preferred for an open-source project. Perhaps we just open an issue for v2 and keep it open to track features and ask questions, etc.
I suggest maintaining the existing code. While it's usefulness is likely declining, I see no reason to pitch the existing code. It will certainly be useful for existing users that wish to upgrade to 2.x, and serves as a template (if we like the design pattern) for SkiaSharp / ImageSharp / etc.
I suggest 'whatever contributors wish to offer PRs for'. Ideally:
Compare to ZXing.Net which has these NuGet packages available:
I would use System.Drawing.Primitives if/as desired, as it creates no dependencies except when targeting .NET Standard 1.3.
Framework targetsFor QRCoder 2.x, I would target no less than .NET Standard 2.0. This is in line with Microsoft's example (most of their libraries are still .NET Standard 2.0 compatible) and their stated recommendation: https://learn.microsoft.com/en-us/dotnet/standard/net-standard#when-to-target-net80-vs-netstandard Having a target of .NET Standard 2.0 as a minimum allows near-maximum compatibility with both .NET Framework and other targets, such as the Wilderness Labs Meadow F7 system-on-a-module (which now runs .NET Standard 2.1 code). I would also have a minimum of testing for currently supported and the last couple unsupported LTS versions of .NET. For instance: .NET Framework, .NET Core 3.1, .NET 6.0, .NET 7.0, .NET 8.0. Personally I would retain compatibility with older frameworks, until if/when we run into problems developing. For instance, if the next version of Visual Studio cannot compile for .NET Framework 3.5, then I would drop it. QRCoder v1.x works well and can be used for .NET Framework 3.5. I also would be fine dropping testing on these older frameworks. I also would be fine dropping testing on platforms that do not have a compilation target. As an example, if .NET Core 2.1 and 3.1 both use the same .NET Standard 2.0 compilation target/output, we really shouldn't need to test on both platforms. But if it's easy to test on more platforms, it doesn't cost anything, so why not? I would mark the assembly as trimmable and add some very basic testing for NativeAOT, if we can. This should require almost no effort (as the project does not use reflection) and allows for QR codes to be generated on NativeAOT compilation targets, webassembly / Blazor, and so on. DependenciesI think it would be ideal if the base NuGet package (with outputs for svg/png/ascii) had zero dependencies. However, the character encoding may require System.Text.Encoding.CodePages. Specifically, the ISO-8859-2 character encoding (non-default) is not included natively with .NET and relies on underlying system support. Also the Russian payment method generator relies on a user-determined code page. There are a few ways to handle this issue:
See: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-text-encoding CommitsI suggest using 'squash and merge' to merge PRs in, and require PRs for all commits. This makes the commit history of the main branch very easy to follow. I would suggest that any time master is merged into develop, or vice versa, a standard merge commit is performed. This allows easy and accurate tracking of the state of the code in the master and develop branches at any point in time. With regular merge commits, it is very difficult to follow the progression of the code. Compare the difference: https://github.com/codebude/QRCoder/commits/master/ https://github.com/graphql-dotnet/graphql-dotnet/commits/master/ In the latter example, each commit contains exactly to the final changes of a merged PR. I also typically configure the "default commit message" to be "pull request title". Without this change, the default message when there's only a single commit is the commit title. When the commit is ill-named (e.g. "Updates"), this is an issue. But when using the pull request title, it greatly reduces the frequency of an ill-named commit in the commit history. DebuggingI suggest embedding debug information into the compilation. While the NuGet download is slightly larger, it does not affect execution speed, nor does it affect the size of trimmed applications. The benefit is that developers can right-click any member and 'peek' to instantly view the exact original source code of QRCoder from within Visual Studio. C# Language versionI suggest upgrading to the latest C# language features via |
New feature suggestions
|
|
4. Documentation and wiki: MudBlazor as an example: Depending on how 2.0 shapes out some documentation maybe could not be interactive but could still be in the same docs project. Here is a rough idea for a demo (This is using MudBlazor, ImageSharp, QRCoder): |
My vote would be to clean up the public API and better organize the structure of the main project. Maybe have separate namespaces for the core logic, for payloads and for renderers at least (for better discoverability). At present there are at least a few ways to create a QR Code. There are That said, I had also been experimenting with a more fluent-like extension method approach: public static TRenderer RenderAs<TRenderer>(this QRCodeData data) where TRenderer : AbstractQRCode, new()
{
TRenderer renderer = new TRenderer();
renderer.SetQRCodeData(data);
return renderer;
}Usage: string graphic = QRCodeGenerator.GenerateQrCode(textBoxQRCode.Text, eccLevel)
.RenderAs<AsciiQRCode>()
.GetGraphic(20);There would be some factoring out of Edit: Disregard the bit about the Here is Other random thoughts:
|
|
My thoughts on Question 1, how to split NuGet packages: Original QRCoder package stays v1.x. I feel that keeping the same package name for v2 would be a landmine for folks blindly upgrading and complaining about things being broken. An updated NuGet package Readme and prominent documentation can point them to new v2.0 packages. New Packages (v2.x):
Renderer and Payload packages would have a dependency on abstractions defined in the Core package. The thought is that it would be pay for play. If you want to bring your own renderer to the game, you don't have to bring in anything other than the Core package as a dependency. Just supply an implementation for the abstractions defined in the Core package. |
|
Regarding the core abstraction for Renderers, this commit removed the genericity of In my opinion, something like the original abstraction would be preferable. Is COM support a MUST in v2.0? If so, can it be supported in some other way without limiting the design? |
|
I suggest removing COM support |
|
I have been working on a prototype branch. Most things are still unimplemented (a matter of migrating over and cleaning up existing code). I had just been focusing on the project structure/organization and public API changes. I would also wait for performance PRs to land before migrating over the core QR generation code. If anyone's interested, the |
|
I was thinking about the
Does a payload ever need to specify a specific size (version), ECC mode or Eci mode? They are never overridden. Seems like logically these are just encoding details and not "payload", and as such perhaps it would make more sense for those options to be specified during generation, and not be part of the |
|
Hi @Shane32
You are wrong this time. The
The idea behind the payload generators is to make it as easy as possible for users to create a QR code according to a given specification. Sometimes the specifications also contain requirements for version or ECC level. In fact, this is not only the case for the SlovenianUpnQr, but also for the following payload generators:
I opened an issue for this techincal debt: |
|
Got it. Perhaps then these settings inside the payload class should be like constraints? Perhaps an exception is thrown if trying to generate a QR code that conflicts with a constraint? |
If it were only up to me, we could happily remove COM support. However, I fear that the feature will be used more than expected. As part of the SwissQRCode, I have received a number of requests asking if I could help integrate the generator into Excel or Access via COM. (Due to lack of time and confidence in my COM skills, I have never done this for 3rd parties - but I suspect that some others have.) I would hate to offend these users. If there is a way (additional wrapper library, etc.) to make the QRCoder usable in COM environments and at the same time get rid of the current implementation, then we can throw away the current support. (Otherwise we propably would have to continue to support the 1 version at least in terms of payload generator updates). |
This is not really possible. If the user passes the payload object to The only way I can see to ensure that there are no conflicts would be to make the ToString() method no longer directly callable or to display a warning if it is called outside of CreateQRCode. |
Right, just a check here in case someone tries to override with an invalid value.
That's fine; users may want to deviate from spec. But normally I expect they are using See sample at #526 |
|
I was a bit unsure about the
You make a good point about I've done so in this commit by adding a Regarding the enforcement of individual I also added a |
|
Also, could add an optional escape hatch to allow a user to explicitly relax Payload spec enforcement (if that's a valid use case). Maybe something like a PayloadSpecEnforcement enum that defaults to Strict. |
Honestly, I think it's overcomplicated. One of the benefits to QRCoder is its simplicity. I don't see a benefit to separating payload settings from the Why don't we examine what deficiencies there are with the existing API before jumping into a full rewrite? For example, if we want a payload to be able to represent an arbitrary bitstream (including data length, format and encoding bits), then we may want to discuss changes to the API. I have similar feelings about separating QRCoder.Core from QRCoder.Payloads and the dependency-free QRCoder.Renderers. Many frameworks over time I've seen consolidate their NuGet packages, not broken them apart. In the scheme you've proposed @csturm83 , even if you wanted a dependency-free renderer, you'd need to consume both QRCoder.Payloads and QRCoder.Payloads at a minimum. Besides being inconvenient, it allows for accidental use of multiple incompatible versions between packages. The entire compiled QRCoder.dll is only 138 kb as it is now, and .NET supports trimming for Blazor and AOT-compiled scenarios. Let's just focus on making sure the library works well with trimming. |
|
First of all let me start my saying that I think it's great to create a v2 of this really nice nuget package, and I'd be happy to help as much as I can. Now after reading this whole thread, I would like to add my 2 cents. I agree that having so many downloads it means we cannot just bring a bunch of breaking changes at least not to the most common ways of generating simple QR codes. Having said that, I don't think a v2 should stay in the past. I think v2 is a chance to update things, to break a few things, while keeping others the same. As such I think @csturm83 proposal is a good idea. I like the idea of having a Core library that has all the abstractions etc in a single package so you can build on top of the library without ever having to worry which renderer you are using. And I like the idea of not updating the QRCoder main nuget package, as such no inconveniences can happen by a blind update. For which .NET version to target, I think we should always target the latest version of .NET just like Microsoft does with all their nuget packages. You want to use version 8 of Entity Framework Core, you need .NET 8. I also think their nuget verisoning makes the most sense. All the package go together, like that you know that there was a change. Finally if there's ever a need to update 1.x, not updating the base package, but instead creating new QRCoder.Core, etc... would allow for a later update to the package if a security fix or other major fix is required. Just my 2 cents. And looking forward to contributing to the project. PS: I think we should keep backward support as small as possible, since it's a very small set of volunteers, working on this in their spare time. |
|
@Shane32 Thanks for your candid feedback. I agree that any change or design decision for QRCoder v2 has tradeoffs. I generally lean toward cleaning up the public facing API and keeping the internals exactly the same as much as possible. That approach would make backporting fixes to v1 internal logic straightforward. I feel like if backward compatibility and minimal breaking changes is what we're after, that's essentially what v1.x currently is (e.g. Breaking Changes in 1.6.0 Release Notes). That said, I had some additional thoughts that might address your concerns (or not, it's totally ok to agree to disagree :) ).
I assume you mean
I think this can be largely addressed by publishing a
I totally agree that AOT and trimming scenarios are important. I went ahead and checked with the folks over at the
I believe you are referring to this line of code in public virtual QRCodeSettings DefaultSettings { get => QRCodeSettings.PayloadDefault; }You are correct, the payload settings don't technically need to be separated from the // TODO: potentially add Create overload with single string plainText parameter
// that uses QRCodeSettings.PayloadDefault ??
// TODO: If so, maybe rename QRCodeSettings.PayloadDefault to QRCodeSettings.Default
// or QRCodeSettings.DefaultSettings ??I can put it back in
FWIW, I am also not sold on whether we should make changes w.r.t. overriding In that vein, maybe it would be helpful to break out discussion on some of these micro decisions into separate issues to better track, document and gather feedback from the community. We could then link to those issues in the placeholder post:
|
|
ImageSharp is your friend. |
|
At first, thanks for the vivid discussion and all your feedback and input so far. I can't remember that we had such an active and talkative community in the last 10 years of the project. Great! Second I want to excuse for being a little silent in the last days. Currently I'm still struggling to find time besides daily task but I try my best. I'll try to give feedback on the active discussions and your input so far over the course of this weekend.
I thought the same. This issue/thread already starts to become cluttered/hard to follow. |
|
I suggest one main tracking issue (this one?), with lists of related issues, PRs, or discussions at the top of the issue, like this:
We can break out discussions from there into whatever way seems appropriate for the topic -- issue, PR or discussion. When a topic is decided upon, we (that is, @codebude ) can add a checkbox into the row with a comment, like this:
I'd also suggest pinning the main tracking issue, and it might also be a good idea to immediately separate all of the dozen or so initial questions into separate issues. Note: please ignore my selection of issues above; they are just to demonstrate the list functionality. |
|
I have taken your suggestions into account and have untangled the issue and the discussion here and split it into individual issues. All of these issues are linked in the first post. I hope this allows a clearer discussion of the individual topics. I have copied the previous comments and discussion threads as quotes into the respective new issues (and linked the source and author) so that the discussion can continue seamlessly. In addition, these planning issues all have [WIP/QRCoder2] in the title and are labeled with qrcoder2. If I overlooked something, didn't copy it or forgot to link it in the first post, please let me know. Otherwise, let's use this meta-issue here for general topics and status info and as soon as we find an idea for a new subtopic, let's break out into a new issue. If you want to see topics in the first post, ping me. |
|
@codebude Maybe add an issue for target framework support. Not clear if v2 should support older targets like 3.5, 4.0 or netstandard1.3. Unless that's already been decided. |
|
For reference, here is recent MS guidance. |
To be honest, I've never thought about it before or actively decided to use squash and merge instead of merge commits. But I've read up on it a bit and yes, we should try it out. For now, I have implemented the settings as suggested by the .
I had already implemented this rule a few weeks ago. |
|
the discussions of the various topics are in full swing. However, I'm pretty sure we won't get all the issues from the first post resolved before we start implementing v2. (You are already submitted PRs related to v2 in the meantime). Therefore the following suggestion for the further procedure. I would like to finalize and merge the following three PRs: Then I release the current code as v1.6.0 and publish the Nuget packages. I would then create a We can continue to use this issue here (especially the first two posts) to track the development and the things that still need to be clarified. Is that a viable option for you? |
|
Before starting any work on v2, I would like to add another PR (after the above PRs are merged) that will:
Secondly, I'd like to do some XUnit cleanup:
My goal over these last few PRs is to get the current v1 code cleaned up nicely so we have a good codebase from which to kick-off version 2, and enforce upon ourselves good coding practices and a high degree of quality for our new code. I would like to add one last optimization which I think will have a considerable effect, which is to interleave the ECC data to the output stream simultaneously while calculating it. Benchmarks indicate that this is currently a considerable bottleneck of the performance, however, this isn't easy to do, and the current implementation is 'fast enough'. We don't need this to make it in v1.x. |
|
By the way, if publishing to NuGet requires some effort, I can assist automating this. In GraphQL.NET, all you have to do is issue a GitHub 'release', and it will publish to NuGet automatically with the version number specified by the tag in the release. This makes it very easy to either issue new releases or issue security fixes for older versions. |
|
@Shane32 would you recommend treating warnings as errors? |
|
Yes |
Ok, then I'll wait for the PRs before publishing the 1.6.
From what I've read it still should be possible to use categories/traits. In Visual Studio Test Explorer (see nunit/nunit3-vs-adapter#639 (comment)) as also as in dotnet test (see https://stackoverflow.com/a/42287012/251719). I guess it's just our CategoryDiscoverer and Attribute class which seems to be broken. But since we always run all the tests anyway, we can do without the attributes and remove them. I just wanted to make it clear that it still seems theoretically possible to use them.
It's not a lot of work. I have a small shell script that downloads the nupkg from the Github package builds. I then upload it via the nuget.org web interface. All in all, maybe 2 minutes of work. For the fact that there is only one release every now and then, that's enough and I don't see a need to automate it. |
|
I would like to know when 2.0 will be released, thank you! |
This issue is intended to initiate version 2.0 of the QRCoder. I would like to see an active discussion here so that we can shape the implementation of version 2.0 together. Feedback from the community is very important to me.
Why QRCoder 2.0?
The current version of the QRCoder is essentially still based on the first developments from 2013. Although the QRCoder has been repeatedly adapted to the times, large parts are still based on System.Drawing.Common, which since .NET 6.0 has only been supported under Windows.
Since then, this has often led to confusion, as parts of the QRcoder have only been available under Windows since .NET 6.0, which despite I wrote an Wiki entry is far from obvious to all users.
Although there have already been some PRs and suggestions for version 2.0 in the past, I have shied away from adopting the vision of individual contributors without consulting the community. I want to make up for this with this issue.
Version 2.0 should first and foremost provide clarity and create a consistent interface that is available on as many platforms as possible. Since this most likely means turning away from System.Drawing(.Common) and thus requires breaking changes, this release will be merged into a new major version (2.0).
Requirements for QRCoder 2.0
What needs to be clarified?
In order for the implementation of version 2.0 to be a success (both in terms of the result and the path to it), a few decisions need to be made in advance. Here I am hoping for feedback from you, the community.
(To keep things clear and simple, please give feedback in the dedicated issues, linked below).
What else is missing from this list? Do you have any other points? Let us know!
Which issues / PRs does the QRCoder 2.0 solve?
As mentioned, there have already been some issues and PRs in connection with a version 2.0 or the replacement of System.Drawing.Common. I would like to list the ones (that are still open) here in order to track which problems we can solve with the 2.0 release.
PRs
Issues
QRCodeis not found in .NET 7 #435Support
To get the discussion started, I would like to mention some of the contributors who have worked on the QRCoder in the past. I hope I can count on your valuable feedback once again:
@Shane32, @csturm83, @mishfit, @iamartyom, @abdoutech93, @DevonJSmith, @jnyrup, @nayuki, @alexnsfx, @sbrickey, @strawberryfield, @giccifelipe, @gfoidl, @oheim, @SteveGTR, @PolnerA, @MarkusG, @pontusi, @Timwi, @maartenba, @JimmyPun610, @AntonKorn, @emorell96, @natehitze, @cezar-pimentel, @codeputer
The text was updated successfully, but these errors were encountered: