lincolnloop / python-qrcode

Python QR Code image generator
https://pypi.python.org/pypi/qrcode
Other
4.34k stars 664 forks source link

Various changes/improvements to text-based QR codes #257

Closed Scripter17 closed 1 year ago

Scripter17 commented 2 years ago

NOTE: THERE MAY BE PYTHON 2 COMPATIBILITY ISSUES. I HAVEN'T CHECKED.

This started as a response to #252 but quickly turned into me adding several QOL changes to how text-based QR codes are made

Admittedly the code in this pull request is a fair bit jankier than I'd like, but I've been procrastinating on this for at a week or two now and I need to submit something

SmileyChris commented 2 years ago

Thanks for the submission! I totally understand procrastinating, so good job on just getting something committed. I haven't taken a deep dive into the code, but here are some initial observations:

This started as a response to #252 but quickly turned into me adding several QOL changes to how text-based QR codes are made

  • --ascii no longer inverts the colorscheme of the output
  • --invert was added to keep this behavior

Unless it's absolutely necessary, we should try to keep backwards compatibility. A new option --no-invert might be a better way to handle this.

  • --tty was added to force a black and white colorscheme. Omitting this option disables all ANSI codes
  • --raw was added to write character codes directly to sys.stdout.buffer instead of converting to unicode then printing

Outputting character codes directly is a better core way of handling the text output anyway. Having an underlying formatter the same as all the other ones seems logical, and something I have just never got around to. I think this is where we should head rather than another option.

  • In addition to colorama, os.system("color") is run on windows to make ANSI codes work with sys.stdout.buffer This should definitely be a separate issue, and something I can just merge straight away.

  • --fg and --bg were added to allow customizing the QR code colorscheme (admittedly not that useful)

  • A chart at the bottom of the --help message was added showing the colors for --fg and --bg

These seem a bit superfluous. Let's not add options just for the sake of it (again, this would be fine as an option for a text formatter).

  • --border was added to customize the width of the border around the QR code
  • The bottom border of QR codes now no longer has an additional filled pixel/half-character (this may break if there's any even-height QR code formats) I'd have to check this, but I'd want to ensure that cutting off this last line doesn't mean you end up with less than the required border.

Admittedly the code in this pull request is a fair bit jankier than I'd like, but I've been procrastinating on this for at a week or two now and I need to submit something

Split out that quick windows color fix to a separate issue, and then let's work on making this formatter based -- that'd be awesome.

Scripter17 commented 2 years ago

Unless it's absolutely necessary, we should try to keep backwards compatibility. A new option --no-invert might be a better way to handle this

Unfortunately I have to agree. I maintain that making --ascii do that was a stupid move but yeah, backwards compatibility is important. (Ignoring the part where making --ascii do that in the first place broke backwards compatibility)

Outputting character codes directly is a better core way of handling the text output anyway. Having an underlying formatter the same as all the other ones seems logical, and something I have just never got around to. I think this is where we should head rather than another option.

  1. This is what --raw looks like in windows image
  2. I only added it to make sp.run(["qr", stuff], shell=True, stdout=sp.PIPE) not crash (Yes. I know I can import qr as a module. There isn't a get_ascii function)
  3. I have no idea how formatters work. I've never used them

This should definitely be a separate issue, and something I can just merge straight away

Can do

These seem a bit superfluous. Let's not add options just for the sake of it (again, this would be fine as an option for a text formatter).

Yeah that's fair. --bg doesn't even look right with the fixed bottom border

I'd have to check this, but I'd want to ensure that cutting off this last line doesn't mean you end up with less than the required border.

Well the nice thing is that there's only finitely many QR code sizes so we can just enumerate through them all

I'm going to restart this from scratch for a cleaner commit history so stay tuned for when I finish that in 20 years