ISLANDORA-2334 Add variables for TN & MED #185
Conversation
|
Looking for some feedback on this before I add 2 more PRs for the other related repos. @Islandora/7-x-1-x-committers |
|
@DonRichards So can these variables only be set by a drush command? Why not in the config form? |
|
@bondjimbond @DonRichards +1 for adding a way to adjust these via admin pages. |
includes/derivatives.inc
Outdated
| @@ -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")); | |||
There was a problem hiding this comment.
Is your intent to make a re-usable variable across multiple modules that get used here?
|
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? |
|
@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. |
|
@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. |
|
Should I create the admin console change before we test this? |
|
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? |
|
Example of what I was suggesting. |
|
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
|
|
@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. |
|
The only thing that would need to be centralized is the TN derivative size, no? |
|
@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 true, still not every TN is built from an actual provided larger image source, think about collections, sound, video, and compound. |
|
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? |
|
What about something more like this for each module |
|
I do like that approach better @DonRichards |
|
Added the admin console with the logic to validate the values. |
|
Left the variables out of the install file deliberately. This variable will be shared by other modules. |
|
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. |
|
Oh, cool! I'm ok with that, seems like the that form validation can be easier than with the "," separators right? |
|
Yeah basically. |
|
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 |
|
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. |
| '#attributes' => array( | ||
| 'type' => 'number', | ||
| ), | ||
| '#element_validate' => array('element_validate_number'), |
There was a problem hiding this comment.
Would element_validate_integer_positive make more sense, across these? Could then get rid of the islandora_large_image_admin_validate() stuff, right?
There was a problem hiding this comment.
I went with this because it was being used in the PDF solution pack and tried to stay consistent.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That does sound like a good way to go.
There was a problem hiding this comment.
That shouldn't block this PR?
| $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>'), |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Implementing the regeneration of all large image TN sounds potentially like a lot more code. Should that logic happen in a 2nd PR?
|
@adam-vessey What are your thoughts on my comments? |
README.md
Outdated
|
|
||
| #### 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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll go with line 30's example.
| $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>'), |
There was a problem hiding this comment.
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.
| '#attributes' => array( | ||
| 'type' => 'number', | ||
| ), | ||
| '#element_validate' => array('element_validate_number'), |
There was a problem hiding this comment.
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?
|
@adam-vessey Do you have a second to respond to my comments? |
|
@Islandora/7-x-1-x-committers Ready for review. |
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