search

sul-dlss-deprecated / iiifManifestLayouts

Other
10 stars 5 forks source link

Sometimes a resource id is not a url. Handle that. #116

Closed nein09 closed 8 years ago

nein09 commented 8 years ago

While testing my fix for #97 I discovered that my work on #112 was incomplete. I hadn't taken into account the fact that a resource id might not be a URL that LegacyTileSource can use.

iangilman commented 8 years ago

Looks reasonable to me, but @aeschylus would know better.

aeschylus commented 8 years ago

Hm. I didn't put this comment in the best place. Sorry about that: https://github.com/sul-dlss/iiifManifestLayouts/issues/97#issuecomment-171080571

Gist: If we can copy the "no url" rule for when the @id is a iiif url (which might have full/full in it), then I think this will be ready.

Since it's so closely related to this pull request, I won't fix the full/full thing, so please add that in here.

nein09 commented 8 years ago

This is better now- if I switch to overview mode on the Osborn manifest and scroll all the way through it while loading thumbs, it remains responsive most of the time.

I tried to strike a balance with the thumbnail size here - quarter-size thumbs still look good in detail mode, and don't make overview mode totally die. But if I switch to eighth-size thumbnails, overview mode just works a lot better- the sacrifice you make there is that detail mode looks a little blurry and grainy on some of these.

What do you think about making the thumbnail factory aware of which mode the viewer is in? it could build 1/8 -size thumbnails for overview mode, and quarter-size thumbs (or even larger, since we only want one at a time) for detail.

aeschylus commented 8 years ago

Good question. I'm not sure about making it aware of the mode. It seems building it into the semantic zoom makes more sense. Then again, the thumbnail swapping is generally fairly quick when in detail mode, so perhaps lower detail is okay for now. Let me try this branch out really quick.

aeschylus commented 8 years ago

This is a little better. I will merge this for now so I can demo, but there seem to be a number of regressions. It seems the layers are turned off by default, but the base layer is always loaded in addition to its duplicate. This means I can end up with a sandwich of the base layer, a detail image, and then the base layer again, and there is no way to turn off the base layer duplicate.

Also, it looks like switching between modes (book, continuous, etc.) in detail mode does not animate the pages to their new positions, but the primary page just jumps into place. Resizing does animate, when it should be an instantaneous resizing.

Also, the adjacent pages and/or the thumbnails for them, do not seem to be pre-loaded (they are, by chance, when the app initialises in overview mode), so shifting from one page to the next is a confusing experience. We can address this one later, but it's good that we take note of it now.

nein09 commented 8 years ago

Can you tell me which manifest/canvas you're seeing that on, with the layers? I'm not able to see that at all.

I'm seeing the animation bug, but with every mode except continuous.

iangilman commented 8 years ago

Seems to me like we should go with whatever size thumbnail looks good in overview (sounds like 1/8th) and then do the semantic zoom when we go into detail.

That said, another option we use LegacyTileSource's ability to have a whole pyramid...you can provide the 1/8th, 1/4th and 1/2 to the tile source and it'll pick the right one based on zoom. Maybe that would be good in the case where we don't have any other full tile source, but I think it would be overkill otherwise.

aeschylus commented 8 years ago

Yes, that is correct. Like in the original demo (sul-reader-test.stanford.edu/manifestor), the thumbnail size is not based on the size of the original, but rather the context in which it needs to be displayed. As in this comment, a size of "twice the size in overview" was good enough in most cases.