Improvements to media pickers/crop handling and URL generation

src/Umbraco.Core/Models/MediaWithCrops.cs

@@ -1,15 +1,86 @@
+using System;
 using Umbraco.Core.Models.PublishedContent;
 using Umbraco.Core.PropertyEditors.ValueConverters;
 
 namespace Umbraco.Core.Models
 {
     /// <summary>
-    /// Model used in Razor Views for rendering
+    /// Represents a media item with local crops.
     /// </summary>
-    public class MediaWithCrops
+    /// <seealso cref="Umbraco.Core.Models.PublishedContent.PublishedContentWrapped" />
+    public class MediaWithCrops : PublishedContentWrapped
     {
-        public IPublishedContent MediaItem { get; set; }
+        /// <summary>
+        /// Gets the media item.
+        /// </summary>
+        /// <value>
+        /// The media item.
+        /// </value>
+        [Obsolete("This instance now implements IPublishedContent by wrapping the media item, use the extension methods directly on MediaWithCrops or use the Content property to get the media item instead.")]
+        public IPublishedContent MediaItem => Content;
 
-        public ImageCropperValue LocalCrops { get; set; }
+        /// <summary>
+        /// Gets the content/media item.
+        /// </summary>
+        /// <value>
+        /// The content/media item.
+        /// </value>
+        public IPublishedContent Content => Unwrap();
+
+        /// <summary>
+        /// Gets the local crops.
+        /// </summary>
+        /// <value>
+        /// The local crops.
+        /// </value>
+        public ImageCropperValue LocalCrops { get; }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="MediaWithCrops" /> class.
+        /// </summary>
+        /// <param name="content">The content.</param>
+        /// <param name="localCrops">The local crops.</param>
+        public MediaWithCrops(IPublishedContent content, ImageCropperValue localCrops)
+            : base(content)
+        {
+            LocalCrops = localCrops;
+        }
+    }
+
+    /// <summary>
+    /// Represents a media item with local crops.
+    /// </summary>
+    /// <typeparam name="T">The type of the media item.</typeparam>
+    /// <seealso cref="Umbraco.Core.Models.PublishedContent.PublishedContentWrapped" />
+    public class MediaWithCrops<T> : MediaWithCrops
+        where T : IPublishedContent
+    {
+        /// <summary>
+        /// Gets the media item.
+        /// </summary>
+        /// <value>
+        /// The media item.
+        /// </value>
+        public new T Content { get; }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="MediaWithCrops{T}" /> class.
+        /// </summary>
+        /// <param name="content">The content.</param>
+        /// <param name="localCrops">The local crops.</param>
+        public MediaWithCrops(T content, ImageCropperValue localCrops)
+            : base(content, localCrops)
+        {
+            Content = content;
+        }
+
+        /// <summary>
+        /// Performs an implicit conversion from <see cref="MediaWithCrops{T}" /> to <see cref="T" />.
+        /// </summary>
+        /// <param name="mediaWithCrops">The media with crops.</param>
+        /// <returns>
+        /// The result of the conversion.
+        /// </returns>
+        public static implicit operator T(MediaWithCrops<T> mediaWithCrops) => mediaWithCrops.Content;
     }
 }

src/Umbraco.Core/PropertyEditors/ImageCropperConfiguration.cs

@@ -1,4 +1,8 @@
 using Newtonsoft.Json;
+using System.Collections.Generic;
+using System.Linq;
+using Umbraco.Core.PropertyEditors.ValueConverters;
+using static Umbraco.Core.PropertyEditors.ValueConverters.ImageCropperValue;
 
 namespace Umbraco.Core.PropertyEditors
 {
@@ -22,4 +26,35 @@ public class Crop
             public int Height { get; set; }
         }
     }
