search

codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.66k stars 1.1k forks source link

🆕 Planning QRCoder 2.0 - let's do it together #512

Open codebude opened 6 months ago

codebude commented 6 months ago

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 - https://github.com/codebude/QRCoder/pull/321 - https://github.com/codebude/QRCoder/pull/407 - https://github.com/codebude/QRCoder/pull/460 - https://github.com/codebude/QRCoder/pull/460
Issues - https://github.com/codebude/QRCoder/issues/242 - https://github.com/codebude/QRCoder/issues/308 - https://github.com/codebude/QRCoder/issues/315 - https://github.com/codebude/QRCoder/issues/366 - https://github.com/codebude/QRCoder/issues/377 - https://github.com/codebude/QRCoder/issues/382 - https://github.com/codebude/QRCoder/issues/389 - https://github.com/codebude/QRCoder/issues/393 - https://github.com/codebude/QRCoder/issues/435 - https://github.com/codebude/QRCoder/issues/441 - https://github.com/codebude/QRCoder/issues/489

Support

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

codebude commented 6 months ago

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

Shane32 commented 6 months ago

Thanks @codebude for all the effort on this project and seeking the advice of the community here! Below I provide my suggestions and reasoning:

1. How do we split the QRCoder

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.

2. How do we split the QRCoder? (Github repositories)

See item 3 below.

3. Version number handling

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.

4. Documentation and wiki

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.

5. How do we handle changes during the 2.0 development

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 net6.0 tfm. If that was the case, I think there is no reason to continue development on 1.x.

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.

6. Coordination of development

I typically push new major version changes into a develop branch, keeping master for the current major version. Inevitably bug fixes come up before the next major version is ready, and then it is easy to push these changes into master while still continuing to develop v2. Also, it allows backwards-compatible features to be pushed into v1 while also being merged into v2. A good example would be the performance improvements PR #509 . If we had been adding SkiaSharp functionality into the master branch, then it would not be possible to push performance improvements into v1. Typically at GraphQL.NET when we release a new major version, I simultaneously release the last version of the current/prior major version alongside the new major version; then I merge develop into master and continue on master.

7. Coordination of development (Organizaion)

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.

8. Communication (outside GitHub)

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.

9. System.Drawing vs X:

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.

10. What will be X?

I suggest 'whatever contributors wish to offer PRs for'. Ideally:

Compare to ZXing.Net which has these NuGet packages available:

11. Use of System.Drawing.Primitives

I would use System.Drawing.Primitives if/as desired, as it creates no dependencies except when targeting .NET Standard 1.3.

12. What else is missing from this list?

Framework targets

For 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.

Dependencies

I 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:

  1. Eliminate the dependency. Users can install the code page provider themselves for full support.
  2. Eliminate the dependency, and write a custom encoder for ISO-8859-2. This should be quite easy to do. Users can install the code page provider themselves if they are creating a Russian payment QR code and require a non-standard code page.
  3. Include the dependency when targeting .NET Standard 2.0. (Note: this hurts trimming for NativeAOT scenarios.)

See: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-text-encoding

Commits

I 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.

Debugging

I 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 version

I suggest upgrading to the latest C# language features via <LangVersion>latest</LangVersion>. This means that we need to be careful to not to use features like init-only properties which will break older targets. But some things like file-scoped namespaces are very nice to use and do not change the compiled code at all.

Shane32 commented 6 months ago

New feature suggestions

digitaldirk commented 6 months ago

4. Documentation and wiki: I would love to have a documentation site that is interactive. A Blazor wasm project that could be in this repo or seperate if wanted. I would be happy to contribute towards this. I have made a few webites/pages that allow for QR code customization using Blazor. I personally find Github's wiki a little lacking.

MudBlazor as an example: https://mudblazor.com/components/slider https://github.com/MudBlazor/MudBlazor

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): InteractiveQRDemo.webm

csturm83 commented 6 months ago

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.

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 QrCodeGenerator.CreateQrCode instance methods (1) which delegate to the QrCodeGenerator.GenerateQrCode static method (2), and then there are also the GetQrCode static helper methods (3). Maybe just pick one preferred way moving forward?

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 IDisposable needed to make it work. Not sure why the MemoryStream in PngByteQRCode needs to be a class variable. Seems as though the MemoryStream lifetime could be managed in the GetGraphics methods (plumbing the Stream through the helper methods). I also wasn't sure of the value of Dispose functionality in QRCodeData? Maybe @codebude could clarify if it could be removed.

Edit: Disregard the bit about the MemoryStream implementation in PngByteQRCode. I was looking at the PngBuilder IDisposable implementation by mistake. All *QrCode implementations implement IDisposable by way of AbstractQrCode's Dispose method, which simply disposes the QRCodeData.

