[WIP/QRCoder2] Consider Updated Abstractions

[csturm83]

QRCoder v2 provides a unique opportunity to streamline and clean up some of the rough edges of public API that has evolved and accumulated over the years.

Potential Goals of an Updated API

• Intuitive
• Discoverable
• Consistent
• Clear (succinct) semantics
• Extensibility
• Maximize code reuse
• 'Pit of success'

Some suggested semantic changes:

• AbstractQrCode --> Renderer
• ***QrCode --> ***Renderer
• QRCodeGenerator.CreateQrCode --> QRCode.Create
• GetGraphic --> Render

Example Renderer Abstraction

Sample Usage

// 'traditional' three-step rendering
QRCodeData qrData = QRCode.Create(payload, qrSettings);
TestRenderer renderer = new TestRenderer(qrData);
byte[] renderedt3 = renderer.Render(renderSettings);

// fluent three-step rendering
byte[] rendered3 = QRCode.Create(payload, qrSettings)
    .RenderAs<TestRenderer>()
    .Render(renderSettings);

// fluent two-step rendering
byte[] rendered2 = TestRenderer.Create(payload, qrSettings)
    .Render(renderSettings);

// one-shot rendering
byte[] rendered1 = TestRenderer.Render(payload, qrSettings, renderSettings);

[Shane32]

I like this concept of a fluent API. It works very well to allow the users to utilize intellisense to determine what they can and cannot do. I suggest we come up with additional samples of common and complex patterns users commonly need to see how they will play together. We can also consider taking this a step further to apply options rather than having options as parameters, often with many overloads. For instance:

var renderedArray = QRCode
    .CreateUrl("http://google.com")
    .WithErrorCorrection(EccLevel.Q)
    .RenderWith<PngRenderer>()
    .WithSize(10)
    .ToByteArray();

QRCode
    .CreateSms("1234567890", "Sample subject")
    .WithQuietZones(false)
    .RenderWith<WindowsRenderer>()
    .WithColor(Color.Red, Color.Transparent) // where applicable, a method may specify multiple properties
    .WithIcon(bitmap, 15, 0) // here the size (percentage) and border width are specified along with the icon
    .ToStream(stream); // writes to an existing stream

var matrix = QRCode
    .CreateMail("[email protected]")
    .WithVersion(10)
    .ToMatrix();

Each payload generator would be able to define its own options, and each renderer would be able to define their own. We just establish some naming conventions for consistency and so on. We can have all payload generators implement an interface that supports WithQuietZones (and similar), while all renderers that return byte streams can implement an interface that support writing to arrays, base64, streams or files, without duplication of code. So the Windows.Drawing renderer may have a special method called ToBitmap() but would also support the byte-stream interface to support ToArray(), ToStream(), etc.

This code design also allows for good trimming support internally so that unused features are not present within trimmed applications. For instance, within the Windows.Drawing renderer, icon support can be trimmed out so long as WithIcon() is not called. Similarly, support for ToBase64() would be trimmed out if not referenced by the caller.

I would also suggest that we do not try to support too many different usage forms. I support other projects where there is multiple ways to do every feature, and it ends up where there is inconsistency with the supported features across the codebase simply because someone forgot to add an extra overload here or another extension method there. If we come up with a good solution, let's run with it.

[csturm83]

I intentionally kept my initial proposed design close to the current design for at least a couple reasons.

It keeps the process of rendering a QR code to basically the same 3 step process, which would hopefully ease transition from v1. If you squint hard enough the first 'traditional' example above is what folks are currently accustomed to, with new succinct semantics (IMO).

It also makes it a fairly straightforward process of adapting the existing code to fit the new abstractions. It should be mostly a mechanical process of dropping in the existing implementations into the new public API with maybe a few minor tweaks here and there to clean things up further.

The fluent variations were a natural side effect of reducing the 3 step process down to a single step reusable 'one-shot' method implementation.

All that said, I think it's at least worth exploring or experimenting with expanding the fluent API as suggested. My first thought would be that it could maybe be layered on top of the base abstraction. Not all use cases support trimming, so making it opt in might be beneficial. Maybe have its own package like QRCoder2.FluentExtensions. I'm thinking along the lines of what's been done with LINQ.

[codebude]

First of all, thank you for your ideas. I find both concepts exciting and welcome the change in the API towards a simpler and more logical design.

Some suggested semantic changes:

• AbstractQrCode --> Renderer
• ***QrCode --> ***Renderer
• QRCodeGenerator.CreateQrCode --> QRCode.Create
• GetGraphic --> Render

I think the renaming to ***Renderer and Render() is very good. This describes the function of the classes much better. I would be careful with renaming QRCodeGenerator.CreateQrCode to QRCode.Create, otherwise we will have a QRCode in v2 that also existed in v1, but which does something completely different. I would not assign new functions to existing names from v1. Otherwise these "false friends" will certainly cause confusion during the migration.

I would also suggest that we do not try to support too many different usage forms. [...] If we come up with a good solution, let's run with it.

I subscribe to that. Re-design, even radically, yes, but please not too many different variants/options. This increases the error rate, the documentation effort and, in the end, certainly also the support effort. Please let's commit to one variant as straight-forwardly as possible.

It keeps the process of rendering a QR code to basically the same 3 step process, which would hopefully ease transition from v1.

I agree that this would certainly simplify the migration process, but I believe that we should prioritize best practices and a nice solution over simplicity during the migration. The v2 will be a new major release and, in my opinion, may also break with the status quo. When I talk about backwards compatibility from time to time, I am essentially talking about feature compatibility, not necessarily compatibility at API level.
In other words: I want to avoid users turning away because QRCoder v2, for example, simply can no longer generate payloads. But if we maintain the user stories/functionalities and still modify the API significantly, I'm fine with that.

I personally like the Fluent API approach shown by @Shane32 very much. Maybe we can combine this with @csturm83's suggestion (looking at the suggested settings). Something along those lines:

var renderedArray = QRCode
    .CreateUrl("http://google.com")
    .WithErrorCorrection(EccLevel.Q)
    .RenderWith<PngRenderer>()
    .WithSize(10)
    .WithIcon(bitmap, 15, 0)
    .ToByteArray();

//With settings
var cfg = RendererSettings(){
   Size = 10,
   Icon = Icon(bitmap, 15, 0),
};
var renderedArray2 = QRCode
    .CreateUrl("http://google.com")
    .WithErrorCorrection(EccLevel.Q)
    .RenderWith<PngRenderer>()
    .WithSettings(cfg)
    .ToByteArray();

[csturm83]

I would be careful with renaming QRCodeGenerator.CreateQrCode to QRCode.Create, otherwise we will have a QRCode in v2 that also existed in v1, but which does something completely different. I would not assign new functions to existing names from v1. Otherwise these "false friends" will certainly cause confusion during the migration.

var renderedArray = QRCode
.CreateUrl("http://google.com")
.WithErrorCorrection(EccLevel.Q)
.RenderWith()
.WithSize(10)
.WithIcon(bitmap, 15, 0)
.ToByteArray();

It's unclear to me how QRCode.Create is much different from QRCode.CreateUrl in the fluent example. Both are presumably static methods defined in the QRCode class.

The current QRCode class has no static methods defined in it. It's the original 'renderer'. Currently, you will only ever see a QRCode as an object/instance. The fact that QRCode as proposed would be completely static I think would help mitigate confusion during migration. Intellisense and documentation should be sufficient to avoid confusion also, in my opinion.

That said, I do fully understand and agree that pause should be taken when considering renaming. In this case I think the benefits of clarity and succinctness are worth it.

I agree that this would certainly simplify the migration process, but I believe that we should prioritize best practices and a nice solution over simplicity during the migration.

Could you please expound on what you consider best practices vs. 'nice solution' vs. simplicity?

I've generally understood simplicity to be a best practice (KISS principle) 😃.

[csturm83]

but please not too many different variants/options.

I feel like a full blown fluent implementation would do just that...