+
+    internal static class ImageCropperConfigurationExtensions
+    {
+        /// <summary>
+        /// Applies the configuration to ensure only valid crops are kept and have the correct width/height.
+        /// </summary>
+        /// <param name="configuration">The configuration.</param>
+        public static void ApplyConfiguration(this ImageCropperValue imageCropperValue, ImageCropperConfiguration configuration)
+        {
+            var crops = new List<ImageCropperCrop>();
+
+            var configuredCrops = configuration?.Crops;
+            if (configuredCrops != null)
+            {
+                foreach (var configuredCrop in configuredCrops)
+                {
+                    var crop = imageCropperValue.Crops?.FirstOrDefault(x => x.Alias == configuredCrop.Alias);
+
+                    crops.Add(new ImageCropperCrop
+                    {
+                        Alias = configuredCrop.Alias,
+                        Width = configuredCrop.Width,
+                        Height = configuredCrop.Height,
+                        Coordinates = crop?.Coordinates
+                    });
+                }
+            }
+
+            imageCropperValue.Crops = crops;
+        }
+    }
 }

src/Umbraco.Core/PropertyEditors/ValueConverters/ImageCropperValue.cs

@@ -140,54 +140,43 @@ public bool HasFocalPoint()
         /// Determines whether the value has a specified crop.
         /// </summary>
         public bool HasCrop(string alias)
-            => Crops.Any(x => x.Alias == alias);
+            => Crops != null && Crops.Any(x => x.Alias == alias);
 
         /// <summary>
         /// Determines whether the value has a source image.
         /// </summary>
         public bool HasImage()
             => !string.IsNullOrWhiteSpace(Src);
 
-        /// <summary>
-        /// Applies a configuration.
-        /// </summary>
-        /// <remarks>Ensures that all crops defined in the configuration exists in the value.</remarks>
-        internal void ApplyConfiguration(ImageCropperConfiguration configuration)
+        internal ImageCropperValue Merge(ImageCropperValue imageCropperValue)
         {
-            // merge the crop values - the alias + width + height comes from
-            // configuration, but each crop can store its own coordinates
-
-            var configuredCrops = configuration?.Crops;
-            if (configuredCrops == null) return;
-
-            //Use Crops if it's not null, otherwise create a new list
             var crops = Crops?.ToList() ?? new List<ImageCropperCrop>();
 
-            foreach (var configuredCrop in configuredCrops)
+            var incomingCrops = imageCropperValue?.Crops;
+            if (incomingCrops != null)
             {
-                var crop = crops.FirstOrDefault(x => x.Alias == configuredCrop.Alias);
-                if (crop != null)
+                foreach (var incomingCrop in incomingCrops)
                 {
-                    // found, apply the height & width
-                    crop.Width = configuredCrop.Width;
-                    crop.Height = configuredCrop.Height;
-                }
-                else
-                {
-                    // not found, add
-                    crops.Add(new ImageCropperCrop
+                    var crop = crops.FirstOrDefault(x => x.Alias == incomingCrop.Alias);
+                    if (crop == null)
+                    {
+                        // Add incoming crop
+                        crops.Add(incomingCrop);
+                    }
+                    else if (crop.Coordinates == null)
                     {
-                        Alias = configuredCrop.Alias,
-                        Width = configuredCrop.Width,
-                        Height = configuredCrop.Height
-                    });
+                        // Use incoming crop coordinates
+                        crop.Coordinates = incomingCrop.Coordinates;
+                    }
                 }
             }
 
-            // assume we don't have to remove the crops in value, that
-            // are not part of configuration anymore?
-
-            Crops = crops;
+            return new ImageCropperValue()
+            {
+                Src = !string.IsNullOrWhiteSpace(Src) ? Src : imageCropperValue?.Src,
+                Crops = crops,
+                FocalPoint = FocalPoint ?? imageCropperValue?.FocalPoint
+            };
         }
 
         #region IEquatable

src/Umbraco.Tests/PropertyEditors/ImageCropperTest.cs

@@ -82,8 +82,20 @@ public void CanConvertImageCropperPropertyEditor(string val1, string val2, bool
 
                 var mediaFileSystem = new MediaFileSystem(Mock.Of<IFileSystem>(), config, scheme, logger);
 
+                var imageCropperConfiguration = new ImageCropperConfiguration()
+                {
+                    Crops = new[]
+                    {
+                        new ImageCropperConfiguration.Crop()
+                        {
+                            Alias = "thumb",
+                            Width = 100,
+                            Height = 100
+                        }
+                    }
+                };
                 var dataTypeService = new TestObjects.TestDataTypeService(
-                    new DataType(new ImageCropperPropertyEditor(Mock.Of<ILogger>(), mediaFileSystem, Mock.Of<IContentSection>(), Mock.Of<IDataTypeService>())) { Id = 1 });
+                    new DataType(new ImageCropperPropertyEditor(Mock.Of<ILogger>(), mediaFileSystem, Mock.Of<IContentSection>(), Mock.Of<IDataTypeService>())) { Id = 1, Configuration = imageCropperConfiguration });
 
                 var factory = new PublishedContentTypeFactory(Mock.Of<IPublishedModelFactory>(), new PropertyValueConverterCollection(Array.Empty<IPropertyValueConverter>()), dataTypeService);
 