Here is QRCodeData.Dispose (not actually releasing unmanaged resources, just setting to default values):

public void Dispose()
{
    this.ModuleMatrix = null;
    this.Version = 0;

}

Other random thoughts:

csturm83 commented 6 months ago

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.

csturm83 commented 6 months ago

Regarding the core abstraction for Renderers, this commit removed the genericity of AbstractQrCode (renderers) to support COM.

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?

Shane32 commented 6 months ago

I suggest removing COM support

csturm83 commented 6 months ago

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 QRCoder.Core, QRCoder.Payloads and QRCoder.Renderers projects in that branch contain the beginnings of a proposed project structure and public API (detailed a bit above). Throwing it out there for feedback or suggestions.

@codebude

Shane32 commented 6 months ago

I was thinking about the Payload class. Currently there are options for:

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 Payload class.

codebude commented 6 months ago

Hi @Shane32

Does a payload ever need to specify a specific size (version), ECC mode or Eci mode? They are never overridden.

You are wrong this time. The SlovenianUpnQr payload generator uses this feature. See: https://github.com/codebude/QRCoder/blob/master/QRCoder/PayloadGenerator.cs#L2382 The values from the payload are used in the corresponding CreateQrCode overload in the QRCodeGenerator. See: https://github.com/codebude/QRCoder/blob/master/QRCoder/QRCodeGenerator.cs#L42

Seems like logically these are just encoding details and not "payload"

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:

Shane32 commented 6 months ago

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?

codebude commented 6 months ago

Is COM support a MUST in v2.0? If so, can it be supported in some other way without limiting the design? sub>@csturm83</sub

I suggest removing COM support sub>@Shane32</sub

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).

codebude commented 6 months ago

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? sub>@Shane32</sub

This is not really possible. If the user passes the payload object to CreateQrCode, then the values from the payload are automatically pulled in. However, if the user calls the ToString() method and passes the string to CreateQrCode (here he could also set other values for ECCLevel, etc.), then CreateQRCode lacks the context/payload object to check constraints.

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.

Shane32 commented 6 months ago

If the user passes the payload object to CreateQrCode, then the values from the payload are automatically pulled in.

Right, just a check here in case someone tries to override with an invalid value.

However, if the user calls the ToString() method and passes the string to CreateQrCode (here he could also set other values for ECCLevel, etc.), then CreateQRCode lacks the context/payload object to check constraints.

That's fine; users may want to deviate from spec. But normally I expect they are using CreateQrCode with the payload object.

See sample at #526

csturm83 commented 6 months ago

I was a bit unsure about the Payload implementation so had left it alone initially in my prototype branch. The discussion above has been really helpful and got me thinking..

This is not really possible. If the user passes the payload object to CreateQrCode, then the values from the payload are automatically pulled in. However, if the user calls the ToString() method and passes the string to CreateQrCode (here he could also set other values for ECCLevel, etc.), then CreateQRCode lacks the context/payload object to check constraints.

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.

You make a good point about ToString(). It's a bit unfortunate that the original PayloadGenerator overrides object.ToString() as an implementation detail. Pretty sure we cannot make an object.ToString() overload 'no longer directly callable', but we could not override object.ToString() in v2 and instead use a different internal implementation.

I've done so in this commit by adding a protected abstract method that is responsible for stringifying the payload and an internal method that delegates to it. The internal method will only be visible to the QRCoder.Core package and the QR generation logic. You still can't technically prevent a consumer of the library from calling ToString() on a Payload, but it would now return the type name as a string instead of a stringified Payload (default behavior of object.ToString()). That would need to be called out in the v2 migration guide, and would be hopefully very obvious in testing.

Regarding the enforcement of individual Payload specifications:

I also added a public virtual IsPayloadSpecificationMet(QRSettings) method that by default returns true. Individual Payloads could override this method to convey their 'constraints' to the generation logic. The generation logic would call this method on the Payload and throw if false is returned. I also added a commit to demonstrate using that method in conjunction with a new virtual DefaultSettings property on Payload (which would ultimately replace the other 3 existing virtual properties). Let me know what you think.

@codebude @Shane32

csturm83 commented 6 months ago

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.

Shane32 commented 6 months ago

Let me know what you think.

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 Payload class. I don't see that ToString() isn't appropriate, given that every QR code is intended to represent a string (in one encoding or another). Would I have done it that way if I was coding it from scratch (used ToString() in this fashion)? Perhaps not. But I think we should have a good reason for making API changes when there has been 27 million downloads and people are used to the existing API. Let's try to make it as easy as possible to upgrade. And if at all possible, the common use-cases should be backwards-compatible.

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.

emorell96 commented 6 months ago

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.

