ISLANDORA-2334 Add variables for TN & MED #185

DonRichards · 2018-11-28T16:43:06Z

JIRA Ticket: https://jira.duraspace.org/browse/ISLANDORA-2334

What does this Pull Request do?

Adds the global variable lookup and assignment as well as documentation.

What's new?

Global variables derivative_tn_size & derivative_med_size

This looks for these variables, if they are not available the module uses the current sizes already hard coded in.

How should this be tested?

Ingest a large object before this PR. View both the medium sized jpg and the TN and take note of their sizes. Medium size image should be no larger than 600 x 800 and TN should be no larger than 200 x 200.

After you switch to this PR follow the instructions on the README.md to set values.

An easy way to see the difference is set the TN to something dramatic (like 900 x 900) and click regenerate TN derivative from the manage datastream page. Now the image should be the size you specified.

Additional Notes:

N/A

Interested parties

@Islandora/7-x-1-x-committers

DonRichards · 2018-12-06T18:16:03Z

Looking for some feedback on this before I add 2 more PRs for the other related repos. @Islandora/7-x-1-x-committers

bondjimbond · 2018-12-06T18:41:11Z

@DonRichards So can these variables only be set by a drush command? Why not in the config form?

bryjbrown · 2018-12-06T18:42:59Z

@bondjimbond @DonRichards +1 for adding a way to adjust these via admin pages.

jordandukart · 2018-12-06T18:43:48Z

includes/derivatives.inc

