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

Print QR code to terminal #524

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Conversation

aliemjay
Copy link
Contributor

@aliemjay aliemjay commented May 8, 2021

Followup for #341

Based on #500

Closes #45

@aliemjay aliemjay force-pushed the qrcode-terminal branch from d7ce0c3 to 9b8963e Compare May 8, 2021 15:26
@aliemjay aliemjay force-pushed the qrcode-terminal branch 2 times, most recently from af4b75f to 7853a9d Compare August 30, 2021 04:55
Cargo.toml Outdated Show resolved Hide resolved
@aliemjay aliemjay force-pushed the qrcode-terminal branch 2 times, most recently from 889e869 to 05ddf23 Compare August 31, 2021 12:21
@aliemjay aliemjay requested a review from svenstaro August 31, 2021 12:26
Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

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

Pretty clean but somehow it doesn't scan for me.

src/main.rs Outdated
{
match QrCode::encode_text(&url, QrCodeEcc::Low) {
Ok(qr) => {
println!("QR code for {} :", Color::Green.paint(url).bold());
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
println!("QR code for {} :", Color::Green.paint(url).bold());
println!("QR code for {}:", Color::Green.paint(url).bold());

src/main.rs Outdated
// Prints the given QrCode object to the console.
fn print_qr(qr: &QrCode) {
let border: i32 = 4;
for y in (-border..qr.size() + border).step_by(2) {
Copy link
Owner

@svenstaro svenstaro Aug 31, 2021

Choose a reason for hiding this comment

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

Can you print a two character-wide white border around these? This might fix the code not scanning properly on black terminals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand this properly~ I mean the content is already white. If doesn't scan then the problem more probably is with the rendering code print_qr because I modified it from https://github.com/nayuki/QR-Code-generator/blob/master/rust/examples/qrcodegen-demo.rs to show smaller code. But I have tested it locally and it scans well.

Can you try another scanner to pinpoint the cause here?

Copy link
Owner

@svenstaro svenstaro Aug 31, 2021

Choose a reason for hiding this comment

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

I tested it and it works on my CLI if the colors are reversed (and with a border). If you want to try it:
echo "lolomg" | qrencode -t UTF8 works but echo "lolomg" | qrencode -t UTF8i doesn't.

Suggestion, since we have lots of horizontal space anyway: Just put the inversed and bordered version right next to the other one. This way, we'll make sure it works for everybody.

src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated
// Prints the given QrCode object to the console.
fn print_qr(qr: &QrCode) {
let border: i32 = 4;
for y in (-border..qr.size() + border).step_by(2) {
Copy link
Owner

@svenstaro svenstaro Aug 31, 2021

Choose a reason for hiding this comment

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

I tested it and it works on my CLI if the colors are reversed (and with a border). If you want to try it:
echo "lolomg" | qrencode -t UTF8 works but echo "lolomg" | qrencode -t UTF8i doesn't.

Suggestion, since we have lots of horizontal space anyway: Just put the inversed and bordered version right next to the other one. This way, we'll make sure it works for everybody.

@aliemjay aliemjay requested a review from svenstaro August 31, 2021 14:36
@svenstaro svenstaro merged commit 10d0fb6 into svenstaro:master Aug 31, 2021
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.

How about a QR-code in the terminal?
2 participants