Skip to content

Conversation

@vaclavHala
Copy link
Contributor

Hi, I'd like to be able to click links inside one PDF document pointing at other PDF documents also available locally and have the other doc be open in new preview inside vscode.

Recording 2025-03-03 at 18 01 24

This PR implements this, though in a very rough, proof-of-concept way - the needed patches are applied directly into the webpacked pdf.js sources.
I expect you may not like this approach as it makes updating to new pdf.js in the future harder. If you like the new capability and have some idea how to better integrate it, let me know and I'll be happy to implement this "for real"

These are the two PDFs used in the demo gif above if you want to try yourself locally (just place them into same folder so the relative link works):
docA.pdf
docB.pdf

await window.PDFViewerApplication.open(config);
const [,hash] = config.url.split("#")
if(hash){
PDFViewerApplication.pdfLinkService.setHash(decodeURIComponent(hash))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this part is basically what was suggested here for the other vscode PDF viewer - it allows opening into selected location inside PDF document by the #fragment of the input url


const settings = {
url: `${webview.asWebviewUri(document.uri)}`,
docBaseUrl: `${webview.asWebviewUri(document.uri)}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is required so pdf.js is able to create proper url in the annotations layer when resolving relative file links

In particular this is needed so the data.url is available here

@jrandolf
Copy link
Contributor

This looks good! I suggest to do the following.

  1. Create a copy of the files you patched write call them "{filename}_baseline.{ext}".
  2. In download_pdfjs, add some code that diffs the baseline file with the newly-downloaded file and fails if there is a difference.

Alternatively, ask ChatGPT for some solutions :)

@jrandolf
Copy link
Contributor

@vaclavHala
Copy link
Contributor Author

Hi, sorry for the slow response

The patch should auto apply in ...

Sure I can do that, maybe I should also rename that script (and places where it is called) though as it no longer just downloads? Maybe prepare_pdfjs.sh

Also, what do you want the development flow to be like? At the moment I separated the create/apply of pdf.js patches so the flow goes like this

// pdfjs releases and you want to update
tools/download_pdfjs.sh

// first try with current patch, if it cleanly applies you are done
tools/apply_patches.sh

// if there are some conflicts that need resolving you have to go fix the patches
tools/create_patches.sh // starts...
// you update the pdf.js as you need
// tools/create_patches.sh creates new .patch

git commit

But maybe you'd prefer it to be all in a single script which walks you through the whole thing (download => try to apply => exit or let you manually create the patches on conflict)?

@jrandolf
Copy link
Contributor

jrandolf commented Apr 15, 2025

Probably a single, well-documented script that calls the subscripts. It shouldn't commit though

@vaclavHala
Copy link
Contributor Author

I updated the scripts so its one interactive thing which walks you through the process now. It's described at the bottom of the README - if this is not the place you want the docs let me know and I can move it wherever

You can take it for a spin with the two new pdf.js versions - 5.0.375 applies cleanly with the current patch. 5.1.91 does not apply cleanly so you can check how the conflict resolution works using that

The bash-fu is a bit much for my taste, I'd prefer to have the tools scripts also be written in plain .js with @ts-check, but I don't know what is the rationale behind having it as .sh so did not want to change that here - maybe something to consider in the future

@jrandolf jrandolf merged commit 0dd02b7 into mathematic-inc:main Apr 16, 2025
@release-please release-please bot mentioned this pull request Apr 16, 2025
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