Demo of wd image positions as IIIF annotations
ClosedPublic

Authored by tomcrane on Oct 27 2018, 4:54 PM.

Details

Reviewers
LucasWerkmeister
Patch without arc
git checkout -b D1122 && curl -L https://phabricator.wikimedia.org/D1122?download=true | git apply
Summary

This is for demo purposes only, and could be made richer.
It adds two new routes to the tool-wd-image-positions application:

  1. A IIIF Manifest, to provide a canvas (annotation space) for the image
  2. An annotation list, that contains the annotations that target the canvas

Examples:

http://localhost:5000/iiif/Q1231009/manifest.json
http://localhost:5000/iiif/Q1231009/list/annotations.json

This can be made more robust with some error handling. The annotations are also very sparse, I made them as simple as possible just to demonstrate them working in Mirador:

https://www.dropbox.com/s/782rc0kadlhq81b/napoleon-mirador.jpg?dl=0

Updated to address most of the comments, and to add some preview links to the home page.

You might want to make those links less obtrusive!

The Preview in Mirador will launch a version of Mirador for the generated manifest; you'll need to then turn on the annotation layer (the speech bubble icon) to see the annotations.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
tomcrane requested review of this revision.Oct 27 2018, 4:54 PM
tomcrane created this revision.

Okay, let’s see if I can figure out how code review on Differential works, I don’t think I’ve ever done this before :)

I haven’t tried this out yet, just some comments from looking at the code. But in general – can you explain to me a bit more what this does, and what it’s good for? I’ve never heard of this “Mirador” thing before.

(Also, I think Phabricator lets you upload images directly instead of going through Dropbox.)

Also, thanks already for contributing! <3

app.py
16

I usually prefer import imports over from imports so that it’s clear in the code where a member comes from (flask.url_for, yaml.parse, etc.), except for local modules (like exceptions below).

124

I’d move this and the next function between item_and_property and iif_region, rather than below api_add_qualifier, so that all the routes which accept an item ID are grouped together.

(Also, what about /iiif/<item_id>/<property_id>/manifest.json and /iiif/<item_id>/<property_id>/list/annotations.json routes?)

124

Generally, I’d like to see some more blank lines in both these functions, separating blocks that belong together, so they’re easier to read. Perhaps some more parts can also be extracted into further functions, like load_image_info already is?

151

This (together with the same line in iiif_annotations below) looks like it could perhaps be a decorator, something like @cross_origin_route? Not sure.

158

“I” is ambiguous once this is merged, so I’d prefer “we” :)

162

I suspect this will give you an HTTP URL on Toolforge because Flask doesn’t know it’s behind a proxy (as opposed to an HTTPS URL, as would be appropriate) – see R2362:e2aaed66be048171e168ae4ca0231d3fa98b6f39.

164

Couldn’t this use the url variable?

171

Might be worth extracting some function (item_url?) that could also be used in the item_link function (which currently does the same thing as this line).

295

Perhaps use the mwapi module here, which we already import for OAuth stuff? (Though I should also direct that comment at myself, I suppose.)

requirements.txt
7

I’d prefer to keep this list alphabetically ordered (i. e. move iiif-prezi up above mwapi).

Hi Lucas, I should have added a link to this discussion!

https://groups.google.com/forum/#!topic/iiif-discuss/fTe2fY1TkDY

Mirador is a IIIF viewer that is capable of displaying a subset of annotation types. The code I have added is really just a demonstration that the metadata available already in Wikidata is sufficient to publish a IIIF Manifest, with a linked annotation list for the regions of interest.

I noticed a unicode bug which I need to fix, and I would also make the code more robust. The annotations I am creating from your data are the minimum possible, there is more information that could be added to them to make them more useful in the IIIF context.

More on IIIF: https://iiif.io and https://resources.digirati.com/iiif/an-introduction-to-iiif/

I will update the diff in response to your comments, though it may be a day or two before I can get to it.

Tom

Thanks for the extra context! I’d definitely be interested to merge this when your new version of the patch is ready (and sorry for the late reply).

(I could also try to clean up the patch myself, but I don’t know how many changes you already made without uploading them yet, so that might result in some duplicate work.)

app.py
295

I’ve pushed a commit to make the rest of the tool use mwapi instead of urllib.request.urlopen :)

Jheald added a subscriber: Jheald.Nov 5 2018, 5:32 PM
tomcrane updated this revision to Diff 2936.Nov 8 2018, 7:35 PM
tomcrane edited the summary of this revision. (Show Details)
tomcrane marked 10 inline comments as done.

Done all apart from the CORS decorator

tomcrane edited the summary of this revision. (Show Details)Nov 8 2018, 7:40 PM

Thanks! (And sorry again for the delay.) I’ve added some fixups in R2422:941b7584a0c3 and deployed the results to Toolforge – is it okay for you if I squash that commit into yours and then push the result to the master branch?

app.py
95

Is there a reason why this issues an external redirect instead of directly returning the result of iiif_manifest_with_property(item_id, DEFAULT_PROPERTY), as e. g. item and iiif_annotations do?

LucasWerkmeister accepted this revision.Sun, Dec 2, 3:00 PM

I’ve merged this into master now (with my minor fixups squashed into the original commit). Thanks for contributing!

This revision is now accepted and ready to land.Sun, Dec 2, 3:00 PM

(I think this revision can be closed now? I still don’t understand Differential.)