Skip to content

Updates the canvas dimension method to use multiple bundles.#8402

Merged
tdonohue merged 6 commits intoDSpace:mainfrom
mspalti:canvas-dims-mutiple-bundles
Dec 6, 2022
Merged

Updates the canvas dimension method to use multiple bundles.#8402
tdonohue merged 6 commits intoDSpace:mainfrom
mspalti:canvas-dims-mutiple-bundles

Conversation

@mspalti
Copy link
Copy Markdown
Member

@mspalti mspalti commented Jul 13, 2022

Description

The current method used to "guess" the default canvas dimensions assumes too much about bundle content. It just checks the first bitstream in the first bundle. We actually don't know which bundles contain IIIF image data. Nor can we assume that the first bitstream in a bundle is an image resource.

This PR updates the method that sets default canvas dimensions so it can handle all possibilities.

Instructions for Reviewers

You will need an IIIF-enabled DSpace instance and a running image server. There are lots of ways to test this, but I suggest creating a new iiif enabled Item. Add one or more images to a custom bundle (say 'iiif'). Do not add IIIF width or height to the bitstream metadata.

When you view the Item the manifest should deliver image canvases and the canvas dimensions should be those of the first image in your custom bundle.

You can further test by adding an image to the ORIGINAL bundle (again with no metadata). When you view the item you should see the additional image. The canvas dimensions should match those of the new image in the ORIGINAL bundle (because the ORIGINAL bundle is processed before the custom bundle you added previously).

Unfortunately, since we don't currently include an image server in CI tests no integration tests are provided here.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
  • My PR passes Checkstyle validation based on the Code Style Guide.
  • My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
  • My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

@mspalti mspalti force-pushed the canvas-dims-mutiple-bundles branch from 4516510 to eaaa876 Compare July 13, 2022 23:17
@mspalti mspalti self-assigned this Jul 13, 2022
@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Sep 21, 2022

@tdonohue , it looks like this one didn't make it onto the 7.4 board. It's a small-ish bug fix for IIIF canvas dimensions.

@tdonohue tdonohue added bug integration: IIIF Related to International Image Interoperability Framework (IIIF) support labels Sep 21, 2022
@tdonohue
Copy link
Copy Markdown
Member

@mspalti : Thanks for catching it! Apologies, sometimes things slip through the cracks. Please feel free to add your PRs directly to our board in the future as soon as you feel they are ready for reviews / consideration for the next release. Added this one now.

@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge. label Sep 22, 2022
Copy link
Copy Markdown
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

thanks @mspalti the code does what the PR describes. I have put some suggestions inline and would like to get a test that proof the issue that the PR fix (to avoid risk of future regression)

Comment thread dspace-iiif/src/main/java/org/dspace/app/iiif/service/CanvasService.java Outdated
Comment thread dspace-iiif/src/main/java/org/dspace/app/iiif/service/CanvasService.java Outdated
@mspalti mspalti force-pushed the canvas-dims-mutiple-bundles branch from 12405df to eedfa36 Compare September 29, 2022 01:04
@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Sep 29, 2022

@abollini, I addressed your feedback and added a new test to verify that the default canvas dimensions are set when images are added to a custom bundle. Existing tests cover the case of images in the ORIGINAL bundle.

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@mspalti : I spun up this PR today and I'm not really sure what I'm supposed to look for in terms of the "canvas dimensions".

I have an Item which has two images of different dimensions:

  • One image is in the ORIGINAL bundle & it has no IIIF metadata
  • Other image in a custom bundle & it has no IIIF metadata.

When I look at the Item manifest (http://localhost:8080/server/iiif/[uuid]/manifest), I see both a "/canvas/c0" and "canvas/c1" listed...but it appears the width & height are the same (and both are the dimensions of the image in my ORIGINAL bundle). Is that what I should be looking at? Or is there some other way to test this?

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Oct 2, 2022

@tdonohue: Sorry I didn't notice your feedback on Friday!!

The result you see is correct behavior. In your test case, the dimensions will be those of the first image added to your ORIGINAL bundle. The dimensions should be the same for all canvases in the manifest.

To test further, remove the image from your ORIGINAL bundle (leaving the image in your custom bundle) and verify that the canvas dimension is correctly set to the height/width of the image in the custom bundle.

Before changes in this PR, the bitstream used for setting the default canvas dimensions was set this way:

Bitstream firstBistream = bundles.get(0).getBitstreams().get(0);

This PR looks at multiple bundles (and bitstreams )and uses the first IIIF image bitstream found to set the default dimensions used for bitstreams that lack metadata.

One gotcha you might run into while testing: the bitstream dimensions are cached server-side; if you suspect caching the easiest way to clear it is just edit any bitstream metadata (e.g. description) and the cache will evict.

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Oct 2, 2022

Just an aside: it's possible to set the canvas dimensions dynamically for all canvases via a configuration setting. That's not recommended for performance reasons, but it's possible if needed. The middle ground used here is to "guess" the default canvas dimension based on a single image. It's an improvement over a statically configured default dimension (and works perfectly for single image items). This and many other things will get formal documentation soon.

@tdonohue tdonohue self-requested a review October 13, 2022 14:57
@tdonohue tdonohue force-pushed the canvas-dims-mutiple-bundles branch from c2f9945 to 34e6ce3 Compare December 6, 2022 18:41
@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Dec 6, 2022

@mspalti : I've pushed up a (automated) rebase to this PR's branch (as it's not been updated in some time). Planning on trying to test this again today.

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @mspalti ! Re-tested this today with an image in a CUSTOM bundle and verified that it now seems to work properly. Code and tests look good here as well.

@tdonohue tdonohue added this to the 7.5 milestone Dec 6, 2022
@tdonohue tdonohue merged commit 705f87a into DSpace:main Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge. bug integration: IIIF Related to International Image Interoperability Framework (IIIF) support

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants