diff --git a/common-lib/src/main/scala/com/gu/mediaservice/model/Crop.scala b/common-lib/src/main/scala/com/gu/mediaservice/model/Crop.scala index 879ddd4c17..3755981610 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/model/Crop.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/model/Crop.scala @@ -15,7 +15,14 @@ object Crop { def getCropId(b: Bounds) = List(b.x, b.y, b.width, b.height).mkString("_") def createFromCropSource(by: Option[String], timeRequested: Option[DateTime], specification: CropSpec, master: Option[Asset] = None, cropSizings: List[Asset] = Nil): Crop = - Crop(Some(getCropId(specification.bounds)), by, timeRequested, specification, master, cropSizings) + Crop( + id = Some(getCropId(specification.bounds)), + author = by, + date = timeRequested, + specification = specification, + master = master, + assets = cropSizings + ) def createFromCrop(crop: Crop, master: Asset, assets: List[Asset]): Crop = Crop(crop.id, crop.author, crop.date, crop.specification, Some(master), assets) @@ -43,6 +50,7 @@ object Crop { sealed trait ExportType { val name: String } case object CropExport extends ExportType { val name = "crop" } case object FullExport extends ExportType { val name = "full" } +case object PointsOfInterestExport extends ExportType { val name = "poi" } object ExportType { @@ -51,6 +59,7 @@ object ExportType { def valueOf(name: String): ExportType = name match { case "crop" => CropExport case "full" => FullExport + case "poi" => PointsOfInterestExport } implicit val exportTypeWrites: Writes[ExportType] = Writes[ExportType](t => JsString(t.name)) diff --git a/cropper/app/controllers/CropperController.scala b/cropper/app/controllers/CropperController.scala index 3a09d11441..2ff896dd2b 100644 --- a/cropper/app/controllers/CropperController.scala +++ b/cropper/app/controllers/CropperController.scala @@ -167,16 +167,16 @@ class CropperController(auth: Authentication, crops: Crops, store: CropStore, no apiImage <- fetchSourceFromApi(exportRequest.uri, onBehalfOfPrincipal) _ <- verify(apiImage.valid, InvalidImage) // Image should always have dimensions, but we want to safely extract the Option - dimensions <- ifDefined(apiImage.source.dimensions, InvalidImage) - cropSpec = ExportRequest.toCropSpec(exportRequest, dimensions) - _ <- verify(crops.isWithinImage(cropSpec.bounds, dimensions), InvalidCropRequest) + originalDimensions <- ifDefined(apiImage.source.dimensions, InvalidImage) + cropSpec = ExportRequest.toCropSpec(exportRequest, originalDimensions) + _ <- verify(crops.isWithinImage(cropSpec.bounds, originalDimensions), InvalidCropRequest) crop = Crop.createFromCropSource( by = Some(Authentication.getIdentity(user)), timeRequested = Some(new DateTime()), specification = cropSpec ) - markersWithCropDetails = logMarker ++ Map("imageId" -> apiImage.id, "cropId" -> Crop.getCropId(cropSpec.bounds)) - ExportResult(id, masterSizing, sizings) <- crops.makeExport(apiImage, crop)(markersWithCropDetails) + markersWithCropDetails = logMarker ++ Map("imageId" -> apiImage.id, "cropId" -> crop.id) + ExportResult(id, masterSizing, sizings) <- crops.makeExport(apiImage, crop, originalDimensions)(markersWithCropDetails) finalCrop = Crop.createFromCrop(crop, masterSizing, sizings) } yield (id, finalCrop) } diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 8300cc24fc..e5bf6d3836 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -23,9 +23,9 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera private val cropQuality = 75d private val masterCropQuality = 95d - def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false): String = { + def outputFilename(source: SourceImage, cropId: String, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false): String = { val masterString: String = if (isMaster) "master/" else "" - s"${source.id}/${Crop.getCropId(bounds)}/${masterString}$outputWidth${fileType.fileExtension}" + s"${source.id}/$cropId/$masterString$outputWidth${fileType.fileExtension}" } def createMasterCrop( @@ -36,7 +36,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera colourModel: Option[String], )(implicit logMarker: LogMarker): Future[MasterCrop] = { - val source = crop.specification + val cropSpec = crop.specification val metadata = apiImage.metadata val iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(apiImage.fileMetadata) @@ -44,15 +44,15 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera for { strip <- imageOperations.cropImage( - sourceFile, apiImage.source.mimeType, source.bounds, masterCropQuality, config.tempDir, + sourceFile, apiImage.source.mimeType, cropSpec.bounds, masterCropQuality, config.tempDir, iccColourSpace, colourModel, mediaType, isTransformedFromSource = false ) file: File <- imageOperations.appendMetadata(strip, metadata) - dimensions = Dimensions(source.bounds.width, source.bounds.height) - filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true) + dimensions = Dimensions(cropSpec.bounds.width, cropSpec.bounds.height) + filename = outputFilename(apiImage, crop.id.get, dimensions.width, mediaType, isMaster = true) sizing = store.storeCropSizing(file, filename, mediaType, crop, dimensions) - dirtyAspect = source.bounds.width.toFloat / source.bounds.height - aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) + dirtyAspect = cropSpec.bounds.width.toFloat / cropSpec.bounds.height + aspect = cropSpec.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) } yield MasterCrop(sizing, file, dimensions, aspect) } @@ -64,7 +64,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera for { file <- imageOperations.resizeImage(sourceFile, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType) optimisedFile = imageOperations.optimiseImage(file, cropType) - filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType) + filename = outputFilename(apiImage, crop.id.get, dimensions.width, cropType) sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions) _ <- delete(file) _ <- delete(optimisedFile) @@ -89,21 +89,28 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera positiveCoords && strictlyPositiveSize && withinBounds } - def makeExport(apiImage: SourceImage, crop: Crop)(implicit logMarker: LogMarker): Future[ExportResult] = { - val source = crop.specification + def makeExport(apiImage: SourceImage, _crop: Crop, originalDimensions: Dimensions)(implicit logMarker: LogMarker): Future[ExportResult] = { + val crop = _crop.specification.`type` match { + case PointsOfInterestExport => _crop.copy(specification = _crop.specification.copy(bounds = Bounds(0, 0, originalDimensions.width, originalDimensions.height))) + case _ => _crop + } + val cropSpec = crop.specification val mimeType = apiImage.source.mimeType.getOrElse(throw MissingMimeType) val secureUrl = apiImage.source.secureUrl.getOrElse(throw MissingSecureSourceUrl) val colourType = apiImage.fileMetadata.colourModelInformation.getOrElse("colorType", "") val hasAlpha = apiImage.fileMetadata.colourModelInformation.get("hasAlpha").flatMap(a => Try(a.toBoolean).toOption).getOrElse(true) val cropType = Crops.cropType(mimeType, colourType, hasAlpha) - Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + Stopwatch(s"making crop assets for ${apiImage.id} ${crop.id}") { for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) colourModel <- ImageOperations.identifyColourModel(sourceFile, mimeType) masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel) - outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions + outputDims = dimensionsFromConfig( + bounds = cropSpec.bounds, + aspectRatio = masterCrop.aspectRatio + ) :+ masterCrop.dimensions sizes <- createCrops(masterCrop.file, outputDims, apiImage, crop, cropType) masterSize <- masterCrop.sizing diff --git a/cropper/app/model/ExportRequest.scala b/cropper/app/model/ExportRequest.scala index f99bc6c019..88b015ac9f 100644 --- a/cropper/app/model/ExportRequest.scala +++ b/cropper/app/model/ExportRequest.scala @@ -16,6 +16,8 @@ case class FullExportRequest(uri: String) extends ExportRequest case class CropRequest(uri: String, bounds: Bounds, aspectRatio: Option[String]) extends ExportRequest +case class PointsOfInterest(uri: String, bounds: Bounds) extends ExportRequest + object ExportRequest { @@ -27,12 +29,18 @@ object ExportRequest { (__ \ "aspectRatio").readNullable[String](pattern(aspectRatioLike)) )(CropRequest.apply _) + private val readPointsOfInterestRequest: Reads[PointsOfInterest] = ( + (__ \ "source").read[String] ~ + __.read[Bounds] + )(PointsOfInterest.apply _) + private val readFullExportRequest: Reads[FullExportRequest] = (__ \ "source").read[String].map(FullExportRequest.apply) implicit val readExportRequest: Reads[ExportRequest] = Reads[ExportRequest](jsValue => (jsValue \ "type").validate[String] match { case JsSuccess("crop", _) => readCropRequest.reads(jsValue) + case JsSuccess("poi", _) => readPointsOfInterestRequest.reads(jsValue) case JsSuccess("full", _) => readFullExportRequest.reads(jsValue) case _ => JsError("invalid type") } @@ -41,6 +49,8 @@ object ExportRequest { def boundsFill(dimensions: Dimensions): Bounds = Bounds(0, 0, dimensions.width, dimensions.height) def toCropSpec(cropRequest: ExportRequest, dimensions: Dimensions): CropSpec = cropRequest match { + case PointsOfInterest(uri, bounds) => + CropSpec(uri, bounds, None, PointsOfInterestExport) case FullExportRequest(uri) => CropSpec( uri, diff --git a/cropper/test/lib/CropsTest.scala b/cropper/test/lib/CropsTest.scala index a0d0ba8a62..961a4c7866 100644 --- a/cropper/test/lib/CropsTest.scala +++ b/cropper/test/lib/CropsTest.scala @@ -46,27 +46,27 @@ class CropsTest extends AnyFunSpec with Matchers with MockitoSugar { private val store = mock[CropStore] private val imageOperations: ImageOperations = mock[ImageOperations] private val source: SourceImage = SourceImage("test", mock[Asset], valid = true, mock[ImageMetadata], mock[FileMetadata]) - private val bounds: Bounds = Bounds(10, 20, 30, 40) + private val cropId: String = "10_20_30_40" private val outputWidth = 1234 it("should should construct a correct address for a master jpg") { val outputFilename = new Crops(config, store, imageOperations) - .outputFilename(source, bounds, outputWidth, Jpeg, isMaster = true) + .outputFilename(source, cropId, outputWidth, Jpeg, isMaster = true) outputFilename shouldBe "test/10_20_30_40/master/1234.jpg" } it("should should construct a correct address for a non-master jpg") { val outputFilename = new Crops(config, store, imageOperations) - .outputFilename(source, bounds, outputWidth, Jpeg) + .outputFilename(source, cropId, outputWidth, Jpeg) outputFilename shouldBe "test/10_20_30_40/1234.jpg" } it("should should construct a correct address for a non-master tiff") { val outputFilename = new Crops(config, store, imageOperations) - .outputFilename(source, bounds, outputWidth, Tiff) + .outputFilename(source, cropId, outputWidth, Tiff) outputFilename shouldBe "test/10_20_30_40/1234.tiff" } it("should should construct a correct address for a non-master png") { val outputFilename = new Crops(config, store, imageOperations) - .outputFilename(source, bounds, outputWidth, Png) + .outputFilename(source, cropId, outputWidth, Png) outputFilename shouldBe "test/10_20_30_40/1234.png" } } diff --git a/kahuna/public/js/crop/controller.js b/kahuna/public/js/crop/controller.js index c6d5c351e8..4033ccbc79 100644 --- a/kahuna/public/js/crop/controller.js +++ b/kahuna/public/js/crop/controller.js @@ -3,7 +3,7 @@ import angular from 'angular'; import '../components/gr-keyboard-shortcut/gr-keyboard-shortcut'; import {radioList} from '../components/gr-radio-list/gr-radio-list'; import {cropUtil} from "../util/crop"; -import {cropOptions} from "../util/constants/cropOptions"; +import {cropOptions, pointsOfInterestBeta} from "../util/constants/cropOptions"; const crop = angular.module('kahuna.crop.controller', [ 'gr.keyboardShortcut', @@ -150,7 +150,7 @@ crop.controller('ImageCropCtrl', [ ctrl.cropping = true; - mediaCropper.createCrop(ctrl.image, coords, ratioString) + mediaCropper.createCrop(ctrl.image, coords, ratioString, ctrl.cropType === pointsOfInterestBeta.key ? 'poi' : 'crop') .then(crop => { // Global notification of action $rootScope.$emit('events:crop-created', { @@ -186,6 +186,8 @@ crop.controller('ImageCropCtrl', [ && cropSettings.shouldShowCropGuttersIfApplicable() && maybeCropRatioIfStandard === "5:3"; + ctrl.isPointsOfInterestCrop = newCropType === pointsOfInterestBeta.key; /* TODO adjust the height of easel to avoid scrollbar from explainer */ + if (isCropTypeDisabled) { ctrl.cropType = oldCropType; } else { diff --git a/kahuna/public/js/crop/view.html b/kahuna/public/js/crop/view.html index b121d321d9..920533337a 100644 --- a/kahuna/public/js/crop/view.html +++ b/kahuna/public/js/crop/view.html @@ -101,10 +101,15 @@ Please try to use a larger crop or an alternative image. -
+
Although this is a 5:3 crop, it might be used in a 5:4 space, so please ensure - there is nothing important in the striped sides as these might be clipped.
- You can also suggest alternative crops for the trail image back in Composer. + there is nothing important in the striped sides as these might be clipped. +
+ + +
+ This is a 'Points of Interest ᴮᴱᵀᴬ` crop. Use the crop rectangle to cover all the vital points of interest, but nothing more. + The frontend can use the crop in spaces of different shapes/ratios ensuring the important parts are always visible.