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

utf-8: initial support #42

Merged
merged 17 commits into from
Feb 11, 2020
Merged

utf-8: initial support #42

merged 17 commits into from
Feb 11, 2020

Conversation

cab404
Copy link
Contributor

@cab404 cab404 commented Feb 9, 2020

This adds support for rendering QR codes with Unicode symbols "▄▀█" (just like qrencode), greatly reducing size of qr codes in terminal, and providing better aspect ratio than current string renderer.

@coveralls
Copy link

coveralls commented Feb 9, 2020

Coverage Status

Coverage increased (+0.2%) to 93.658% when pulling f395cf2 on cab404:feature/utf-8 into c2c875f on kennytm:master.

@cab404 cab404 requested a review from kennytm February 9, 2020 13:07
@cab404
Copy link
Contributor Author

cab404 commented Feb 9, 2020

I am not sure whether it's okay to use u8 as a pixel. It may create confusion.

@kennytm
Copy link
Owner

kennytm commented Feb 9, 2020

@cab404 Thanks for the PR

I am not sure whether it's okay to use u8 as a pixel. It may create confusion.

Perhaps create a new type to reduce confusion (e.g. struct DenseUnicode1x2;).

@cab404
Copy link
Contributor Author

cab404 commented Feb 9, 2020

Perhaps create a new type to reduce confusion (e.g. struct DenseUnicode1x2;).

✔️

@cab404
Copy link
Contributor Author

cab404 commented Feb 9, 2020

This way it would actually be easier to add 2x3, if it exists.

@cab404
Copy link
Contributor Author

cab404 commented Feb 9, 2020

Not sure how to fix nighly fails :| @kennytm?

README.md Outdated Show resolved Hide resolved
Co-Authored-By: kennytm <[email protected]>
Copy link
Owner

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Please also add a test case for dark-on-light output.

Rest LGTM.

src/render/mod.rs Outdated Show resolved Hide resolved
src/render/utf8.rs Outdated Show resolved Hide resolved
src/render/utf8.rs Outdated Show resolved Hide resolved
src/render/utf8.rs Outdated Show resolved Hide resolved


fn new(width: u32, height: u32, dark_pixel: Unicode1x2, light_pixel: Unicode1x2) -> Self {
let a = vec![light_pixel.value(); (width * height) as usize];
Copy link
Owner

Choose a reason for hiding this comment

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

Add an assert! that (width+1)/2 == height

Copy link
Contributor Author

@cab404 cab404 Feb 10, 2020

Choose a reason for hiding this comment

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

But why? It works on non-rectangle canvas just fine :| I can make test cases for that.

Copy link
Owner

Choose a reason for hiding this comment

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

In other non-text canvas, the width and height can be independently scaled and result in rectangular modules. This is not the case for 1x2 since the width:height ration is fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and also my implementation doesn't store it on per-symbol basis :|

@kennytm kennytm merged commit 03e18d5 into kennytm:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants