Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Django QR Code to use Segno instead of qrcode / Pillow. #13

Merged
merged 10 commits into from
Sep 7, 2020

Conversation

heuer
Copy link
Contributor

@heuer heuer commented Aug 22, 2020

Removes the requirement "Pillow" since Segno creates PNG images without further dependencies
Support for other colors than black and white, each module type may have its own color.
Added support for Micro QR codes.
Removed the image.py module, not needed anymore.

The changes are not as extensive as the diff might suggest. These changes make #12 superfluous (use 'dark' instead of 'fill_color')

heuer added 6 commits August 22, 2020 22:01
Removes the requirement "Pillow" since Segno creates PNG images without further dependencies
Support for other colors than black and white, each module may have its own color.
Added support for Micro QR codes.
Removed the image.py module, not needed anymore.
Fixed <img src="..."> code examples (use HTML entities)
@heuer
Copy link
Contributor Author

heuer commented Aug 24, 2020

If this PR is accepted, I could add a page to the docs which lists the available module types, similiar to https://segno.readthedocs.io/en/latest/colorful-qrcodes.html#module-names and how to use them in the template tags

@heuer
Copy link
Contributor Author

heuer commented Aug 30, 2020

@philippe-docourt, any thoughts or improvements to this?

@philippe-docourt
Copy link
Member

philippe-docourt commented Sep 5, 2020

Hi @heuer, this sounds good!

The qrcode API and its dependency to Pillow is not really clean. Segno might be a good alternative.

One thing that is bothering me a litlle is the scale change for SVG format. This will be quite annoying for existing project.

Please give me a few days so that I can dive into the changes and submit some comments or suggestions before we move on with you proposal.

One question: qrcode supports "QR code optimization", do you have any plan for developing such feature in Segno?

@heuer
Copy link
Contributor Author

heuer commented Sep 5, 2020

Thanks for your feedback.

One thing that is bothering me a litlle is the scale change for SVG format.

I thought the size is identical. Are you referring to
5c21a57#diff-04c6e90faac2675aa89e2176d2eec7d8R68?

I just corrected a typo there. If you look into the code of python-qrcode, you'll see that is uses 1 mm as smallest unit, not 0.1 mm.

IMO the SVG output is identical (size-wise) to the current images, but I'll recheck it.

Please give me a few days so that I can dive into the changes and submit some comments or suggestions before we move on with you proposal.

No problem, thanks for considering it!

One question: qrcode supports "QR code optimization", do you have any plan for developing such feature in Segno?

Yes it is planned but I haven't found the time yet.
There is an open issue for it: heuer/segno#25

@heuer
Copy link
Contributor Author

heuer commented Sep 5, 2020

Here a comparison from your demo site. The size of the SVG output is identical

python-qrcode

pythonqrcode-svg

segno

segno-svg

@philippe-docourt
Copy link
Member

philippe-docourt commented Sep 6, 2020

I just corrected a typo there. If you look into the code of python-qrcode, you'll see that is uses 1 mm as smallest unit, not 0.1 mm.

Oh! Thanks for the fix. I can't believe I never realized that was completely wrong. So you're right, the size did not change :-)

Copy link
Member

@philippe-docourt philippe-docourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@philippe-docourt
Copy link
Member

@heuer, I'm merging the pull request right now in order to check that everything is working smoothly. I'm wondering about Python 3.6 compatibility. You mention Python 3.7+... is there any kind of trouble with Python 3.6?

@heuer
Copy link
Contributor Author

heuer commented Sep 7, 2020

@heuer, I'm merging the pull request right now in order to check that everything is working smoothly. I'm wondering about Python 3.6 compatibility. You mention Python 3.7+... is there any kind of trouble with Python 3.6?

You mean that I mention 2.7+ and 3.7+ in Segno? I dropped support for testing 3.6 but I am not aware of any problems with 3.6. It should even work with 3.5. But I don't test it against 3.5 and 3.6. That's why I'm on the safe side when I mention 3.7+. ;)

If you discover problems with 3.6 I'll fix them.

philippe-docourt added a commit that referenced this pull request Sep 7, 2020
Removes the requirement "Pillow" since Segno creates PNG images without further dependencies
Support for other colors than black and white, each module type may have its own color.
Added support for Micro QR codes.
Removed the image.py module, not needed anymore.
@philippe-docourt philippe-docourt merged commit 28401ed into dprog-philippe-docourt:master Sep 7, 2020
@philippe-docourt
Copy link
Member

Merge done!

I adjusted the test script in order to make sure that tests are run with all combination of Django (2.2, 3.0 and 3.6) and Python (3.6, 3.7 and 3.8) versions. All tests passed!

The demo site seems to run as expected.

Thanks @heuer for your contribution.

@heuer
Copy link
Contributor Author

heuer commented Sep 7, 2020

Thanks for switching to Segno! 👍

@philippe-docourt
Copy link
Member

Thanks for switching to Segno! +1

I look forward to adding support for some Segno features like Artistic QR codes :-)

@heuer
Copy link
Contributor Author

heuer commented Sep 7, 2020

Thanks for switching to Segno! +1

I look forward to adding support for some Segno features like Artistic QR codes :-)

Well, thanks for considering it but you'll introduce further dependencies like qrcode-artistic and Pillow then. IMO this shouldn't belong to this project / the core of this project. Maybe it's worth to create another project or make the dependencies optional. IMO a tool should do its job well but no longer.

philippe-docourt added a commit that referenced this pull request Sep 26, 2020
Added docs about the color keywords (pr #13)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants