claudep / swiss-qr-bill

A Python library to generate Swiss QR-bill payment slips
MIT License
98 stars 39 forks source link

line break in additional_information isnt shown and breaks many scanners #86

Closed dschmide closed 2 years ago

dschmide commented 2 years ago

Providing a line break character "\n" in the string for the additional_information parameter will cause problems with many scanners.

steps to reproduce:

QRBill(additional_information= f"Hello \n Line break")

The line break will not be visible in the human-readable text field, and will cause an error in the encoded bytes data of the qr code, which many scanners will be unable to decode

Even though some scanners can properly decode the resulting bytes, I do suggest sanitizing line breaks from the provided string, because the line break itself isnt properly displayed anyway, and this might cause many issues for users trying to use multiple lines in this section.

claudep commented 2 years ago

Thanks for the report. Do you intend to work on a patch?

dschmide commented 2 years ago

No, I just removed my line breaks in the inputs. The fix for me is simple enough, but it's quite hard to even realize what the problem is, because proprietary scanners that I don't have access to might be having a problem. (ZKB, Postfinance, etc...)

I think a simple fix could be implemented on line 361 or 407 in bill.py by adding additional_info.replace("\n", "") or something. You better look into this yourself and get to the bottom of why the line breaks a) aren't displayed and b) produce an error in the bytecode of the QR code.

result for f"Hello \nLine break \n\n\r\ntest"

linebreaktest-visual screenshot-linebreaktest

(see visually missing line break, QR code can't be decoded by many apps)

claudep commented 2 years ago

Could you please have a look at the proposed fix (#87)?

dschmide commented 2 years ago

I think that solves the issue with the bytes data in the qr code but I dont see why it should change anything about the visual linebreaks not showing. I'm fine with this, but I guess the commit message isnt technically correct - they still won't show.

I'll checkout this version and test it tomorrow. Thank you for looking into this!

claudep commented 2 years ago

The visual fix should be done through for text in line.splitlines(): in wrap_infos. We'll see…

dschmide commented 2 years ago

You are right, this also solves the line breaks visually not showing up. They do now, and line breaks are replaced by whitespace in the bytecode. I would use this, its a clear improvement. Thank you very much

fixed line breaks

replaced line breaks

small comment: I actually tested this with multiple line breaks (f"Hello \nLine break \n\n\r\ntest"), and I only got the single, normal line break. If people do want to visually separate information with a double line break, this is not gonna work. However, I would argue this is negible or even better this way and could be called intended behaviour. It's up to you. I also don't think adding tripple whitespace into the bytecode is really necessary.

claudep commented 2 years ago

I guess we'll stick with the current behavior for now. I struggle to find a real use case for adding blank lines inside content.