csturm83 commented 6 months ago

@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 :) ).

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.

I assume you mean QRCoder.Core and QRCoder.Renderers, which would be true if you are targeting the NuGet packages individually.

Besides being inconvenient, it allows for accidental use of multiple incompatible versions between packages.

I think this can be largely addressed by publishing a QRCoder2 metapackage which could keep compatible dependencies versioned in lockstep. The mainstream scenario would likely be to target the metapackage with the option to only reference one or two packages individually if you didn't need them all. You could even extend the approach to include QRCoder2.Crossplat or QRCoder2.Windows metapackages to be very explicit on which packages/versions work together for which platform.

Let's just focus on making sure the library works well with trimming.

I totally agree that AOT and trimming scenarios are important. I went ahead and checked with the folks over at the dotnet/runtime repo to see if breaking a package down into multiple smaller packages could cause any issues with trimming. Here is their response.

I don't see a benefit to separating payload settings from the Payload class.

I believe you are referring to this line of code in Payload.cs in my prototype branch:

public virtual QRCodeSettings DefaultSettings { get => QRCodeSettings.PayloadDefault; }

You are correct, the payload settings don't technically need to be separated from the Payload class via QRCodeSettings.PayloadDefault. It could just as easily been defined as a static Payload.DefaultSettings property in the Payload class. I originally started with that, but was toying around with the idea of having the property available to QR code generation in general (for string overloads as well, not just Payloads).

  // 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 Payload.cs if we don't think it's worth using default settings more broadly.

I don't see that ToString() isn't appropriate, given that every QR code is intended to represent a string (in one encoding or another). Would I have done it that way if I was coding it from scratch (used ToString() in this fashion)? Perhaps not.

FWIW, I am also not sold on whether we should make changes w.r.t. overriding Payload's object.ToString(). I was looking to demonstrate what is possible based on what @codebude mentioned regarding enforcing Payload constraints. On one hand, allowing a user to inadvertently circumvent Payload constraints is a bit of a footgun, but maybe there are valid use cases where a user would like to stringify a Payload for some other purpose. If nothing else, further thoughts or discussion on the topic might be helpful before making any decision.

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:

This second post serves as a placeholder to record decisions/results that we have made as a community for version 2.0.

catdsnny commented 6 months ago

ImageSharp is your friend.

codebude commented 6 months ago

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.

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

I thought the same. This issue/thread already starts to become cluttered/hard to follow. I would propose to either create an issue per main topic for further discussions or to transfer this issue to a discussion. (As GitHub discussions allow sub threads so that we could keep answers tied to the initial posts/topics.) Any preferences? (In terms of separate issues vs. GH discussions.)

Shane32 commented 6 months ago

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.

codebude commented 6 months ago

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.

csturm83 commented 6 months ago

@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.

csturm83 commented 6 months ago

For reference, here is recent MS guidance.

codebude commented 6 months ago

@csturm83 I carved out the target framework discussion into this issue: https://github.com/codebude/QRCoder/issues/553 and linked it (as also as your issue from yesterday) in the first post.

codebude commented 5 months ago

I 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.

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 .

image

[...] and require PRs for all commits.

I had already implemented this rule a few weeks ago.

codebude commented 5 months ago

Hi @csturm83 @Shane32 ,

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 develop/v2 branch from the master in which we start development for v2. I would then stop any further changes to v1 (unless they are security-relevant).

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?

Shane32 commented 5 months ago

Before starting any work on v2, I would like to add another PR (after the above PRs are merged) that will:

  1. Add editorconfig with standards for naming, etc, including using file-scoped namespaces
  2. Reformat code via dotnet format to match editorconfig, and complete any non-automatic formatting requirements
  3. Add test to enforce compliance

Secondly, I'd like to do some XUnit cleanup:

  1. Remove the XUnit [Category] attributes which have no effect today
  2. Figure out why each test is listed twice in Text Explorer.
  3. Add Linux testing for the non-Windows tests

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.

Shane32 commented 5 months ago

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.

csturm83 commented 5 months ago

@Shane32 would you recommend treating warnings as errors?

Shane32 commented 5 months ago

Yes

codebude commented 5 months ago

Before starting any work on v2, I would like to add another PR (after the above PRs are merged)

Ok, then I'll wait for the PRs before publishing the 1.6.

Remove the XUnit [Category] attributes which have no effect today

From what I've read it still should be possible to use categories/traits. In Visual Studio Test Explorer (see https://github.com/nunit/nunit3-vs-adapter/issues/639#issuecomment-515124100) 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.

By the way, if publishing to NuGet requires some effort, I can assist automating this.

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.

HuaFangYun commented 3 months ago

I would like to know when 2.0 will be released, thank you!