Skip to content

Avoid disabling all downloads on high res disable#1525

Merged
demiankatz merged 3 commits intoUniversalViewer:devfrom
sarangj:download_bug
Aug 16, 2025
Merged

Avoid disabling all downloads on high res disable#1525
demiankatz merged 3 commits intoUniversalViewer:devfrom
sarangj:download_bug

Conversation

@sarangj
Copy link
Copy Markdown
Contributor

@sarangj sarangj commented Aug 15, 2025

#1526

Description of what you did:

When disabling high res downloads in openseadragon, all downloads get disabled due to how these cases are grouped - instead, move the high res check up and allow fallthrough if it is enabled.

To test, I first repro'd the bug I'm seeing locally with the provided npm example. I used this manifest: https://qa-api-collections.nypl.org/manifests/510d47e3-2b96-a3d9-e040-e00a18064a99 (so that it would have canvas renderings for download). On the dev branch, if you set high res to true, but everything else as false, it works as expected:
Screen Shot 2025-08-15 at 3 21 22 PM

However, if you flip high res to false, everything disappears:
Screen Shot 2025-08-15 at 3 21 49 PM

After my change, setting high res to false still lets the manifest provided downloads show up:
Screen Shot 2025-08-15 at 3 22 56 PM

When disabling high res downloads in openseadragon, *all* downloads get
disabled due to how these cases are grouped - instead, move the high res
check up and allow fallthrough if it is enabled.
@vercel
Copy link
Copy Markdown

vercel bot commented Aug 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
universalviewer Ready Ready Preview Comment Aug 16, 2025 3:02pm

demiankatz
demiankatz previously approved these changes Aug 16, 2025
Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for catching this, @sarangj; your fix makes sense to me! Looks like this bug was introduced by commit 7d2b543 a couple of years ago.

@demiankatz
Copy link
Copy Markdown
Contributor

Prior to merging, I ran npm run fix to apply automatic style fixes, and I added a comment just to make it explicit that we're intentionally allowing the logic to drop from one case into another. Thanks again for the fix!

@demiankatz demiankatz merged commit 4eb49e3 into UniversalViewer:dev Aug 16, 2025
4 checks passed
@sarangj sarangj deleted the download_bug branch August 16, 2025 19:53
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