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

Add pic support #543

Closed
wants to merge 3 commits into from
Closed

Add pic support #543

wants to merge 3 commits into from

Conversation

afh
Copy link
Contributor

@afh afh commented Jul 9, 2024

Description

Add support for PIC output format, which was added to qrcode-rust 0.14.1 (released July 5th) with kennytm/qrcode-rust#65.

Unfortunately my Rust expertise is rather lacking, hence the crude implementation. I'd greatly appreciated advice and guidance on how to improve this PR. I'm dissatisfied with the if-else in order to handle the given module_size and dislike that therefore margin is ignored.

Trying to follow the same approach as to_svg and call module_dimensions on a previously created renderer:

pub fn to_pic(code: &QrCode, margin: u32, module_size: Option<u32>) -> String {
    let c = code.to_colors();
    let mut renderer = Renderer::<pic::Color>::new(&c, code.width(), margin);
    let mut result = String::new();
    if let Some(size) = module_size {
        renderer = renderer.module_dimensions(size, size);
    }
    renderer.build();
}

result in the following error for me:

error[E0308]: mismatched types
  --> src/encode.rs:60:20
   |
57 |     let mut renderer = Renderer::<pic::Color>::new(&c, code.width(), margin);
   |                        ----------------------------------------------------- expected due to this value
...
60 |         renderer = renderer.module_dimensions(size, size);
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Renderer<'_, Color>`, found `&mut Renderer<'_, ...>`
   |
   = note:         expected struct `Renderer<'_, _>`
           found mutable reference `&mut Renderer<'_, _>`

Hopefully I've been able to explain well, what I'd like to achieve and which errors I run into, if not please ask me to clarify 🙂

ℹ️ This PR includes the changes from #539 (pending review and approval) to update qrcode-rust to 0.14.1

Additional context

Pic is a [domain-specific] programming language by Brian Kernighan for specifying line diagrams.

Wikipedia

It is most commonly used in typesetting documents using implementations of roff, such as GNU roff, Plan 9 troff, or heirloom doctools to name a few.

Checklist

dependabot bot and others added 2 commits July 9, 2024 10:25
Bumps [qrcode](https://github.com/kennytm/qrcode-rust) from 0.14.0 to 0.14.1.
- [Release notes](https://github.com/kennytm/qrcode-rust/releases)
- [Commits](https://github.com/kennytm/qrcode-rust/commits/v0.14.1)

---
updated-dependencies:
- dependency-name: qrcode
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
src/cli.rs Outdated Show resolved Hide resolved
src/encode.rs Outdated Show resolved Hide resolved
Copy link
Owner

@sorairolake sorairolake left a comment

Choose a reason for hiding this comment

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

If possible, please do the following:

$ reuse annotate -c "<YOUR_NAME>" -y "<YEAR>" <CHANGED_FILES>

@sorairolake
Copy link
Owner

Merged as 01009da. Thanks!

@sorairolake sorairolake closed this Jul 9, 2024
@afh afh deleted the add-pic-support branch July 9, 2024 17:20
@afh
Copy link
Contributor Author

afh commented Jul 9, 2024

Thanks for reviewing, approving and merging the suggested changes in this PR, @sorairolake, very much appreciated! 😄

I look forward to the next release of qrtool and will keep a keen eye on the releases 👀

@sorairolake
Copy link
Owner

sorairolake commented Jul 10, 2024

@afh I generated a PDF file from PIC with the following (2bb143a):

qrtool encode -t pic "QR code" | groff -Tpdf -p -P-p3i,3i > output.pdf

generated PDF file

The result of converting this PDF file to a PNG image is as follows:

magick output.pdf output.png

generated PNG image

This seems like an incorrect result to me. How can I produce the correct result?

Environment

$ groff --version
GNU groff version 1.23.0
Copyright (C) 2022 Free Software Foundation, Inc.
GNU groff comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of groff and its subprograms
under the terms of the GNU General Public License.
For more information about these matters, see the file
named COPYING.

called subprograms:

GNU grops (groff) version 1.23.0
GNU troff (groff) version 1.23.0
$ lsb_release -sd
"Arch Linux"

@afh
Copy link
Contributor Author

afh commented Jul 10, 2024

Thanks for reaching out, @sorairolake.

TL;DR:

qrtool encode -t pic "QR code" \
  | awk 'BEGIN{print ".vs 0\n.po 0\n.PS"} END{print "scale=25.4*3\n.PE"} {print}' \
  | groff -Tpdf -p -P-p3i,3i \
  > output.pdf

To make the QR code fill the PDF page (as I think you intend to) the TL;DR example does the following:

  1. .vs 0 set the vertical spacing (line-height) to 0 so that the QR code starts at the very top of the page
  2. .po 0 set the page offset (left margin) to 0 so that the QR code starts at the left edge of the page
  3. .PS starts an "environment" for PIC code
  4. scale=25.4*3 set the scale of the image, so that the QR code fills the entire page
  5. .PE ends that "environment"

Given the nature of PIC (or my understanding of it) it isn't as much of a stand-alone graphics format, but rather something that is pre-processed and then embedded in groff or TeX documents.

What qrcode-rust generates is "just" the plain PIC code,1 without any surrounding "markup", i.e. the .PS and .PE, necessary to make it work in a document.
This allows to generate the PIC code for a QR code and use PICs copy instruction to include the PIC code in an existing PIC environment (for a complete example see encode_pic.roff from qrcode-rust).

From my experience it is easier/more convenient to have a document that uses copy in its PIC "environment" than having to pre-process the generated files using pic individually and source that using roffs .so macro, e.g.:

% qrtool encode -t pic "QR code" > qrcode.pic; groff -p document.roff

where document.roff contains:

.PS
# other document specific PIC code to "customize/annotate" the QR code
copy "qrcode.pic"
.PE

is easier/more flexible than:

% qrtool encode -t pic "QR code" | pic > qrcode.pic; groff document.roff

where document.roff contains:

.so qrcode.pic

What would be ideal, I guess, is a stand-alone mode, in which qrtool (or qrcode-rust) adds the surrounding .PS and .PE, which would allow users to decide which mode of operation they prefer.

Hopefully I've been able to explain well what the idea of how to use this is. Happy to hear feedback on where this maybe falls shorts or could be improved 🙂

What do you think, @sorairolake?


1 ℹ️ I am the author of the PRs (kennytm/qrcode-rust#65, kennytm/qrcode-rust#66) that add support for PIC to qrcode-rust and any short-comings in these are my faultresponsibility.

@sorairolake
Copy link
Owner

@afh I think it would be useful if the documentation of the qrcode crate explained how to generate a PDF file.

Based on your comments, I'll add to the README how to do something similar to the following:

qrencode -t PIC "qrencode PIC test" | groff -Tpdf -p -P-p3i,3i > qrcode.pdf

@afh
Copy link
Contributor Author

afh commented Jul 10, 2024

Sounds good, @sorairolake, do let me know if I can help with anything related 🙂

@sorairolake
Copy link
Owner

I didn't look closely at encode_pic.roff.

I think the output of the qrcode crate is fine as is (generates "just" the plain PIC code). However, I think this language is less well known than PNG and SVG. I don't think it's as easy to use as these image file formats (e.g., use the output of qrtool as the image). So, as mentioned above, it would be easier to understand if the documentation explained how to convert the PIC code to a PDF file or the like.

@sorairolake sorairolake mentioned this pull request Jul 10, 2024
2 tasks
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