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

Switch to fast_qr lib #848

Merged
merged 20 commits into from
Sep 18, 2022
Merged

Switch to fast_qr lib #848

merged 20 commits into from
Sep 18, 2022

Conversation

cyqsimon
Copy link
Contributor

As mentioned in #846 , there is a clear mistake but I'm unsure how to fix.

@cyqsimon
Copy link
Contributor Author

Seems like VSCode auto-sorted Cargo.toml. Looks okay though.

src/listing.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
tests/qrcode.rs Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

Please rebase, sorry, I updated some deps. Won't update deps anymore until this is merged.

@cyqsimon cyqsimon requested a review from svenstaro July 22, 2022 17:24
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.

Great stuff! But the test stuff needs looking at I think.

src/renderer.rs Outdated Show resolved Hide resolved
tests/qrcode.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/consts.rs Outdated Show resolved Hide resolved
data/style.scss Outdated Show resolved Hide resolved
@cyqsimon
Copy link
Contributor Author

Also I've noticed an issue in the QR code dropdown menu when run with -q. Give me some time to fix it. Meanwhile I'll mark this PR as draft.

@cyqsimon cyqsimon marked this pull request as draft July 26, 2022 07:40
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.

The QR code doesn't actually show up for me in the browser currently:
image

data/style.scss Outdated Show resolved Hide resolved
data/style.scss Outdated Show resolved Hide resolved
data/style.scss Outdated Show resolved Hide resolved
tests/qrcode.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

Hey @cyqsimon, I'd like to cut a new release of miniserve with this in it after I merge the other PRs soon. Do you think you can finish this one up in the coming days? No problem if it's not in the release, just curious.

@cyqsimon
Copy link
Contributor Author

@svenstaro please go ahead with the release. Life got in the way for me during this past week and I am unable to squeeze out extra time. Sorry about that. I'll try to work on this PR this weekend.

@svenstaro
Copy link
Owner

No worries!

@svenstaro svenstaro mentioned this pull request Aug 15, 2022
@cyqsimon cyqsimon mentioned this pull request Aug 22, 2022
@cyqsimon
Copy link
Contributor Author

Rebased.

@cyqsimon
Copy link
Contributor Author

Still working on fixing the QR code in the drop-down menu. The previous implementation seems rather jerry-rigged. I'd like to spend some time to do it properly, if I can say so.

Cargo.toml Outdated Show resolved Hide resolved
@cyqsimon cyqsimon changed the title Switch to qrcode lib Switch to fast_qr lib Sep 1, 2022
@cyqsimon
Copy link
Contributor Author

cyqsimon commented Sep 1, 2022

Finally I have everything the way I like it. Sorry for the long long wait.

Previously the QR code in the webpage works by having the client send an extra request to the server with the qrcode param set to the URL, which was very ugly IMO. Now the QR code is generated at once with the rest of the page. The URL is worked out entirely using the info from the request.

Also thanks for the fast_qr recommendation. It is indeed quite a bit easier to work with than the qrcode crate. In particular, the ability to set the margin size when building the SVG is very nice. There is however one downside: fast_qr is much more restrictive than qrcode when it comes to generating ASCII art. There is no customising the characters used for background/foreground, which makes the previously supported feature of printing an inverted QR code side-by-side basically impossible. That being said, I don't think it's such a deal-breaker, so altogether the benefits outweigh the downsides.

P.s. I did notice some weird artefacting in VScode's integrated terminal. It seems like the background colour was not reset properly by fast_qr:
flameshot-20220901-203755
... but it's not present in my other terminal emulators (Alacritty, Konsole, Cool Retro Term). Please help me try it out on other terminal emulators if possible.

@cyqsimon cyqsimon marked this pull request as ready for review September 1, 2022 12:44
@cyqsimon
Copy link
Contributor Author

cyqsimon commented Sep 1, 2022

And finally I think I need some help on writing a test for the webpage QR code. If I understand correctly, the fixtures crate should be used for this test, but I'm not familiar with the crate at all.

@cyqsimon cyqsimon requested a review from svenstaro September 1, 2022 12:47
tests/qrcode.rs Outdated Show resolved Hide resolved
@cyqsimon cyqsimon requested a review from svenstaro September 15, 2022 19:13
@svenstaro svenstaro merged commit 9b4be68 into svenstaro:master Sep 18, 2022
@svenstaro
Copy link
Owner

Thanks, this is great!

@cyqsimon cyqsimon deleted the qrcode branch September 19, 2022 16:24
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