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 support for v3 manifests #7

Merged
merged 7 commits into from
Sep 16, 2024
Merged

Add support for v3 manifests #7

merged 7 commits into from
Sep 16, 2024

Conversation

martimpassos
Copy link
Contributor

Hi, this adds support to v3 manifests by using iiif-builder to downgrade them. I added a custom prop to modify the image URL syntax (/full/full to /full/max) if necessary, there's probably a better way to propagate it all the way down to the Image objects though.

@martimpassos
Copy link
Contributor Author

martimpassos commented Sep 12, 2024

edited so isDowngraded is an environment variable, much cleaner this way.

I'm curious about your thoughts on allowing users to paste manifest links in the importer rather than downloading the JSON file. It could be a window where you can paste a comma separated list of URLs or repeatedly import manifest by manifest by submitting links without the window closing.

@inukshuk
Copy link
Member

Yes, pasting URLs is something we had in mind to add. Using a file is often a little annoying. The main reason we haven't done it so far is that we want to have a common dialog infrastructure for this. At the moment we're utilizing the OS dialogs (file picker etc.) and there is unfortunately no good cross-platform option for a dialog with an input field. Dialog input is useful elsewhere as well so we'll build these dialogs into Tropy at some point and expose them to plugins.

About the PR itself, that's an intriguing idea! It's also great to see that there seem to be standard IIIF tools available now for us to use. We can certainly consider updating the plugin this way, but I wonder if we shouldn't turn it around: i.e., use version 3 by default and upgrade version 2 manifests. What do you think?

@martimpassos
Copy link
Contributor Author

I had totally overestimated the complexity of doing it this way. Last commit supports v3 natively and upgrades v2 for backward compatibility.

@inukshuk
Copy link
Member

Nice!

Why do we need to differentiate between the URLs using the environment variable? Tropy can import images in parallel so using a global variable in combination with await/async is potentially dangerous.

@martimpassos
Copy link
Contributor Author

Passed isUpgraded as a parameter to the Resource constructor, do you recommend another approach?

@inukshuk
Copy link
Member

Looks good!

Have you tested this in Tropy already?

@martimpassos
Copy link
Contributor Author

Yes, I'm able to import the Northwestern manifest I added for tests and the Bodleian one, both together, one before the other and vice versa, etc.

@inukshuk inukshuk merged commit 7f5a543 into tropy:main Sep 16, 2024
@inukshuk
Copy link
Member

Great. So that means that the bundled iiif-builder loads fine in Tropy, that's good.

Do you want to add anything else or shall we tag a release as is?

@martimpassos
Copy link
Contributor Author

I think it is good to go, if you feel like testing it on your end or else just tag it

@inukshuk
Copy link
Member

Are you sure this was working? It even fails to parse the northwestern fixture for me?

@inukshuk inukshuk mentioned this pull request Sep 16, 2024
@inukshuk
Copy link
Member

I reverted the main branch for now and opened #8 separately. I added one commit, moving the upgrade into Manifest.parse but it's essentially still unchanged from the original PR. Parsing/expanding the Northwestern fixture fails however. Was this working for you?

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