To reduce usage variants, the method chaining could easily be made an internal implementation detail (not a publicly available usage pattern). The public API surface could be reduced to:

// 'traditional' three-step rendering
QRCodeData qrData = QRCode.Create(payload, qrSettings);
TestRenderer renderer = new TestRenderer(qrData);
byte[] renderedt3 = renderer.Render(renderSettings);

// one-shot rendering
byte[] rendered1 = TestRenderer.Render(payload, qrSettings, renderSettings);

This is essentially a minimal cleaned up version of what we have today. The static one-shot Render method is equivalent to QRCodeHelper static class methods (implemented in a way that reduces code duplication).

[Shane32]

I don't care for that design. It would make it even harder (if not impossible) to have effective trimming, and it does not abstract all of the details that people typically don't care about (e.g. ECI mode) from the typical code path. It also doesn't help provide multiple output types (memory stream, existing stream, byte array, base64).

I'm fine making the Create methods have all the common parameters, rather than a bunch of stuff like Create(email).WithSubject(subject).WithBody(body), but with a fluent API, we can separate optional or rarely used features (WithMailEncoding(blahblah)), while still allowing them to be easily found.

[Shane32]

I'm also fine if it makes sense for some payloads to have a settings class to configure them. For instance:

QRCode.CreateRussianPaymentOrder(
    new() {
        Name = "Henry Jones",
        BankName = "Test Test",
    })
    .RenderAsPng(....)

The renderers should be designed similarly; for instance, the Art renderer could look like this:

.RenderWithArt(20, Color.Black, Color.White) // some typical properties everyone uses, as overloads or optional arguments
.WithFinderImage(finderImage) // optional features here (that may get trimmed out)
.WithBackgroundImage(backgroundImage)
.WithLogo(logoImage, 20, true)
.ToArray()

Compared to right now the Art renderer's method signature looks like this:

public Bitmap GetGraphic(
    int pixelsPerModule, Color darkColor, Color lightColor, 
    Color backgroundColor, Bitmap? backgroundImage = null, 
    double pixelSizeFactor = 1, bool drawQuietZones = true, 
    QuietZoneStyle quietZoneRenderingStyle = QuietZoneStyle.Dotted, 
    BackgroundImageStyle backgroundImageStyle = BackgroundImageStyle.DataAreaOnly, 
    Bitmap? finderPatternImage = null)

Finally, if everything are parameters, the only way to extend functionality without breaking the syntax is a new overload. Since the Art renderer doesn't support logos, we can't add them without adding an overload with even more arguments. Eventually you end up with many overloads to prevent breaking changes. With a fluent API, all that is done internally; just add another .WithSomething() and you can add the feature.

[codebude]

Could you please expound on what you consider best practices vs. 'nice solution' vs. simplicity?

I've generally understood simplicity to be a best practice (KISS principle) 😃.

Ok, I may not have expressed myself well enough. I meant to say that the ease of migration from v1 to v2 does not necessarily correlate with the ease of implementation of v2. And in case of a decision I would rather prefer an easy to use v2 than to keep the migration effort low.

Example: If you wanted to keep the migration as simple as possible, that would mean no API changes in v2. However, we are currently discussing how to make the API of v2 better/simpler. If an improved API in v2 requires an increased (=less simple) migration effort, then I would accept that.

[csturm83]

I don't care for that design.

Duly noted 😄 . I think at least some consumers (within the millions of downloads of QRCoder) might appreciate or care for a design that includes a usage pattern that is close to the current version. It doesn't have to be the exact design I shared, but I think removing a familiar usage pattern may be throwing the baby out with the bath water to some degree. I understand the appeal of a Fluent API. I think it has a place in v2. However, it does come with tradeoffs as well. For one, it's a lot of additional public API to design, document and maintain.

It would make it even harder (if not impossible) to have effective trimming

I would like to better understand how you made this assessment. I am admittedly relatively new to trimming. It seems like a very important use case for you 😄. Are there some resources you could share related to design best practices as it relates to trimming? Or was there some other means for your analysis? Did you measure somehow? If so, could you please share?

