Merge pull request #17258 from guardian/jfs-img-meta

Always use correct image meta dimensions
guardian · Jun 26, 2017 · fa01962 · fa01962
2 parents 15e15f7 + ce82b2b
commit fa01962
Show file tree

Hide file tree

Showing 21 changed files with 80 additions and 98 deletions.
diff --git a/applications/app/services/VideoSiteMap.scala b/applications/app/services/VideoSiteMap.scala
@@ -89,7 +89,7 @@ class VideoSiteMap(contentApiClient: ContentApiClient) extends ExecutionContexts
 
         Url(
           location = item.metadata.webUrl,
-          thumbnail_loc = item.elements.thumbnail.flatMap(thumbnail => Naked.bestFor(thumbnail.images)),
+          thumbnail_loc = item.elements.thumbnail.flatMap(thumbnail => Naked.bestSrcFor(thumbnail.images)),
           content_loc = contentLocation,
           title = item.trail.headline,
           description = item.fields.trailText,

diff --git a/applications/app/views/fragments/mediaBody.scala.html b/applications/app/views/fragments/mediaBody.scala.html
@@ -85,8 +85,8 @@
 
                                             @defining(video.elements.thumbnail.map(_.images) orElse video.mediaAtom.flatMap(_.posterImage)) {bestThumbnail =>
                                                 @bestThumbnail.map { thumbnail =>
-                                             <meta itemprop="thumbnail" content="@SeoOptimisedContentImage.bestFor(thumbnail)" />
-                                             <meta itemprop="thumbnailUrl" content="@SeoOptimisedContentImage.bestFor(thumbnail)" />
+                                             <meta itemprop="thumbnail" content="@SeoOptimisedContentImage.bestSrcFor(thumbnail)" />
+                                             <meta itemprop="thumbnailUrl" content="@SeoOptimisedContentImage.bestSrcFor(thumbnail)" />
                                                 }
                                             }
 

diff --git a/applications/app/views/videoEmbed.scala.html b/applications/app/views/videoEmbed.scala.html
@@ -38,7 +38,7 @@
             }
             <meta itemprop="requiresSubscription" content="no" />
             <meta itemprop="image" content="@Html(video.content.openGraphImage)" />
