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

Support for IIIF Presentation API 3.0 #224

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

Conversation

joesong168
Copy link

Based on sauterl's work. Passed test for following kind of annotation.

{
            "id": `${this.baseUrl}/c/3/${canvasUuid}/ap/c/a/${annoUuid}`,
            "type": "Annotation",
            "motivation": "supplementing",
            "body": {
                "type": "TextualBody",
                "value": comment,
                "format": "text/plain",
                "language": "zh-Hants"
            },
            "target": `${this.baseUrl}/c/3/${canvasUuid}#${fragmentSelector}`,
}

Although not sure what following code means in hasExternalResourceV3

Object.keys(anno.body).length === 1 

@jbaiter jbaiter self-requested a review November 26, 2021 15:05
@jbaiter jbaiter self-assigned this Nov 26, 2021
@joesong168 joesong168 marked this pull request as draft November 27, 2021 03:51
@joesong168 joesong168 marked this pull request as ready for review November 27, 2021 03:53
@jbaiter
Copy link
Member

jbaiter commented Dec 15, 2021

Thank you for the PR! And sorry it took so long to review :-(

A few things in addition to the line-comments:

  • Could you add a unit test for your code? There are some existing v2 tests in __tests__/lib/ocrFormats.test.js that you could add to.
  • Also, a publicly accessible IIIFv3 manifest for seeAlso and annotations each would be nice for inclusion in the demo, if you have them :-)

As for the length-check on the body, I think the purpose is to check if the resource needs to be fetched from an external URI (in this case the body has a single key, id with the URI).

if (!annotationJson.resources.some(hasExternalResource)) {
if (naiveIIIFv3Check(annotationJson)) {
// We treat this as IIIF 3.0
yield fetchExternalAnnotationResourceIIIFv3({ targetId, annotationId, annotationJson });
Copy link
Member

@jbaiter jbaiter Dec 15, 2021

Choose a reason for hiding this comment

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

Did you verify that this works? I think delegating to other sagas should either use the call effect or yield* to yield all of the effects from the delegated saga?

}

/** Fetching external annotation resources IIIF 2.x style */
export function* fetchExternalAnnotationResourcesIIIFv2({
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be exported, since it's an internal API, non?

@@ -151,8 +180,48 @@ export function* fetchExternalAnnotationResources({ targetId, annotationId, anno
);
}

/** Fetching external annotation resources IIIF 3.0 style */
export function* fetchExternalAnnotationResourceIIIFv3({ targetId, annotationId, annotationJson }) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as for v2, I don't the export is needed.

}
const resourceUris = uniq(annotationJson.items.map((anno) => anno.body.id.split('#')[0]));

console.log(resourceUris);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that should be in there :-)


console.log(resourceUris);
const contents = yield all(resourceUris.map((uri) => call(fetchAnnotationResource, uri)));
const contentMap = Object.fromEntries(contents.map((c) => [c.id ?? c['@id'], c]));
Copy link
Member

Choose a reason for hiding this comment

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

Is the c.id ?? c['@id'] check neccessary given that we know we're operating in a IIIFv3 context?

@@ -168,6 +237,24 @@ export function* processTextsFromAnnotations({ targetId, annotationId, annotatio
}
}

/** Saga for processing texts from IIIF annotations IIIF 3.0 */
export function* processTextsFromAnnotationsIIIFv3({ targetId, annotationId, annotationJson }) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as for the other v2/v3 sagas, I don't think the export is needed.

anno.motivation === 'supplementing' &&
anno.type === 'Annotation' &&
anno.body &&
anno.body.type === 'TextualBody' // See https://www.w3.org/TR/annotation-model/#embedded-textual-body
Copy link
Member

@jbaiter jbaiter Dec 15, 2021

Choose a reason for hiding this comment

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

I think those two can be simplified to anno.body?.type === 'TextualBody'

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