it does not abstract all of the details that people typically don't care about (e.g. ECI mode) from the typical code path

I may be misunderstanding this statement, but the existing overloads for QR creation from QRCodeGenerator (renamed to Create) would still be present and don't require explicitly specifying ECI mode. There was that whole discussion about Payload defaults as I recall...

It also doesn't help provide multiple output types (memory stream, existing stream, byte array, base64).

I feel like a lot of that (if not all of that) could be implemented as extension methods over byte[]. I'm not sure how that's inherently exclusive to a Fluent design 😄 .

Finally, if everything are parameters, the only way to extend functionality without breaking the syntax is a new overload. Since the Art renderer doesn't support logos, we can't add them without adding an overload with even more arguments. Eventually you end up with many overloads to prevent breaking changes. With a fluent API, all that is done internally; just add another .WithSomething() and you can add the feature.

This is where the design I proposed deviates the most from the current way of doing things. Renderer settings can vary quite a bit, and can get to be extensive, as you've noted 😄 . Including the overload Render(ArtRenderSettings settings) allows for adding additional settings (properties) to the settings object (setting a default value if optional) - without needing to update the overload signature.

[Shane32]

Trimming ... use cases

I believe trimming is going to become exceedingly important over the next couple years. We have seen the progression of NativeAOT and trimming within .NET 6 through .NET 8, where design changes have been made within Microsoft's infrastructure specifically to enable trimming support and reductions in the size of trimmed applications. Some use cases for NativeAOT and/or trimming are serverless architecture, where startup speed is critical, and Blazor/webassembly, where size is critical. It is also likely to be useful for mobile applications, again where a fast startup speed is desired. ASP.NET within .NET 8.0 now officially supports NativeAOT/trimming for web API applications. This is a huge enhancement for IOT devices, and is from my perspective, the main feature added to .NET 8.

See here, ASP.NET Core with NativeAOT's performance:

• https://learn.microsoft.com/en-us/aspnet/core/fundamentals/native-aot?view=aspnetcore-8.0

Take a look at EF Core 9's roadmap for NativeAOT, demonstrating MS's effort towards NativeAOT:

• [9.0] NativeAOT work dotnet/efcore#29754

And Sqlite trimming is supported with .NET 8.0:

• NativeAOT/trimming compatibility for Microsoft.Data.Sqlite dotnet/efcore#29725

The first feature listed for 'what's new' in .NET 9.0 mentions trimming:

• The .NET 9 runtime includes a new attribute model for feature switches with trimming support.

• https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-9/overview

Trimming ... design

I'm not sure about references, but I can tell you how it works. Trimming simply examines all code paths reachable by the executable and removes all of the other code paths. So take for instance this code:

public byte[] GenerateQrCode(int size) => GenerateQrCode(new() { Size = size });

public byte[] GenerateQrCode(MySettings settings)
{
    // generate the code
    if (settings.BackgroundImage != null)
    {
        // apply the background image
    }
    // return the generated image
}

The compiler must include all of the code for applying background images no matter whether background images are needed or not, even if settings.BackgroundImage is always null. Now consider this code:

public byte[] GenerateQrCode(int size) => GenerateQrCodeInternal(size, null);

public byte[] GenerateQrCode(int size, Bitmap backgroundImage)
    => GenerateQrCodeInternal(size, bimap => {
        // apply the background image
    });

private byte[] GenerateQrCodeInternal(int size, Action<Bitmap>? action)
{
    // generate qr code
    // run the action if present
    // return the image
}

Here we can see that all support for background images are completely removed from the compiled exe so long as the proper overload is called.

Here is a similar example with a fluent API design:

public class MyQrCodeGenerator
{
    private int _size;
    private Action<Bitmap>? _backgroundImageTransformer;

    public MyQrCodeGenerator(int size) => _size = size;

    public MyQrCodeGenerator WithBackgroundImage(Bitmap image, double sizePercentage)
        => _backgroundImageTransformer = bmp => {
            // apply the image over bmp in the correct spot
        };

