[MGNLDAM-371] Move PDF and video preview from internal sandbox to submodule Created: 14/Jan/14  Updated: 28/May/14  Resolved: 22/May/14

Status: Closed
Project: Magnolia DAM Module
Component/s: None
Affects Version/s: 1.2.1
Fix Version/s: 2.0

Type: Bug Priority: Neutral
Reporter: Jan Haderka Assignee: Jan Haderka
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
dependency
depends upon MGNLUI-2905 DefaultImageProvider is difficult to ... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

The preview module in internal sandbox that brings in preview for PDF and videos needs to be moved to DAM as optional submodule to make this functionality part of DAM and easily pluggable.



 Comments   
Comment by Jan Haderka [ 19/May/14 ]

Preview for various types. While this module is part of the DAM, it's not part of the bundle and instead it creates own bundle for anyone to install separately. This is due to size of dependent libraries.

One pending decision - should the dependency to imaging module be kept in this module or moved up to parent's dependency management?

Comment by Jan Haderka [ 20/May/14 ]

Move imaging dependency up to dep. management section in parent pom.

Comment by Mikaël Geljić [ 21/May/14 ]
  • remove .gitignore, everything's covered in "root" .gitignore already
  • remove submodule <version> in pom; we most likely don't want dam-preview version to be out of sync w/ parent
  • ensure all copyright years are up to 2014, there's even one with 2003-2013
  • a whole lotta commented code in MultiGeneratorImageProvider, I suppose from legacy unused 5.2.x methods
  • you may split MultiGeneratorImageProviderDefinition into interface (getter only) + ConfiguredMultiGeneratorIPD impl (w/ type-mapping in module descriptor for n2b to know). Additionally you could #setImageProviderClass directly in constructor in there, to avoid having to bootstrap both def class + impl class
  • I would call version handler DamPreviewMVH, Magnolia is full of previews isn't it? — or even better, remove it as long as it's empty?
  • add module dependency to ui-framework in module descriptor, use versions 5.3/* (sync w/ maven deps)
  • any reason @Test annotations are commented out in FromVideoTest?
  • finally, silly question/too lazy to find out myself: is it ok to have the Activatable mixin properties in bootstraps? or would they be ignored anyway?
Comment by Jan Haderka [ 22/May/14 ]

remove .gitignore, everything's covered in "root" .gitignore already

Done.

remove submodule <version> in pom; we most likely don't want dam-preview version to be out of sync w/ parent

Done.

ensure all copyright years are up to 2014, there's even one with 2003-2013

Done. BTW that class is from original impl of the image provider I had for DMS so the date was not so off And there's some other code in other DAM submodules that have wrong copyright year.

a whole lotta commented code in MultiGeneratorImageProvider, I suppose from legacy unused 5.2.x methods

Indeed. Removed.

you may split MultiGeneratorImageProviderDefinition into interface (getter only) + ConfiguredMultiGeneratorIPD impl (w/ type-mapping in module descriptor for n2b to know). Additionally you could #setImageProviderClass directly in constructor in there, to avoid having to bootstrap both def class + impl class

Actually, no. My hope is that we improve imaging module and remove the need for having separate generators per type. This is currently needed because we pass only byte array with raw data in the image chain w/o any info as to what those data are. Once we do that, we would not need multiple generators for same type of the output image (thumb/preview).

I would call version handler DamPreviewMVH, Magnolia is full of previews isn't it? — or even better, remove it as long as it's empty?

Gone. It was just artefact generated by archetype.

add module dependency to ui-framework in module descriptor, use versions 5.3/* (sync w/ maven deps)

What preview needs is m-ui-imageprovider but that is not a magnolia module, just maven module, hence I can't add dependency. I could add dep to m-ui-framework itself, but then that's equally artificial as I do not need to depend on whole framework so I would be adding it to maven pom w/ real reason too. However upped dep to DAM to 2.0.

any reason @Test annotations are commented out in FromVideoTest?

Yes, comment in the header of the test class explains that. Since the tests are used to test underlying image providers not the dam code itself hence are not necessary unless changing those libraries.

finally, silly question/too lazy to find out myself: is it ok to have the Activatable mixin properties in bootstraps? or would they be ignored anyway?

Since you can activate each node in config on its own they should all be Activatable.

Comment by Mikaël Geljić [ 22/May/14 ]

Push a little harder, can't see the commit yet

Actually, no. My hope is that we improve imaging module and remove the need for having separate generators per type. This is currently needed because we pass only byte array with raw data in the image chain w/o any info as to what those data are. Once we do that, we would not need multiple generators for same type of the output image (thumb/preview).

I don't see how this can be an objection to split the def; having a def interface doesn't engrave it in stone, you'll still be able to get rid of it when the imaging changes occur in a few weeks/months. On the other hand I don't care that much, we've never had other definition 'providers' — which is where I think the split comes from originally...

What preview needs is m-ui-imageprovider but that is not a magnolia module, just maven module, hence I can't add dependency. I could add dep to m-ui-framework itself, but then that's equally artificial as I do not need to depend on whole framework so I would be adding it to maven pom w/ real reason too. However upped dep to DAM to 2.0.

Granted.

Yes, comment in the header of the test class explains that. Since the tests are used to test underlying image providers not the dam code itself hence are not necessary unless changing those libraries.

Sry I can't read obviously O:), might be more explicit w/ @Ignore though

Since you can activate each node in config on its own they should all be Activatable.

Not questioning that, but I don't think these properties make any sense in bootstraps at all, they'll be generated upon first publication. Bonus question: what happens if someone bootstraps some node w/ activationStatus green?

Comment by Jan Haderka [ 25/May/14 ]

Module is integrated in dam, but due to its size we do not bundle it with dam by default (same principle as for Swagger ui for the REST module), so to use it, one has to download magnolia-dam-preview from nexus and install it in Magnolia. Upon installation, it should work out of the box (providing preview of pdf and video in thumbnail view and in preview in action bar.

Generated at Mon Feb 12 04:59:10 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.