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

Fix multi-file media to allow image fields only on image derivatives. #892

Merged
merged 3 commits into from
Dec 14, 2022

Conversation

rosiel
Copy link
Member

@rosiel rosiel commented Jul 26, 2022

GitHub Issue: Islandora/documentation#2003

What does this Pull Request do?

with @ajstanley, fixes the general and the image case of multifile media derivatives so that expected behaviours are maintained.

What's new?

This change is set in the UI form and can probably be overridden by setting up the action in the raw config, if you so desire.

  • Does this change add any new dependencies? no
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? no
  • Could this change impact execution of existing code? no

How should this be tested?

Reproduce

  • on 2.x-dev, enable Islandora and Islandora Image.
  • Go to Configuration > System > Actions and set up a new "Generate a derivative file for media attachment".
  • You should be able to select any image or file fields you have enabled (including the defaults like field_media_file and field_media_image.). The fact that you can see images is unnecessary.
  • Set up a "Generate an image derivative for media attachment".
  • You should be able to select any image or file field. The fact that you can select file fields is unnecessary.

Test fix

  • With this PR, "Generate a derivative file for media attachment" should only allow file fields.
  • With this PR, "Generate an image derivative for media attachment"
  • Please be as detailed as possible.
  • Good testing instructions help get your PR completed faster.

Documentation Status

  • Does this change existing behaviour that's currently documented? i don't think this is documented
  • Does this change require new pages or sections of documentation?
  • Who does this need to be documented for? everybody
  • Associated documentation pull request(s): ___ or documentation issue : [DOCS] Multi-File Media documentation#2140

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

@Islandora/committers

@rosiel rosiel changed the title Update image file derivs Fix multi-file media to allow image fields only on image derivatives. Jul 26, 2022
@whikloj
Copy link
Member

whikloj commented Sep 14, 2022

@rosiel I'm a little confused on how the weird images that Drupal considers files get handled with this change to the "Generate Image Derivative For Media Attachement". If I wanted to create a JP2 can that go in an image field, and if I want to generate a Tiff from (say) a PDF so I use the "Generate File Derivative..." instead?

Otherwise this seems to work as described and reduces the available fields.

@rosiel
Copy link
Member Author

rosiel commented Sep 14, 2022

I'll look this over in more detail. I think you may be right that if you want to create a JP2 derivative from a TIFF you might unintuitively need to use the "File derivative" option.

@rosiel rosiel marked this pull request as draft September 14, 2022 17:54
@rosiel rosiel self-assigned this Sep 14, 2022
@rosiel rosiel force-pushed the update-image-file-derivs branch from 3b4dea9 to 72eaaf6 Compare November 24, 2022 21:47
@rosiel rosiel marked this pull request as ready for review November 24, 2022 23:51
@nigelgbanks nigelgbanks self-requested a review December 14, 2022 09:40
@nigelgbanks
Copy link
Member

nigelgbanks commented Dec 14, 2022

In the default site configuration (I'm testing against the starter site) field_media_file seems to be set up to handle tiff, tif, jp2 whereas field_media_image is set up to only handle png, gif, jpg, jpeg.

With this change, I'm no longer able to select field_media_file for generate_an_image_derivative_for_media_attachment actions, which is the intention, but likely the defaults should change such that field_media_file no longer handles tiff, tif, jp2 and field_media_image can handle them?

I'm uncertain if this will cause issues, as tiff and jp2 images cannot be displayed in the browser without a IIIF viewer. Does Drupal make some assumptions about image fields? Perhaps that why this is set up in this way?

I tried to manually change the field_media_image to also have tiff and jp2, though this doesn't seem to be reflected in the wiget.

Field Settings
Screenshot 2022-12-14 at 12 17 05

Create Image Media form
Screenshot 2022-12-14 at 12 17 37

Though that being said, the generate_image_derivative_file is not used in the starter site, only generate_image_derivative is which works with the field_media_file. So, there is no problem here other than my confusion as to how this all works :D

Copy link
Member

@nigelgbanks nigelgbanks left a comment

Choose a reason for hiding this comment

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

Works as stated, I was able to add another image field onto my image media type and populated it via the new action, which was limited to only selecting image fields.

@nigelgbanks nigelgbanks merged commit dfa0959 into Islandora:2.x Dec 14, 2022
@rosiel rosiel deleted the update-image-file-derivs branch December 14, 2022 14:06
@rosiel
Copy link
Member Author

rosiel commented Dec 14, 2022

Thank you! We should document the multifile media better so that you know that if you're creating a tiff or jpeg, you need to use File.

Drupal does make assumptions about images. To my knowledge, Drupal Image fields refuse to accept JP2 or Tiff. I think this has something to do with the Drupal ImageService interface (which i understand is a wrapper for some backend software like imagemagick or GD). The ImageService perhaps outlaws those file types? or perhaps if we had a version of imagemagick that was compiled properly it would work? But would we want to ship drupal software that makes those kinds of assumptions about the capabilities of underlying and abstracted services?)

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.

3 participants