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

pic: initial support #65

Merged
merged 1 commit into from
Jul 3, 2024
Merged

pic: initial support #65

merged 1 commit into from
Jul 3, 2024

Conversation

afh
Copy link
Contributor

@afh afh commented Jun 30, 2024

This PR adds initial support for rendering a QR-Code as PIC, so that the QR-Code can be included in documents typeset using roff (only tested with GNU roff 1.23.0 so far).

The newly added code in src/renders/pic.rs is based on src/renders/svg.rs and was adapted as needed.
The example for pic includes additional files to show a full example including roff source. Within those files example commands are given to show a complete pipeline.

I'm not too familiar with Rust and hence kindly request a scrutinizing review. 😅

@afh
Copy link
Contributor Author

afh commented Jul 1, 2024

ℹ️ The generated output is compatible to what has been proposed to libqrencode in fukuchi/libqrencode#208

README.md Outdated

Generates PIC output that renders as follows:

[![Output](examples/encode_pic.png)](examples/encode_pic.png)
Copy link
Owner

Choose a reason for hiding this comment

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

You should link to the text output of the PIC code, instead of the rendered PNG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be possible to have all the call to the p(…) macro on a single line separated by ;, would you prefer that?

Copy link
Owner

Choose a reason for hiding this comment

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

It would be possible to have all the call to the p(…) macro on a single line separated by ;, would you prefer that?

if it is all in one line, there is no improvement in terms of readability or file size, so no.

README.md Outdated Show resolved Hide resolved
src/render/pic.rs Outdated Show resolved Hide resolved
src/render/pic.rs Outdated Show resolved Hide resolved
src/render/pic.rs Outdated Show resolved Hide resolved
src/render/pic.rs Outdated Show resolved Hide resolved
@afh
Copy link
Contributor Author

afh commented Jul 1, 2024

Thanks for the review and the helpful comments, @kennytm, very much appreciated!

I've responded to your comments and will address all of them in a future change to this PR, once all comments have been resolved 🙂

@afh afh force-pushed the pic-init branch 5 times, most recently from 9acd1fd to cb26cf2 Compare July 2, 2024 05:35
@afh afh requested a review from kennytm July 2, 2024 05:36
@afh
Copy link
Contributor Author

afh commented Jul 2, 2024

Kindly requesting re-review, @kennytm. The current state of this PR and the PIC code it generates offers to most compatible output according to my tests. Hopefully I've addressed all of your other comments properly, if not please let me know 🙂

README.md Show resolved Hide resolved
src/render/pic.rs Outdated Show resolved Hide resolved
@afh afh force-pushed the pic-init branch 4 times, most recently from 50b46f7 to 63d9a6a Compare July 2, 2024 17:31
@afh afh requested a review from kennytm July 2, 2024 17:31
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.

Rest LGTM

#[doc(hidden)]
pub struct Canvas {
pic: String,
marker: PhantomData<Color>,
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
marker: PhantomData<Color>,

no need to include the PhantomData since it no longer needs to capture the unused generic argument.

@kennytm
Copy link
Owner

kennytm commented Jul 2, 2024

and please fix the rustfmt before committing, running cargo fmt should be enough.

@afh
Copy link
Contributor Author

afh commented Jul 2, 2024

Thank you for your patience and helpful advice on this, @kennytm, very much appreciated! 🙏

I've addressed your comments in the latest force-push to this PR.

@afh afh requested a review from kennytm July 2, 2024 18:11
README.md Outdated
fn main() {
let code = QrCode::new(b"01234567").unwrap();
let image = code
.render()
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
.render()
.render::<pic::Color>()

Comment on lines 47 to 50
r#"define p {{ box wid $3 ht $4 fill 1 with .nw at $1,-$2 }}"#,
"\n",
r#"box wid maxpswid ht maxpsht with .nw at 0,0"#,
"\n",
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
r#"define p {{ box wid $3 ht $4 fill 1 with .nw at $1,-$2 }}"#,
"\n",
r#"box wid maxpswid ht maxpsht with .nw at 0,0"#,
"\n",
"define p {{ box wid $3 ht $4 fill 1 with .nw at $1,-$2 }}\n",
"box wid maxpswid ht maxpsht with .nw at 0,0\n",

@afh
Copy link
Contributor Author

afh commented Jul 3, 2024

Thanks for taking another scrutinising look, suggesting helpful changes, and already approving this PR. I've updated this PR to include your suggestions 🙂

@kennytm kennytm merged commit 5a8fbdb into kennytm:master Jul 3, 2024
5 checks passed
@afh afh deleted the pic-init branch July 3, 2024 07:08
@afh
Copy link
Contributor Author

afh commented Jul 3, 2024

Thanks for your patience, guidance and help on getting this merged, kennytm, very much appreciated! 😃 For this new functionality to be useful for me a new release would be helpful, so that the qrcode-rust dependency in sorairolake/qrtool can be updated and a PR to add the necessary changes to support PIC output can created.

Are there any plans for a new release any time soon(ish) by any chance?

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.

2 participants