src/Umbraco.Web/ImageCropperTemplateCoreExtensions.cs

@@ -28,6 +28,12 @@ public static string GetCropUrl(this IPublishedContent mediaItem, string cropAli
             return mediaItem.GetCropUrl(imageUrlGenerator, cropAlias: cropAlias, useCropDimensions: true);
         }
 
+        public static string GetCropUrl(this MediaWithCrops mediaWithCrops, string cropAlias, IImageUrlGenerator imageUrlGenerator)
+        {
+            return mediaWithCrops.GetCropUrl(imageUrlGenerator, cropAlias: cropAlias, useCropDimensions: true);
+        }
+
+        [Obsolete("This method does not get the crops or cache buster value from the media item.")]
         public static string GetCropUrl(this IPublishedContent mediaItem, string cropAlias, IImageUrlGenerator imageUrlGenerator, ImageCropperValue imageCropperValue)
         {
             return mediaItem.Url().GetCropUrl(imageUrlGenerator, imageCropperValue, cropAlias: cropAlias, useCropDimensions: true);
@@ -53,6 +59,11 @@ public static string GetCropUrl(this IPublishedContent mediaItem, string propert
             return mediaItem.GetCropUrl(imageUrlGenerator, propertyAlias: propertyAlias, cropAlias: cropAlias, useCropDimensions: true);
         }
 
+        public static string GetCropUrl(this MediaWithCrops mediaWithCrops, string propertyAlias, string cropAlias, IImageUrlGenerator imageUrlGenerator)
+        {
+            return mediaWithCrops.GetCropUrl(imageUrlGenerator, propertyAlias: propertyAlias, cropAlias: cropAlias, useCropDimensions: true);
+        }
+
         /// <summary>
         /// Gets the ImageProcessor URL from the IPublishedContent item.
         /// </summary>
@@ -122,6 +133,47 @@ public static string GetCropUrl(
              string furtherOptions = null,
              ImageCropRatioMode? ratioMode = null,
              bool upScale = true)
+        {
+            return mediaItem.GetCropUrl(imageUrlGenerator, null, width, height, propertyAlias, cropAlias, quality, imageCropMode, imageCropAnchor, preferFocalPoint, useCropDimensions, cacheBuster, furtherOptions, ratioMode, upScale);
+        }
+
+        public static string GetCropUrl(
+             this MediaWithCrops mediaWithCrops,
+             IImageUrlGenerator imageUrlGenerator,
+             int? width = null,
+             int? height = null,
+             string propertyAlias = Constants.Conventions.Media.File,
+             string cropAlias = null,
+             int? quality = null,
+             ImageCropMode? imageCropMode = null,
+             ImageCropAnchor? imageCropAnchor = null,
+             bool preferFocalPoint = false,
+             bool useCropDimensions = false,
+             bool cacheBuster = true,
+             string furtherOptions = null,
+             ImageCropRatioMode? ratioMode = null,
+             bool upScale = true)
+        {
+            return mediaWithCrops.MediaItem.GetCropUrl(imageUrlGenerator, mediaWithCrops.LocalCrops, width, height, propertyAlias, cropAlias, quality, imageCropMode, imageCropAnchor, preferFocalPoint, useCropDimensions, cacheBuster, furtherOptions, ratioMode, upScale);
+        }
+
+        private static string GetCropUrl(
+             this IPublishedContent mediaItem,
+             IImageUrlGenerator imageUrlGenerator,
+             ImageCropperValue localCrops,
+             int? width,
+             int? height,
+             string propertyAlias,
+             string cropAlias,
+             int? quality,
+             ImageCropMode? imageCropMode,
+             ImageCropAnchor? imageCropAnchor,
+             bool preferFocalPoint,
+             bool useCropDimensions,
+             bool cacheBuster,
+             string furtherOptions,
+             ImageCropRatioMode? ratioMode,
+             bool upScale)
         {
             if (mediaItem == null) throw new ArgumentNullException("mediaItem");
 
@@ -132,31 +184,38 @@ public static string GetCropUrl(
 
             var mediaItemUrl = mediaItem.MediaUrl(propertyAlias: propertyAlias);
 
-            //get the default obj from the value converter
-            var cropperValue = mediaItem.Value(propertyAlias);
-
-            //is it strongly typed?
-            var stronglyTyped = cropperValue as ImageCropperValue;
-            if (stronglyTyped != null)
+            // Only get crops when used
+            if (imageCropMode == ImageCropMode.Crop || imageCropMode == null)
             {
-                return GetCropUrl(
-                    mediaItemUrl, imageUrlGenerator, stronglyTyped, width, height, cropAlias, quality, imageCropMode, imageCropAnchor, preferFocalPoint, useCropDimensions,
-                    cacheBusterValue, furtherOptions, ratioMode, upScale);
-            }
+                // Get the default cropper value from the value converter
+                var cropperValue = mediaItem.Value(propertyAlias);
 
-            //this shouldn't be the case but we'll check
-            var jobj = cropperValue as JObject;
-            if (jobj != null)
-            {
-                stronglyTyped = jobj.ToObject<ImageCropperValue>();
-                return GetCropUrl(
-                    mediaItemUrl, imageUrlGenerator, stronglyTyped, width, height, cropAlias, quality, imageCropMode, imageCropAnchor, preferFocalPoint, useCropDimensions,
-                    cacheBusterValue, furtherOptions, ratioMode, upScale);
+                var mediaCrops = cropperValue as ImageCropperValue;
+
+                if (mediaCrops == null && cropperValue is JObject jobj)
+                {
+                    mediaCrops = jobj.ToObject<ImageCropperValue>();
+                }
+
+                if (mediaCrops == null && cropperValue is string imageCropperValue &&
+                    string.IsNullOrEmpty(imageCropperValue) == false && imageCropperValue.DetectIsJson())
+                {
+                    mediaCrops = imageCropperValue.DeserializeImageCropperValue();
+                }
+
+                // Merge crops
+                if (localCrops == null)
+                {
+                    localCrops = mediaCrops;
+                }
+                else if (mediaCrops != null)
+                {
+                    localCrops = localCrops.Merge(mediaCrops);
+                }
             }
 
-            //it's a single string
             return GetCropUrl(
-                mediaItemUrl, imageUrlGenerator, width, height, mediaItemUrl, cropAlias, quality, imageCropMode, imageCropAnchor, preferFocalPoint, useCropDimensions,
+                mediaItemUrl, imageUrlGenerator, localCrops, width, height, cropAlias, quality, imageCropMode, imageCropAnchor, preferFocalPoint, useCropDimensions,
                 cacheBusterValue, furtherOptions, ratioMode, upScale);
         }
 
@@ -237,6 +296,7 @@ public static string GetCropUrl(
             {
                 cropDataSet = imageCropperValue.DeserializeImageCropperValue();
             }
+
             return GetCropUrl(
                 imageUrl, imageUrlGenerator, cropDataSet, width, height, cropAlias, quality, imageCropMode,
                 imageCropAnchor, preferFocalPoint, useCropDimensions, cacheBusterValue, furtherOptions, ratioMode, upScale);
@@ -381,10 +441,10 @@ public static string GetCropUrl(
             return imageUrlGenerator.GetImageUrl(options);
         }
 
+        [Obsolete("Use GetCrop to merge local and media crops, get automatic cache buster value and have more parameters.")]
         public static string GetLocalCropUrl(this MediaWithCrops mediaWithCrops, string alias, IImageUrlGenerator imageUrlGenerator, string cacheBusterValue)
         {
             return mediaWithCrops.LocalCrops.Src + mediaWithCrops.LocalCrops.GetCropUrl(alias, imageUrlGenerator, cacheBusterValue: cacheBusterValue);
-
         }
     }
 }