    public byte[] ToArray()
    {
        // generate the bitmap
        _backgroundImageTransformer?.Invoke(image);
        // convert to byte and return
    }
}

You can do the same thing with interfaces also instead of delegates, but compiled they are implemented pretty similarly.

In short, a fluent API design, when properly implemented, can easily trim all parts of the library that is not necessary for execution. And of course many parts are simply settings that have defaults, and as such would not take advantage of trimming, like the size (pixels per module). But for the Art renderer, there are many different options which could be trimmed away.

Perhaps in v2 the QrCode and ArtQrCode becomes the same class with various options accessible by a easy-to-use fluent API. This won't hurt any trimmed applications, as all of the unused features are trimmed out.

I feel like a lot of that (if not all of that) could be implemented as extension methods over byte[].

I very much dislike libraries that provide extension methods for base .NET types. For instance, in the GraphQL.NET library that I support, there are a slew of extensions for Type 👎 . This now means that any unrelated code in my application that deals with Type now includes an extension method called GetNamedType() which has nothing to do with my code.

If we are not providing the extension methods that target QRCoder interfaces, then I say we should not include them at all (vs byte[] or Stream extensions). I would normally recommend that the base renderers always render to an existing Stream as their base function, but considering the design of many of the existing renderers, it seemed more logical to render to a new MemoryStream as their base function (as shown in #558 ). This is because (1) many of the renderers use MemoryStream already to create the byte array, and so this eliminates unnecessary copying of the data to a new byte array, (2) for renderers that write directly to a byte array, it is a very low cost to wrap it in a MemoryStream, which does not duplicate the data, and (3) it is easy to extract a byte[] or ArraySegment from a MemoryStream without unnecessary copying of the data (unless it is necessary).

[Shane32]

Renderer settings can vary quite a bit, and can get to be extensive, as you've noted 😄 . Including the overload Render(ArtRenderSettings settings) allows for adding additional settings (properties) to the settings object (setting a default value if optional) - without needing to update the overload signature

I certainly understand the logic here. If it were not for trimming, I would probably agree, at least for the Art renderer which has a myriad of options. I do think the fluent design should be relatively easy to use. Also, we can do a mix, like use .WithBackgroundImage(BackgroundImageSettings settings) or .WithBackgroundImage(Action<BackgroundImageSettings> configureSettings). This would still allow for trimming the entire background image capability out while still allowing an easy way to extend functionality within the method.

[Shane32]

I think at least some consumers (within the millions of downloads of QRCoder) might appreciate or care for a design that includes a usage pattern that is close to the current version.

Well, I agree there too. This is just a little bit at odds with our other goals I think. (New APIs, refactoring to new namespaces, etc.)

I personally would release v2 with the fluent API and mark all old classes obsolete, leaving it all present in the codebase and >95% backwards compatible. All users can then immediately upgrade to v2, with friendly comments in their build warnings telling them how to rewrite their code. Or they can ignore the warnings until they have time to work on it. And if there are interim libraries (like my own Shane32.EasyPDF) that use QRCoder, those libraries have time to upgrade without immediately breaking when an end user upgrades to v2. Then I'd release v3 later on (maybe a year or two later) without the obsolete methods. It would be relatively easy to start by having the new fluent API call the old code, as shown in #558 , and we could transition the actual code from the old classes to the new classes (having the old classes call the new fluent API) anytime during v2. We could even write a .NET Analyzer to rewrite user's old code into fluent API, if we wanted to. (I don't want to myself, but another maintainer did so for migrating GraphQL.NET code into a fluent API, and it works great.)

But this is certainly debatable; other users often have told me if it was them, they'd just remove the obsolete methods. But I try to support older users as long as possible, and especially where other libraries may depend on this library. As it relates to my own Shane32.EasyPDF library, now it means I need to support two versions of EasyPDF, one referencing QRCoder 1.x and another referencing QRCoder 2.x, for a time until all end users of my library can upgrade to QRCoder v2. (I'm really talking about my own company's projects here, as I doubt anyone else has used my library.)

Regardless, it does not sound like @codebude wishes to go down that route, which is okay with me, just not what I'd do.