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

Page range generation fails due to structures #20

Closed
tristanr-cogapp opened this issue May 10, 2023 · 2 comments
Closed

Page range generation fails due to structures #20

tristanr-cogapp opened this issue May 10, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@tristanr-cogapp
Copy link

If a user requests a range of pages for a manifest that contains structures (a.k.a. table of contents for the generated PDFs) then PDF generation will fail if any on the canvases present in the structures are not also included in the requested page range.

The effect is that a "Could not find canvas with id XXX in manifest!" error is thrown in the _addOutline method, which causes PDF generation to fail.

Steps to reproduce

Visit the demo site and use the sample Book-with-TOC manifest
https://iiif.io/api/cookbook/recipe/0024-book-4-toc/manifest.json - if you select any page range other than 1-6 then it will fail, given that all six pages are referenced in the structures array.

@jonw-cogapp
Copy link

Sharing a potential solution for consideration, something along these lines:

In generator.ts, the _addOutline() method is expected to return an array that contains a pdfObject and a number. Update the types so that it can return an array or null, and instead of throwing inside _addOutline() return null. In the setup() method, we should be able to test if the return value from _addOutline() is null and use continue; to skip over it. We might also need to look at the recursive call for _addOutline().

My only concern might be that I'd like to better understand if this would effect the original intent of the commit which added the throw.

Either way I'm happy to open a PR to address this issue.

@jbaiter jbaiter self-assigned this May 23, 2023
@jbaiter jbaiter added the bug Something isn't working label May 23, 2023
@jbaiter
Copy link
Owner

jbaiter commented May 23, 2023

I just pushed a fix for this, thanks for reporting the issue @tristanr-cogapp and for the suggestions @jonw-cogapp!
I fixed it before passing in the parsed outline to the PDF generator, at that point we can already decide which ranges have no corresponding canvases and throw them out during the build of our TocItem[] list.

jbaiter added a commit that referenced this issue May 24, 2023
**Fixed:**
- pdiiif-lib: Selecting a custom range of canvases now also works when the
  Manifest has ranges defined, this would result in an error previously
  ([#20][issue-20])
- pdiiif-lib: Prevent inclusion of duplicate Table of Contents tree when converting
  from IIIFv2 manifests with a `top` range ([#21][issue-21])
- pdiiif-lib: PDFs from manifests that reference OCR that fails to fetch during
  converting now no longer result in a broken state ([#17][issue-17])

**Changed:**
- Removed `lodash` dependency in favor of EcmaScript builtins and to reduce bundle
  size
- pdiiif-web: SSL via `mkcert` is now fully optional for running the
  development server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants