[WIP/QRCoder2] Shall we re-think the Payload Generator?

[codebude]

Note: This issue is part of the planning of version 2 of the QRCoder. The meta-issue can be found here. These comments have been copied into this issue here, with the comments marked as such. If comments on the topic of this issue already exist in the meta thread, they have been copied here, naming the authors.

Topic

In the course of the original discussions in the meta-issue, the topic of what the future of the payload generator should look like came up. In particular, the following questions were raised:

• In which package (core or a separate one) should the payload generator be included?
• Should the interface be revised (especially with regard to the overridden ToString method)?
• Does the payload generator need a settings class?

[codebude]

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

• Version (auto or specific size)
• ECC level (always specified)
• Eci mode (auto or specific mode)
• Content, as a string

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.

^{Originally posted by @Shane32 here.}

[codebude]

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:

• Girocode': requires ECC level 'M' (see)
• 'SwissQrCode': requires ECC level 'M' and 'EciMode.Utf8' (see)

I opened an issue for this techincal debt:

• Some Payload generators miss overriding EciMode and EccLevel #525

[codebude]

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?

^{Originally posted by @Shane32 here.}

---

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?
_@Shane32

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.

[codebude]

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

^{Originally posted by @Shane32 here.}

[codebude]

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

^{Originally posted by @csturm83 here.}

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.

^{Originally posted by @csturm83 here.}

[codebude]

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.

^{Originally posted by @Shane32 here.}

[codebude]

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

^{Originally posted by @csturm83 here.}