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

Parse TOC from JSON and add PDF metadata #24

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kellda
Copy link

@kellda kellda commented Jun 9, 2023

Using the JSON description of the book allows easier and more reliable generation for the table of contents / pdf outline.

This PR also add the book title, author, and description to the PDF metadata.

@HollowMan6
Copy link
Owner

Thanks for your contribution! just had a quick glance at this PR, generating the ToC from JSON directly is an interesting idea. I want to check, do you see any clear difference in the ToC content between the original way and your way? Maybe we can just use your way as the default option if they are actually producing the same result.

@kellda
Copy link
Author

kellda commented Jun 9, 2023

Well on my document, the html-based one misses at least half of the TOC, which is why I did this in the first place. As such, I can't really compare. It should have the same result, thought.

For context, I use Unicode characters in titles, titles do not match the file names and I use msedge.exe on Windows to generate the PDF.

@HollowMan6
Copy link
Owner

HollowMan6 commented Jun 9, 2023

  1. output-your-pr-with-fixed-internal-links.pdf
  2. output-your-pr-without-fixed-internal-links.pdf
  3. output.pdf

Well on my document, the html-based one misses at least half of the TOC, which is why I did this in the first place.

Okay, now I know what's going on, you may be in the same situation as:
#18 (comment)

Since it's meaningless to generate a PDF file with broken internal links, the mdbook-pdf-outline is designed for my patched version of mdBook rust-lang/mdBook#1738 (and I don't think we should support the current unpatched mdBook version because of their broken nature) I have already mentioned this in README with related issues and I do hope people can check this before using this software! https://github.com/HollowMan6/mdbook-pdf#common-issues

Your PR doesn't work with my patched version of mdBook (see the first pdf file), as in that patch, I append a path id prefix to all the anchors. You can modify this PR to use the file path information in JSON and later I may consider replacing the HTML way with your method if this gets fixed.

Even though I can see that your PR works almost correctly with the unpatched mdBook, some are still problematic (See the second pdf file), and you may also want to handle these issues as well:

image

(The third one is the current HTML-based version) If you find any issues with the current version, please let me know.

@HollowMan6 HollowMan6 marked this pull request as draft June 9, 2023 11:40
@HollowMan6
Copy link
Owner

Please let me know when you are ready, thanks for your contribution again! (Since I've been working on this repository all by myself, you will become the 2nd one who has contributed to this repository if your PR gets merged 🙌 !)

@kellda
Copy link
Author

kellda commented Jun 9, 2023

Thanks for your time and review. Right now I won't have time to fix this further, and it currently works for me. I'll come back and fix this in one or two months. Don't hesitate to ping me then.

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.

None yet

2 participants