@@ -159,7 +159,7 @@ function islandora_large_image_create_jpg_derivative(AbstractObject $object, $fo

    $uploaded_file = islandora_large_image_get_uploaded_file($object);
    $args = array();
-    $args[] = '-resize ' . escapeshellarg("600 x 800");
+    $args[] = '-resize ' . escapeshellarg(variable_get('derivative_med_size', "600 x 800"));


Is your intent to make a re-usable variable across multiple modules that get used here?

DonRichards · 2018-12-10T16:24:14Z

After some brief discussion in the committer's call: the thumbnails should be consistent between content models so their configuration should live in a central place. Any reason they shouldn't?

bondjimbond · 2018-12-10T16:31:57Z

@DonRichards I'm all for centralized TN sizing... but I wouldn't want it to be set exclusively by a drush command. There should be an admin form setting somewhere.

DonRichards · 2018-12-10T17:56:12Z

@bondjimbond I agree. This is the first step. First make it changeable. Then create the admin console. The admin console would never exist in this PR so this makes the next PR possible. A drush command is just one manual way of testing this.

DonRichards · 2018-12-10T17:56:51Z

Should I create the admin console change before we test this?

adam-vessey · 2018-12-10T18:15:11Z

So... I'm pretty sure the original "only via drush" (and not a config form) bit was for consistency, as in: What might be expected to happen when the values are changed at a time other than the genesis of an installation? Do we expect to automatically regenerate all derivatives dependent on the given values, when the values change? Extremely loud warnings where things are configurable in the GUI? Or...?

Also: What about multisite installations, where different sites can manipulate the same data, but have different configurations?

DonRichards · 2018-12-10T18:31:06Z

Example of what I was suggesting.

DiegoPino · 2018-12-10T18:37:36Z

Don, issue here is that Derivative Medium Size is not an Islandora Core thing, only applies to a single SP and a single CMODEL. Not sure what type of UX issues mixing things can bring. But i do think derivatives, in general, should have settings, and those should be provided, centralized somewhere, but code and UI should be set per SP providing the actual logic. Just my 2 cents Diego Pino Navarro Metropolitan New York Library Council 599 11th Av. New York, NY 10036

DonRichards · 2018-12-10T18:38:10Z

@adam-vessey This PR alone would have no effect on existing installs unless you run the drush command. Even then it would only effect objects created after it was ran. If you want to apply the new size for derivative generation to existing collections then you would want to regenerate for all collections you wish to apply changes to. I'm not sure how this would effect multi-sites installs. It's storing the variables in the same way the "site's name" and "which theme to use" is stored.

bondjimbond · 2018-12-10T18:43:13Z

The only thing that would need to be centralized is the TN derivative size, no?

DonRichards · 2018-12-10T18:45:28Z

@DiegoPino The admin console would apply to islandora solution pack disk image, islandora solution pack web archive and this module. To keep it all consistent same TN and MED size images should each of these 3 have their own admin settings for this instead or should this exist in core and apply to islandora's collection solution pack when it generates thumbnails?
@bondjimbond All 3 have both med and tn size images.

DiegoPino · 2018-12-10T18:45:38Z

@bondjimbond true, still not every TN is built from an actual provided larger image source, think about collections, sound, video, and compound.

DonRichards · 2018-12-10T18:47:54Z

Going back to the original need for this. It was expressed that someone needed the ability to specify TN and medium sizes and didn't want the hard coded dimensions. So how should it best be made available?

DonRichards · 2018-12-10T19:34:04Z

What about something more like this for each module

DiegoPino · 2018-12-10T20:22:51Z

I do like that approach better @DonRichards

DonRichards · 2018-12-10T21:06:52Z

Added the admin console with the logic to validate the values.

DonRichards · 2018-12-10T21:11:39Z

Left the variables out of the install file deliberately. This variable will be shared by other modules.

DonRichards · 2018-12-11T21:09:27Z

I just noticed this. Should I just change it to look like this and use the exact same variables? This is a screenshot from the PDF SP.

DiegoPino · 2018-12-11T21:36:59Z

Oh, cool! I'm ok with that, seems like the that form validation can be easier than with the "," separators right?

DonRichards · 2018-12-12T12:38:55Z

Yeah basically.

DonRichards · 2018-12-12T14:02:09Z

I added a little extra bit as well. Drupal text input to set the values as numeric only. This made validation a lot easier. Once the Travis check conclude this will be ready for testing. Also updated the documentation to reflect the changes (removed the drush examples). @bondjimbond @DiegoPino

dannylamb · 2019-01-10T19:48:55Z

Any chance a @Islandora/7-x-1-x-committers can take a look at this? Seems like a good improvement and it's been through several rounds of feedback.

adam-vessey · 2019-01-24T19:28:05Z

includes/admin.form.inc

+    '#attributes' => array(
+      'type' => 'number',
+    ),
+    '#element_validate' => array('element_validate_number'),


Would element_validate_integer_positive make more sense, across these? Could then get rid of the islandora_large_image_admin_validate() stuff, right?

I went with this because it was being used in the PDF solution pack and tried to stay consistent.

The PDF SP also appears to allow negative/zero values... which could get into an annoying situation where the PDF admin form could submit values for the variables that this form would then reject... should the validation be extended in the PDF SP as well?

That does sound like a good way to go.

That shouldn't block this PR?

adam-vessey · 2019-01-24T19:29:09Z

includes/admin.form.inc

+  $form['islandora_large_image_preview_fieldset'] = array(
+    '#type' => 'fieldset',
+    '#title' => t('Preview image'),
+    '#description' => t('Settings for creating preview image derivatives<br/><span style="color:red">WARNING: This will set the size for all medium sized images generated as a derivative. Regenerate derivatives to apply this setting to existing objects.</span>'),


Appears to indicate the MEDIUM_SIZE instead of the PREVIEW... doesn't quite seem to capture how it might affect other modules derivatives, like PDF...

Should it define what a medium size image is and how during the creation of a medium size image derivative for PDFs share the size parameter for consistency? Sorry, I hope this isn't coming off wrong. I'm looking for a little "word-smithing" here, in hope to better word or identify if something really should change.

I'm really not sure how best to handle the phrasing for this, since we're now essentially dynamically scoping these variables to whatever might use them... almost thinking there may be a better way to handle this: parameterize the dimensions in all the relevant derivative functions, and then use some hook_islandora_derivative_alter() implementation to inject our configuration from another module... the module which performs the alters can then describe the properties however it wants to, as it knows what derivative implementations it alters.

Implementing the regeneration of all large image TN sounds potentially like a lot more code. Should that logic happen in a 2nd PR?

includes/admin.form.inc

DonRichards · 2019-02-18T19:47:05Z

@adam-vessey What are your thoughts on my comments?

adam-vessey · 2019-02-20T14:01:17Z

README.md

+
+#### Width & Height
+
+These are handled as maximum sizes (retains original x to y ratio) and does not crop. These values are shared with the [PDF Solution Pack](http://localhost:8000/admin/islandora/solution_pack_config/pdf).


Just noticed the link here, pointing at localhost:8000... not really sure it makes sense... could make reference to the path (admin/islandora/solution_pack_config/pdf), but as a clickable link... yeah, not sure it makes sense.

I'll go with line 30's example.

adam-vessey · 2019-02-20T14:18:06Z

includes/admin.form.inc

+  $form['islandora_large_image_preview_fieldset'] = array(
+    '#type' => 'fieldset',
+    '#title' => t('Preview image'),
+    '#description' => t('Settings for creating preview image derivatives<br/><span style="color:red">WARNING: This will set the size for all medium sized images generated as a derivative. Regenerate derivatives to apply this setting to existing objects.</span>'),


I'm really not sure how best to handle the phrasing for this, since we're now essentially dynamically scoping these variables to whatever might use them... almost thinking there may be a better way to handle this: parameterize the dimensions in all the relevant derivative functions, and then use some hook_islandora_derivative_alter() implementation to inject our configuration from another module... the module which performs the alters can then describe the properties however it wants to, as it knows what derivative implementations it alters.

adam-vessey · 2019-02-20T14:21:17Z

includes/admin.form.inc

+    '#attributes' => array(
+      'type' => 'number',
+    ),
+    '#element_validate' => array('element_validate_number'),


The PDF SP also appears to allow negative/zero values... which could get into an annoying situation where the PDF admin form could submit values for the variables that this form would then reject... should the validation be extended in the PDF SP as well?

DonRichards · 2019-03-27T11:54:06Z

@adam-vessey Do you have a second to respond to my comments?

DonRichards · 2019-12-20T19:46:01Z

@Islandora/7-x-1-x-committers Ready for review.

DonRichards added 2 commits November 28, 2018 11:29

Add variable get for TN & MED

95533c8

Add info to use variable get for TN & MED

29e3591

jordandukart reviewed Dec 6, 2018

View reviewed changes

Adds admin console

6ed7f8f

Update screenshot

d4664f3

DonRichards added 3 commits December 10, 2018 16:18

Make a single number invalid

f1ea688

Copy pasta issue.

dcff7ae

Correct travis errors

f1aac11

DonRichards added 2 commits December 12, 2018 08:51

Align with PDF SP admin form

672e3d4

phpcs corrections

d6ef94b

Update screenshot

20c5623

adam-vessey reviewed Jan 24, 2019

View reviewed changes

adam-vessey reviewed Feb 20, 2019

View reviewed changes

replace localhost link and a string

3cc69f0

Merge branch '7.x' into ISLANDORA-2334

e81eb04


		#### Width & Height

		These are handled as maximum sizes (retains original x to y ratio) and does not crop. These values are shared with the [PDF Solution Pack](http://localhost:8000/admin/islandora/solution_pack_config/pdf).

ISLANDORA-2334 Add variables for TN & MED #185

Are you sure you want to change the base?

ISLANDORA-2334 Add variables for TN & MED #185

Conversation

DonRichards commented Nov 28, 2018

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

DonRichards commented Dec 6, 2018

bondjimbond commented Dec 6, 2018

bryjbrown commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonRichards commented Dec 10, 2018 • edited Loading

bondjimbond commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DonRichards commented Dec 10, 2018

adam-vessey commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DiegoPino commented Dec 10, 2018 via email

DonRichards commented Dec 10, 2018

bondjimbond commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DiegoPino commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DiegoPino commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DonRichards commented Dec 10, 2018

DonRichards commented Dec 11, 2018

DiegoPino commented Dec 11, 2018

DonRichards commented Dec 12, 2018

DonRichards commented Dec 12, 2018

dannylamb commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonRichards commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonRichards commented Mar 27, 2019

DonRichards commented Dec 20, 2019

DonRichards commented Dec 10, 2018 •

edited

Loading