From 75bb8051bff84e3cc2e9da2379d696f02d7a8845 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Wed, 5 Jan 2022 11:11:27 +0100 Subject: [PATCH] Prune Image Cropper and Media Picker (v3) values (#11805) * Clean up redundant/default Umbraco.ImageCropper data * Fix ToString() and add HasCrops() method * Re-use crop/focal point pruning for Umbraco.MediaPicker3 * Fix ImageCropperTest Co-authored-by: Elitsa Marinovska --- .../ValueConverters/ImageCropperValue.cs | 88 ++++++++++++++----- .../PropertyEditors/ImageCropperTest.cs | 6 +- .../ImageCropperPropertyValueEditor.cs | 39 +++++--- .../MediaPicker3PropertyEditor.cs | 81 ++++++++++++++++- 4 files changed, 174 insertions(+), 40 deletions(-) diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/ImageCropperValue.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/ImageCropperValue.cs index f2151778d9ce..555c198f7df1 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/ImageCropperValue.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/ImageCropperValue.cs @@ -1,12 +1,11 @@ using System; using System.Collections.Generic; using System.ComponentModel; -using System.Globalization; using System.Linq; using System.Runtime.Serialization; -using System.Text; using System.Web; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using Umbraco.Core.Composing; using Umbraco.Core.Models; using Umbraco.Core.Serialization; @@ -18,14 +17,14 @@ namespace Umbraco.Core.PropertyEditors.ValueConverters /// [JsonConverter(typeof(NoTypeConverterJsonConverter))] [TypeConverter(typeof(ImageCropperValueTypeConverter))] - [DataContract(Name="imageCropDataSet")] + [DataContract(Name = "imageCropDataSet")] public class ImageCropperValue : IHtmlString, IEquatable { /// /// Gets or sets the value source image. /// - [DataMember(Name="src")] - public string Src { get; set;} + [DataMember(Name = "src")] + public string Src { get; set; } /// /// Gets or sets the value focal point. @@ -41,9 +40,7 @@ public class ImageCropperValue : IHtmlString, IEquatable /// public override string ToString() - { - return Crops != null ? (Crops.Any() ? JsonConvert.SerializeObject(this) : Src) : string.Empty; - } + => HasCrops() || HasFocalPoint() ? JsonConvert.SerializeObject(this, Formatting.None) : Src; /// public string ToHtmlString() => Src; @@ -134,13 +131,19 @@ public string GetCropUrl(int width, int height, IImageUrlGenerator imageUrlGener /// /// public bool HasFocalPoint() - => FocalPoint != null && (FocalPoint.Left != 0.5m || FocalPoint.Top != 0.5m); + => FocalPoint is ImageCropperFocalPoint focalPoint && (focalPoint.Left != 0.5m || focalPoint.Top != 0.5m); + + /// + /// Determines whether the value has crops. + /// + public bool HasCrops() + => Crops is IEnumerable crops && crops.Any(); /// /// Determines whether the value has a specified crop. /// public bool HasCrop(string alias) - => Crops != null && Crops.Any(x => x.Alias == alias); + => Crops is IEnumerable crops && crops.Any(x => x.Alias == alias); /// /// Determines whether the value has a source image. @@ -179,6 +182,51 @@ internal ImageCropperValue Merge(ImageCropperValue imageCropperValue) }; } + /// + /// Removes redundant crop data/default focal point. + /// + /// The image cropper value. + /// + /// The cleaned up value. + /// + public static void Prune(JObject value) + { + if (value is null) throw new ArgumentNullException(nameof(value)); + + if (value.TryGetValue("crops", out var crops)) + { + if (crops.HasValues) + { + foreach (var crop in crops.Values().ToList()) + { + if (crop.TryGetValue("coordinates", out var coordinates) == false || coordinates.HasValues == false) + { + // Remove crop without coordinates + crop.Remove(); + continue; + } + + // Width/height are already stored in the crop configuration + crop.Remove("width"); + crop.Remove("height"); + } + } + + if (crops.HasValues == false) + { + // Remove empty crops + value.Remove("crops"); + } + } + + if (value.TryGetValue("focalPoint", out var focalPoint) && + (focalPoint.HasValues == false || (focalPoint.Value("top") == 0.5m && focalPoint.Value("left") == 0.5m))) + { + // Remove empty/default focal point + value.Remove("focalPoint"); + } + } + #region IEquatable /// @@ -212,8 +260,8 @@ public override int GetHashCode() // properties are, practically, readonly // ReSharper disable NonReadonlyMemberInGetHashCode var hashCode = Src?.GetHashCode() ?? 0; - hashCode = (hashCode*397) ^ (FocalPoint?.GetHashCode() ?? 0); - hashCode = (hashCode*397) ^ (Crops?.GetHashCode() ?? 0); + hashCode = (hashCode * 397) ^ (FocalPoint?.GetHashCode() ?? 0); + hashCode = (hashCode * 397) ^ (Crops?.GetHashCode() ?? 0); return hashCode; // ReSharper restore NonReadonlyMemberInGetHashCode } @@ -258,7 +306,7 @@ public override int GetHashCode() { // properties are, practically, readonly // ReSharper disable NonReadonlyMemberInGetHashCode - return (Left.GetHashCode()*397) ^ Top.GetHashCode(); + return (Left.GetHashCode() * 397) ^ Top.GetHashCode(); // ReSharper restore NonReadonlyMemberInGetHashCode } } @@ -312,9 +360,9 @@ public override int GetHashCode() // properties are, practically, readonly // ReSharper disable NonReadonlyMemberInGetHashCode var hashCode = Alias?.GetHashCode() ?? 0; - hashCode = (hashCode*397) ^ Width; - hashCode = (hashCode*397) ^ Height; - hashCode = (hashCode*397) ^ (Coordinates?.GetHashCode() ?? 0); + hashCode = (hashCode * 397) ^ Width; + hashCode = (hashCode * 397) ^ Height; + hashCode = (hashCode * 397) ^ (Coordinates?.GetHashCode() ?? 0); return hashCode; // ReSharper restore NonReadonlyMemberInGetHashCode } @@ -339,7 +387,7 @@ public class ImageCropperCropCoordinates : IEquatable public bool Equals(ImageCropperCropCoordinates other) => ReferenceEquals(this, other) || Equals(this, other); @@ -369,9 +417,9 @@ public override int GetHashCode() // properties are, practically, readonly // ReSharper disable NonReadonlyMemberInGetHashCode var hashCode = X1.GetHashCode(); - hashCode = (hashCode*397) ^ Y1.GetHashCode(); - hashCode = (hashCode*397) ^ X2.GetHashCode(); - hashCode = (hashCode*397) ^ Y2.GetHashCode(); + hashCode = (hashCode * 397) ^ Y1.GetHashCode(); + hashCode = (hashCode * 397) ^ X2.GetHashCode(); + hashCode = (hashCode * 397) ^ Y2.GetHashCode(); return hashCode; // ReSharper restore NonReadonlyMemberInGetHashCode } diff --git a/src/Umbraco.Tests/PropertyEditors/ImageCropperTest.cs b/src/Umbraco.Tests/PropertyEditors/ImageCropperTest.cs index c40708770e05..eed45b0d2715 100644 --- a/src/Umbraco.Tests/PropertyEditors/ImageCropperTest.cs +++ b/src/Umbraco.Tests/PropertyEditors/ImageCropperTest.cs @@ -29,13 +29,13 @@ public class ImageCropperTest { private const string CropperJson1 = "{\"focalPoint\": {\"left\": 0.96,\"top\": 0.80827067669172936},\"src\": \"/media/1005/img_0671.jpg\",\"crops\": [{\"alias\":\"thumb\",\"width\": 100,\"height\": 100,\"coordinates\": {\"x1\": 0.58729977382575338,\"y1\": 0.055768992440203169,\"x2\": 0,\"y2\": 0.32457553600198386}}]}"; private const string CropperJson2 = "{\"focalPoint\": {\"left\": 0.98,\"top\": 0.80827067669172936},\"src\": \"/media/1005/img_0672.jpg\",\"crops\": [{\"alias\":\"thumb\",\"width\": 100,\"height\": 100,\"coordinates\": {\"x1\": 0.58729977382575338,\"y1\": 0.055768992440203169,\"x2\": 0,\"y2\": 0.32457553600198386}}]}"; - private const string CropperJson3 = "{\"focalPoint\": {\"left\": 0.98,\"top\": 0.80827067669172936},\"src\": \"/media/1005/img_0672.jpg\",\"crops\": []}"; + private const string CropperJson3 = "{\"focalPoint\": {\"left\": 0.5,\"top\": 0.5},\"src\": \"/media/1005/img_0672.jpg\",\"crops\": []}"; private const string MediaPath = "/media/1005/img_0671.jpg"; [Test] public void CanConvertImageCropperDataSetSrcToString() { - //cropperJson3 - has not crops + //cropperJson3 - has no crops var cropperValue = CropperJson3.DeserializeImageCropperValue(); var serialized = cropperValue.TryConvertTo(); Assert.IsTrue(serialized.Success); @@ -45,7 +45,7 @@ public void CanConvertImageCropperDataSetSrcToString() [Test] public void CanConvertImageCropperDataSetJObject() { - //cropperJson3 - has not crops + //cropperJson3 - has no crops var cropperValue = CropperJson3.DeserializeImageCropperValue(); var serialized = cropperValue.TryConvertTo(); Assert.IsTrue(serialized.Success); diff --git a/src/Umbraco.Web/PropertyEditors/ImageCropperPropertyValueEditor.cs b/src/Umbraco.Web/PropertyEditors/ImageCropperPropertyValueEditor.cs index 4aac8f54aa03..8e13d1bb5ac4 100644 --- a/src/Umbraco.Web/PropertyEditors/ImageCropperPropertyValueEditor.cs +++ b/src/Umbraco.Web/PropertyEditors/ImageCropperPropertyValueEditor.cs @@ -1,6 +1,6 @@ -using Newtonsoft.Json.Linq; -using System; +using System; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using Umbraco.Core; using Umbraco.Core.IO; using Umbraco.Core.Logging; @@ -66,31 +66,42 @@ public override object ToEditor(Property property, IDataTypeService dataTypeServ /// public override object FromEditor(ContentPropertyData editorValue, object currentValue) { - // get the current path + // Get the current path var currentPath = string.Empty; try { var svalue = currentValue as string; var currentJson = string.IsNullOrWhiteSpace(svalue) ? null : JObject.Parse(svalue); - if (currentJson != null && currentJson["src"] != null) - currentPath = currentJson["src"].Value(); + if (currentJson != null && currentJson.TryGetValue("src", out var src)) + { + currentPath = src.Value(); + } } catch (Exception ex) { - // for some reason the value is invalid so continue as if there was no value there + // For some reason the value is invalid, so continue as if there was no value there _logger.Warn(ex, "Could not parse current db value to a JObject."); } + if (string.IsNullOrWhiteSpace(currentPath) == false) currentPath = _mediaFileSystem.GetRelativePath(currentPath); - // get the new json and path - JObject editorJson = null; + // Get the new JSON and file path var editorFile = string.Empty; - if (editorValue.Value != null) + if (editorValue.Value is JObject editorJson) { - editorJson = editorValue.Value as JObject; - if (editorJson != null && editorJson["src"] != null) + // Populate current file + if (editorJson["src"] != null) + { editorFile = editorJson["src"].Value(); + } + + // Clean up redundant/default data + ImageCropperValue.Prune(editorJson); + } + else + { + editorJson = null; } // ensure we have the required guids @@ -118,7 +129,7 @@ public override object FromEditor(ContentPropertyData editorValue, object curren return null; // clear } - return editorJson?.ToString(); // unchanged + return editorJson?.ToString(Formatting.None); // unchanged } // process the file @@ -135,7 +146,8 @@ public override object FromEditor(ContentPropertyData editorValue, object curren // update json and return if (editorJson == null) return null; editorJson["src"] = filepath == null ? string.Empty : _mediaFileSystem.GetUrl(filepath); - return editorJson.ToString(); + + return editorJson.ToString(Formatting.None); } private string ProcessFile(ContentPropertyData editorValue, ContentPropertyFile file, string currentPath, Guid cuid, Guid puid) @@ -160,7 +172,6 @@ private string ProcessFile(ContentPropertyData editorValue, ContentPropertyFile return filepath; } - public override string ConvertDbToString(PropertyType propertyType, object value, IDataTypeService dataTypeService) { if (value == null || string.IsNullOrEmpty(value.ToString())) diff --git a/src/Umbraco.Web/PropertyEditors/MediaPicker3PropertyEditor.cs b/src/Umbraco.Web/PropertyEditors/MediaPicker3PropertyEditor.cs index 43d190e17330..5f1b319e0113 100644 --- a/src/Umbraco.Web/PropertyEditors/MediaPicker3PropertyEditor.cs +++ b/src/Umbraco.Web/PropertyEditors/MediaPicker3PropertyEditor.cs @@ -1,8 +1,9 @@ -using Newtonsoft.Json; -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Runtime.Serialization; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using Umbraco.Core; using Umbraco.Core.Logging; using Umbraco.Core.Models; @@ -50,7 +51,36 @@ public override object ToEditor(Property property, IDataTypeService dataTypeServ { var value = property.GetValue(culture, segment); - return Deserialize(value); + var dtos = Deserialize(value).ToList(); + + var dataType = dataTypeService.GetDataType(property.PropertyType.DataTypeId); + if (dataType?.Configuration != null) + { + var configuration = dataType.ConfigurationAs(); + + foreach (var dto in dtos) + { + dto.ApplyConfiguration(configuration); + } + } + + return dtos; + } + + public override object FromEditor(ContentPropertyData editorValue, object currentValue) + { + if (editorValue.Value is JArray dtos) + { + // Clean up redundant/default data + foreach (var dto in dtos.Values()) + { + MediaWithCropsDto.Prune(dto); + } + + return dtos.ToString(Formatting.None); + } + + return base.FromEditor(editorValue, currentValue); } public IEnumerable GetReferences(object value) @@ -117,6 +147,51 @@ internal class MediaWithCropsDto [DataMember(Name = "focalPoint")] public ImageCropperValue.ImageCropperFocalPoint FocalPoint { get; set; } + + /// + /// Applies the configuration to ensure only valid crops are kept and have the correct width/height. + /// + /// The configuration. + public void ApplyConfiguration(MediaPicker3Configuration configuration) + { + var crops = new List(); + + var configuredCrops = configuration?.Crops; + if (configuredCrops != null) + { + foreach (var configuredCrop in configuredCrops) + { + var crop = Crops?.FirstOrDefault(x => x.Alias == configuredCrop.Alias); + + crops.Add(new ImageCropperValue.ImageCropperCrop + { + Alias = configuredCrop.Alias, + Width = configuredCrop.Width, + Height = configuredCrop.Height, + Coordinates = crop?.Coordinates + }); + } + } + + Crops = crops; + + if (configuration?.EnableLocalFocalPoint == false) + { + FocalPoint = null; + } + } + + /// + /// Removes redundant crop data/default focal point. + /// + /// The media with crops DTO. + /// + /// The cleaned up value. + /// + /// + /// Because the DTO uses the same JSON keys as the image cropper value for crops and focal point, we can re-use the prune method. + /// + public static void Prune(JObject value) => ImageCropperValue.Prune(value); } } }