-            @video.elements.thumbnail.map { img => <meta itemprop="thumbnail" content="@SeoOptimisedContentImage.bestFor(img.images)" /> }
+            @video.elements.thumbnail.map { img => <meta itemprop="thumbnail" content="@SeoOptimisedContentImage.bestSrcFor(img.images)" /> }
             @video.elements.mainVideo.map { videoElement =>
                 @fragments.media.video(VideoPlayer(
                     videoElement,

diff --git a/article/app/model/structuredData/Image.scala b/article/app/model/structuredData/Image.scala
@@ -1,16 +1,19 @@
 package model.structuredData
 
-import model.ImageElement
-import play.api.libs.json.{JsValue, Json}
-import views.support.{ImgSrc, Item700}
+import model.{ImageAsset, ImageElement}
+import play.api.libs.json.{JsString, JsValue, Json}
+import views.support.Item700
 
 object Image {
 
-  def apply(picture: ImageElement): JsValue = Json.obj(
-    "@type" -> "ImageObject",
-    "url" -> ImgSrc.findNearestSrc(picture.images, Item700),
-    "height" -> ImgSrc.getFallbackAsset(picture.images).fold(0)(_.height),
-    "width" -> ImgSrc.getFallbackAsset(picture.images).fold(0)(_.width)
-  )
+  def apply(picture: ImageElement): JsValue = {
+    val asset: Option[ImageAsset] = Item700.bestFor(picture.images)
+    Json.obj(
+      "@type" -> "ImageObject",
+      "url" -> JsString(Item700.bestSrcFor(picture.images).getOrElse("")),
+      "height" -> asset.fold(0)(_.height),
+      "width" -> asset.fold(0)(_.width)
+    )
+  }
 
 }
diff --git a/article/app/views/fragments/emailArticleBody.scala.html b/article/app/views/fragments/emailArticleBody.scala.html
@@ -86,7 +86,7 @@ <h2 class="block__title">@title</h2>
                     }
 
                     case ImageBlockElement(media, data, showCredit) => {
-                        @EmailImage.bestFor(media).map { imageUrl =>
+                        @EmailImage.bestSrcFor(media).map { imageUrl =>
                             @row(Seq("no-pad")) {
                                 @if(article.isTheMinute && block.url.isDefined) {
                                     <a href="@block.url.getOrElse("#")">
@@ -116,7 +116,7 @@ <h2 class="block__title">@title</h2>
                     }
 
                     case GuVideoBlockElement(video, media, data) => {
-                        @EmailVideoImage.bestFor(media).map { imageUrl =>
+                        @EmailVideoImage.bestSrcFor(media).map { imageUrl =>
                             @row(Seq("no-pad")) {
                                 @data.get("url").fold {
                                     @imgForArticle(imageUrl, data.get("alt"))
@@ -131,7 +131,7 @@ <h2 class="block__title">@title</h2>
 
                     case ContentAtomBlockElement(atomId) => {
                         @page.article.content.media.find(_.id == atomId).map { atom: MediaAtom =>
-                            @atom.posterImage.flatMap(EmailVideoImage.bestFor).map { imageUrl =>
+                            @atom.posterImage.flatMap(EmailVideoImage.bestSrcFor).map { imageUrl =>
                                 @atom.assets.map { asset =>
                                     @row(Seq("padded")) {
                                         @asset.platform match {

diff --git a/article/app/views/fragments/emailMainMedia.scala.html b/article/app/views/fragments/emailMainMedia.scala.html
@@ -6,7 +6,7 @@
 
 @if(!article.elements.hasMainEmbed) {
     @article.elements.mainPicture.map { picture =>
-        @EmailImage.bestFor(picture.images).map { url =>
+        @EmailImage.bestSrcFor(picture.images).map { url =>
             @row(Seq("no-pad")) {
                 <img width="580" class="full-width" src="@url" alt="@ImgSrc.getFallbackAsset(picture.images).flatMap(_.altText).getOrElse("")" />
             }

diff --git a/commercial/app/model/merchandise/Merchandise.scala b/commercial/app/model/merchandise/Merchandise.scala
@@ -241,7 +241,7 @@ object Masterclass {
       "venue" -> m.venue,
       "ticketPrice" -> m.tickets.headOption.map(_.price),
       "capacity" -> m.capacity,
-      "pictureUrl" -> m.mainPicture.map(picture => Item300.bestFor(picture.images)),
+      "pictureUrl" -> m.mainPicture.map(picture => Item300.bestSrcFor(picture.images)),
       "ratioTicketsLeft" -> m.ratioTicketsLeft
     )
   }

diff --git a/common/app/common/TrailsToRss.scala b/common/app/common/TrailsToRss.scala
@@ -20,12 +20,12 @@ import scala.collection.JavaConverters._
 
 object TrailsToRss extends implicits.Collections {
 
-  /* 
+  /*
     This regex pattern matches all invalid XML characters (see https://www.w3.org/TR/xml/#charsets)
     by specifying individual and ranges of valid ones in a negated set.  The final range \\u10000-\\u10FFFF
     is intended to match the supplementary character set, but I believe it is invalid java regex.
     It ought to be changed to \\x{10000}-\\x{10FFFF} but that produces unexpected behaviour which I suspect
-    is a bug (http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8179668).  For now, leaving this 
+    is a bug (http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8179668).  For now, leaving this
     unchanged as the end result gives valid XML, although it may exclude supplementary characters.
   */
   val pattern = Pattern.compile("[^\\x09\\x0A\\x0D\\x20-\\uD7FF\\uE000-\\uFFFD\\u10000-\\u10FFFF]")
@@ -82,8 +82,8 @@ object TrailsToRss extends implicits.Collections {
       val mediaModules: Seq[MediaEntryModuleImpl] = for {
         profile: Profile <- List(Item140, Item460)
         trailPicture: ImageMedia <- trail.trailPicture
-        trailAsset: ImageAsset <- profile.elementFor(trailPicture)
-        resizedImage <- profile.bestFor(trailPicture)
+        trailAsset: ImageAsset <- profile.bestFor(trailPicture)
+        resizedImage <- profile.bestSrcFor(trailPicture)
       } yield {
         // create media
         val media = new MediaContent(new UrlReference(resizedImage))
@@ -189,8 +189,8 @@ object TrailsToRss extends implicits.Collections {
       val mediaModules: Seq[MediaEntryModuleImpl] = for {
         profile: Profile <- List(Item140, Item460)
         trailPicture: ImageMedia <- faciaContent.trail.trailPicture
-        trailAsset: ImageAsset <- profile.elementFor(trailPicture)
-        resizedImage <- profile.bestFor(trailPicture)
+        trailAsset: ImageAsset <- profile.bestFor(trailPicture)
+        resizedImage <- profile.bestSrcFor(trailPicture)
       } yield {
         // create image
         val media = new MediaContent(new UrlReference(resizedImage))

diff --git a/common/app/common/commercial/hosted/ContentUtils.scala b/common/app/common/commercial/hosted/ContentUtils.scala
@@ -33,5 +33,5 @@ object ContentUtils {
     } getOrElse ImageMedia(Nil)
 
   def thumbnailUrl(item: Content): String =
-    ImgSrc.findNearestSrc(imageMedia(item), Item300) getOrElse ""
+    Item300.bestSrcFor(imageMedia(item)) getOrElse ""
 }
diff --git a/common/app/model/VideoPlayer.scala b/common/app/model/VideoPlayer.scala
@@ -25,7 +25,7 @@ case class VideoPlayer(
   hasFaciaHeader: Boolean = false,
   faciaHeaderProperties: Option[VideoFaciaProperties] = None
 ) {
-  def poster: String = profile.bestFor(video.images).getOrElse(Static("images/media-holding.jpg"))
+  def poster: String = profile.bestSrcFor(video.images).getOrElse(Static("images/media-holding.jpg"))
 
   /** Width and height are always defined for video profile, so this is OK. */
   def width: Int = profile.width.get

diff --git a/common/app/model/content.scala b/common/app/model/content.scala
@@ -629,7 +629,7 @@ final case class Video (
 
   def sixteenByNineMetaImage: Option[String] = for {
     imageMedia <- mediaAtom.flatMap(_.posterImage) orElse content.elements.thumbnail.map(_.images)
-    videoProfile <- Video1280.bestFor(imageMedia)
+    videoProfile <- Video1280.bestSrcFor(imageMedia)
   } yield videoProfile
 }
 
@@ -734,7 +734,7 @@ case class GalleryLightbox(
         "caption" -> JsString(img.caption.getOrElse("")),
         "credit" -> JsString(img.credit.getOrElse("")),
         "displayCredit" -> JsBoolean(img.displayCredit),
-        "src" -> JsString(Item700.bestFor(container.images).getOrElse("")),
+        "src" -> JsString(Item700.bestSrcFor(container.images).getOrElse("")),
         "srcsets" -> JsString(ImgSrc.srcset(container.images, GalleryMedia.lightbox)),
         "sizes" -> JsString(GalleryMedia.lightbox.sizes),
         "ratio" -> Try(JsNumber(img.width.toDouble / img.height.toDouble)).getOrElse(JsNumber(1)),
@@ -783,7 +783,7 @@ case class GenericLightbox(
         "caption" -> JsString(img.caption.getOrElse("")),
         "credit" -> JsString(img.credit.getOrElse("")),
         "displayCredit" -> JsBoolean(img.displayCredit),
-        "src" -> JsString(Item700.bestFor(container.images).getOrElse("")),
+        "src" -> JsString(Item700.bestSrcFor(container.images).getOrElse("")),
         "srcsets" -> JsString(ImgSrc.srcset(container.images, GalleryMedia.lightbox)),
         "sizes" -> JsString(GalleryMedia.lightbox.sizes),
         "ratio" -> Try(JsNumber(img.width.toDouble / img.height.toDouble)).getOrElse(JsNumber(1)),

diff --git a/common/app/model/content/Atoms.scala b/common/app/model/content/Atoms.scala
@@ -448,7 +448,7 @@ object ReviewAtom {
     for {
       image <- images.headOption
       media = model.content.MediaAtom.imageMediaMake(image, "")
-      url <- ImgSrc.findLargestSrc(media, GoogleStructuredData)
+      url <- GoogleStructuredData.bestSrcFor(media)
     } yield url
   }
 }

diff --git a/common/app/views/fragments/atoms/youtube.scala.html b/common/app/views/fragments/atoms/youtube.scala.html
@@ -67,7 +67,7 @@
         }
         @defining(posterImageOverride.filter(_ => !playable || YouTubePosterOverride.isSwitchedOn) orElse media.posterImage) { bestPosterImage =>
             @bestPosterImage.map { image =>
-                <div class="@RenderClasses(Map("youtube-media-atom__overlay" -> true, "vjs-big-play-button" -> !expired))" style="background-image: url(@Video700.bestFor(image))">
+                <div class="@RenderClasses(Map("youtube-media-atom__overlay" -> true, "vjs-big-play-button" -> !expired))" style="background-image: url(@Video700.bestSrcFor(image))">
 
                 @if(!expired) {
                     @if(mediaWrapper.contains(ImmersiveMainMedia)) {

diff --git a/common/app/views/fragments/containers/facia_cards/videoContainer.scala.html b/common/app/views/fragments/containers/facia_cards/videoContainer.scala.html
@@ -72,7 +72,7 @@ <h2 class="video-title fc-container__header__title">
                                 <div class="fc-item__image-container u-responsive-ratio inlined-image">
                                 @InlineImage.fromFaciaContent(item).map { fallbackImage =>
                                     <img
-                                    @if(index > 1) {data-}src="@Video700.bestFor(fallbackImage.imageMedia)" class="js-video-playlist-image js-video-playlist-image--@{
+                                    @if(index > 1) {data-}src="@Video700.bestSrcFor(fallbackImage.imageMedia)" class="js-video-playlist-image js-video-playlist-image--@{
                                     index
                                 }" />
                                 }

diff --git a/common/app/views/fragments/imageFigure.scala.html b/common/app/views/fragments/imageFigure.scala.html
@@ -37,9 +37,12 @@
         @if(isMain) {
             <meta itemprop="representativeOfPage" content="true">
         }
-        <meta itemprop="url" content="@ImgSrc.findNearestSrc(picture, Item700)">
-        <meta itemprop="width" content="@ImgSrc.getFallbackAsset(picture).fold(0)(_.width)">
-        <meta itemprop="height" content="@ImgSrc.getFallbackAsset(picture).fold(0)(_.height)">
+
+        @defining(Item700.bestFor(picture)) { asset =>
+            <meta itemprop="url" content="@Item700.bestSrcFor(picture).getOrElse("")">
+            <meta itemprop="width" content="@asset.fold(0)(_.width)">
+            <meta itemprop="height" content="@asset.fold(0)(_.height)">
+        }
 
         @defining(
             if (isMain) {

diff --git a/common/app/views/support/EmailHelpers.scala b/common/app/views/support/EmailHelpers.scala
@@ -10,7 +10,7 @@ object EmailHelpers {
   def imageUrlFromCard(contentCard: ContentCard, width: Int): Option[String] = {
     for {
       InlineImage(imageMedia) <- contentCard.displayElement
-      url <- SmallFrontEmailImage(width).bestFor(imageMedia)
+      url <- SmallFrontEmailImage(width).bestSrcFor(imageMedia)
     } yield url
   }
 

diff --git a/common/app/views/support/Profile.scala b/common/app/views/support/Profile.scala
@@ -20,23 +20,28 @@ sealed trait ElementProfile {
   def isPng: Boolean
   def autoFormat: Boolean
 
-  def elementFor(image: ImageMedia): Option[ImageAsset] = {
-    val sortedCrops = image.imageCrops.sortBy(-_.width)
-    width.flatMap{ desiredWidth =>
-      sortedCrops.find(_.width >= desiredWidth)
-    }.orElse(image.largestImage)
+  private def toSrc(maybeAsset: Option[ImageAsset]): Option[String] =
+    maybeAsset.flatMap(_.url).map(ImgSrc(_, this))
+
+  def bestFor(image: ImageMedia): Option[ImageAsset] = {
+    if(ImageServerSwitch.isSwitchedOn) {
+      val sortedCrops = image.imageCrops.sortBy(-_.width)
+      width.flatMap{ desiredWidth =>
+        sortedCrops.find(_.width >= desiredWidth)
+      }.orElse(image.largestImage)
+    }
+    else image.largestImage
   }
+  def bestSrcFor(image: ImageMedia): Option[String] = toSrc(bestFor(image))
 
   def largestFor(image: ImageMedia): Option[ImageAsset] = image.largestImage
-
-  def bestFor(image: ImageMedia): Option[String] =
-    elementFor(image).flatMap(_.url).map{ url => ImgSrc(url, this) }
+  def largestSrcFor(image: ImageMedia): Option[String] = toSrc(largestFor(image))
 
   def captionFor(image: ImageMedia): Option[String] =
-    elementFor(image).flatMap(_.caption)
+    bestFor(image).flatMap(_.caption)
 
   def altTextFor(image: ImageMedia): Option[String] =
-    elementFor(image).flatMap(_.altText)
+    bestFor(image).flatMap(_.altText)
 
   // NOTE - if you modify this in any way there is a decent chance that you decache all our images :(
   val qualityparam = if (hidpi) {"q=20"} else {"q=55"}
@@ -100,8 +105,6 @@ object Item1200 extends Profile(width = Some(1200))
 object Video640 extends VideoProfile(width = Some(640), height = Some(360)) // 16:9
 object Video700 extends VideoProfile(width = Some(700), height = Some(394)) // 16:9
 object Video1280 extends VideoProfile(width = Some(1280), height = Some(720)) // 16:9
-
-
 object GoogleStructuredData extends Profile(width = Some(300), height = Some(300)) // 1:1
 
 abstract class ShareImage(shouldIncludeOverlay: Boolean) extends Profile(width = Some(1200)) {
@@ -202,18 +205,6 @@ object ImgSrc extends Logging with implicits.Strings {
     }
   }
 
-  def findNearestSrc(ImageElement: ImageMedia, profile: Profile): Option[String] = {
-    profile.elementFor(ImageElement).flatMap(_.url).map{ largestImage =>
-      ImgSrc(largestImage, profile)
-    }
-  }
-
-  def findLargestSrc(ImageElement: ImageMedia, profile: Profile): Option[String] = {
-    profile.largestFor(ImageElement).flatMap(_.url).map{ largestImage =>
-      ImgSrc(largestImage, profile)
-    }
-  }
-
   def srcset(imageContainer: ImageMedia, widths: WidthsByBreakpoint): String = {
     widths.profiles.map { profile => srcsetForProfile(profile, imageContainer, hidpi = false) } mkString ", "
   }
@@ -231,38 +222,25 @@ object ImgSrc extends Logging with implicits.Strings {
       .mkString(", ")
   }
 
-  def srcsetForProfile(profile: Profile, imageContainer: ImageMedia, hidpi: Boolean): String = {
-    if(ImageServerSwitch.isSwitchedOn) {
-      s"${findLargestSrc(imageContainer, profile).get} ${profile.width.get * (if (hidpi) 2 else 1)}w"
-    } else {
-      s"${findNearestSrc(imageContainer, profile).get} ${profile.width.get * (if (hidpi) 2 else 1)}w"
-    }
-  }
+  def srcsetForProfile(profile: Profile, imageContainer: ImageMedia, hidpi: Boolean): String =
+    s"${profile.bestSrcFor(imageContainer).get} ${profile.width.get * (if (hidpi) 2 else 1)}w"
 
-  def srcsetForProfile(profile: Profile, path: String, hidpi: Boolean): String = {
+  def srcsetForProfile(profile: Profile, path: String, hidpi: Boolean): String =
     s"${ImgSrc(path, profile)} ${profile.width.get * (if (hidpi) 2 else 1)}w"
-  }
 
-  def getFallbackUrl(ImageElement: ImageMedia): Option[String] = {
-    if(ImageServerSwitch.isSwitchedOn) {
-      findLargestSrc(ImageElement, Item300)
-    } else {
-      findNearestSrc(ImageElement, Item300)
-    }
-  }
+  def getFallbackUrl(ImageElement: ImageMedia): Option[String] =
+    Item300.bestSrcFor(ImageElement)
 
-  def getAmpImageUrl(ImageElement: ImageMedia): Option[String] = {
-    findNearestSrc(ImageElement, Item620)
-  }
+  def getAmpImageUrl(ImageElement: ImageMedia): Option[String] =
+    Item620.bestSrcFor(ImageElement)
 
-  def getFallbackAsset(ImageElement: ImageMedia): Option[ImageAsset] = {
-    Item300.elementFor(ImageElement)
-  }
+  def getFallbackAsset(ImageElement: ImageMedia): Option[ImageAsset] =
+    Item300.bestFor(ImageElement)
 }
 
 object SeoThumbnail {
   def apply(page: Page): Option[String] = page match {
-    case content: ContentPage => content.item.elements.thumbnail.flatMap(thumbnail => Item620.bestFor(thumbnail.images))
+    case content: ContentPage => content.item.elements.thumbnail.flatMap(thumbnail => Item620.bestSrcFor(thumbnail.images))
     case _ => None
   }
 }
diff --git a/common/app/views/support/cleaner/VideoEmbedCleaner.scala b/common/app/views/support/cleaner/VideoEmbedCleaner.scala
@@ -69,9 +69,7 @@ case class VideoEmbedCleaner(article: Article) extends HtmlCleaner {
         element.getElementsByTag("source").remove()
 
         // add the poster url
-        video.map(_.images).flatMap(Item640.bestFor).map(_.toString()).foreach { url =>
-          element.attr("poster", url)
-        }
+        video.map(_.images).flatMap(Item640.bestSrcFor).foreach(element.attr("poster", _))
 
         video.foreach { videoElement =>
           videoElement.videos.encodings.map { encoding => {