Get missing crops from configuration

[cleversolutions]

Prerequisites

Replaces pull request #5739

Description

When you add a crop alias to Image Cropper after media already exists in your Media library it doesn't pick up any of the crops. This PR updates ImageCropperTemplateExtensions to check for this condition and configure crops from the DataType.Configuration.

Steps to reproduce

As per issue #5737

• V8 with SK installed
• Update Image Media Type -> Update umbracoFile Image Cropper DataType settings to include a crop type (ie test)
• Update Products.cshtml / template to use that crop ie @product.Pho0tos.GetCropUrl("test")
• View page on frontend & confirm images render (they don't before PR)
• Goto Media Section & Choose a Product Image
• Set manual crops for an image
• View page on frontend & confirm all images render (only the updated image does before PR)

Fixes #5737

[cleversolutions]

@Shazwazza I somehow ruined #5739 when updating my local. Based on your comments on #5739 I took a look at ImageCropperValue and found a very simple solution.

[nul800sebastiaan]

Hi there @cleversolutions,

First of all: A big #H5YR for contributing to Umbraco during Hacktoberfest! We are very thankful for the huge amount of PRs submitted, and all the amazing work you've been doing 🥇

We have had another record breaking amount of PR's in both the CMS, Documentation, and other smaller public repositories. You can read all about the numbers in the Umbraco Hacktoberfest round-up blog post.

Due to the amazing work you and others in the community have been doing, we've had a bit of a hard time keeping up. 😅 While all of the PRs for Hacktoberfest might not have been merged yet, you still qualify for receiving some Umbraco swag, congratulations! 🎉

In the spirit of Hacktoberfest we've prepared some exclusive Umbraco swag for all our contributors - including you!

Receive your swag! 👈 Please follow this link to fill out and submit the form, before December 19th 2019.

Following this date we'll be sending out all the swag, which also means that it might not reach your doorstep before February, so please bear with us and be patient 🙏

The only thing left to say is thank you so much for participating in Hacktoberfest! We really appreciate the help!

Kind regards,
The Umbraco PR Team & The Documentation Curators

[kjac]

@cleversolutions I can't reproduce this... see my comment here

[cleversolutions]

@kjac I can still reproduce the problem. I've replied to your comment #5737 (comment) with some screenshots.

[ronaldbarendse]

I can verify that after adding the first crop, you need to re-save the item with the 'Image Cropper' data type (used by default on the Image media type, so you'd need to re-save the media item).

This is because the stored property value is a JSON version of ImageCropperValue that doesn't yet contain any crops, so when deserialized, the Crops property is null and the configured crops are not added/applied:

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

Lines 155 to 192 in c63af8e

	internal void ApplyConfiguration(ImageCropperConfiguration configuration)
	{
	// merge the crop values - the alias + width + height comes from
	// configuration, but each crop can store its own coordinates
	if (Crops == null) return;
	var configuredCrops = configuration?.Crops;
	if (configuredCrops == null) return;
	var crops = Crops.ToList();
	foreach (var configuredCrop in configuredCrops)
	{
	var crop = crops.FirstOrDefault(x => x.Alias == configuredCrop.Alias);
	if (crop != null)
	{
	// found, apply the height & width
	crop.Width = configuredCrop.Width;
	crop.Height = configuredCrop.Height;
	}
	else
	{
	// not found, add
	crops.Add(new ImageCropperCrop
	{
	Alias = configuredCrop.Alias,
	Width = configuredCrop.Width,
	Height = configuredCrop.Height
	});
	}
	}
	// assume we don't have to remove the crops in value, that
	// are not part of configuration anymore?
	Crops = crops;
	}

This PR ensures the crops from the configuration are used, by not short-circuiting and using an empty crop list instead. This means the ImageCropperValueConverter will always get all configured crops, even when none are yet saved in the property value:

Umbraco-CMS/src/Umbraco.Core/PropertyEditors/ValueConverters/ImageCropperValueConverter.cs

Lines 29 to 53 in 23f8c3e

	public override object ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object source, bool preview)
	{
	if (source == null) return null;
	var sourceString = source.ToString();
	ImageCropperValue value;
	try
	{
	value = JsonConvert.DeserializeObject<ImageCropperValue>(sourceString, new JsonSerializerSettings
	{
	Culture = CultureInfo.InvariantCulture,
	FloatParseHandling = FloatParseHandling.Decimal
	});
	}
	catch (Exception ex)
	{
	// cannot deserialize, assume it may be a raw image url
	Current.Logger.Error<ImageCropperValueConverter>(ex, "Could not deserialize string '{JsonString}' into an image cropper value.", sourceString);
	value = new ImageCropperValue { Src = sourceString };
	}
	value?.ApplyConfiguration(propertyType.DataType.ConfigurationAs<ImageCropperConfiguration>());
	return value;
	}

BTW: re-saving works, because the AngularJS controller adds the configured crops when the editor is loaded in the back-office:

Umbraco-CMS/src/Umbraco.Web.UI.Client/src/views/propertyeditors/imagecropper/imagecropper.controller.js

Lines 102 to 110 in bd26cb3

	//sync any config changes with the editor and drop outdated crops
	_.each($scope.model.value.crops, function (saved) {
	var configured = _.find(config.crops, function (item) { return item.alias === saved.alias });
	if (configured && configured.height === saved.height && configured.width === saved.width) {
	configured.coordinates = saved.coordinates;
	}
	});
	$scope.model.value.crops = config.crops;

The ImageCropperPropertyValueEditor also tries to add the configured crops before returning the ImageCropperValue to the editor, but this has the same problem. This also makes it redundant, as it's also done within the AngularJS controller (as you can see above):

Umbraco-CMS/src/Umbraco.Web/PropertyEditors/ImageCropperPropertyValueEditor.cs

Lines 35 to 55 in ef19633

	public override object ToEditor(Property property, IDataTypeService dataTypeService, string culture = null, string segment = null)
	{
	var val = property.GetValue(culture, segment);
	if (val == null) return null;
	ImageCropperValue value;
	try
	{
	value = JsonConvert.DeserializeObject<ImageCropperValue>(val.ToString());
	}
	catch
	{
	value = new ImageCropperValue { Src = val.ToString() };
	}
	var dataType = dataTypeService.GetDataType(property.PropertyType.DataTypeId);
	if (dataType?.Configuration != null)
	value.ApplyConfiguration(dataType.ConfigurationAs<ImageCropperConfiguration>());
	return value;
	}

[nul800sebastiaan]

@ronaldbarendse Thanks! as you mention:

This means the ImageCropperValueConverter will always get all configured crops, even when none are yet saved in the property value

This sounds like a breaking change in behavior to me, which we want to avoid.

What do you suggest?

[ronaldbarendse]

@nul800sebastiaan This isn't a breaking change, as it's already correctly merging all configured crops when the saved crops aren't null (e.g. one or more)... And just re-saving the affected item from the back-office will make it return the same value.

Because there's logic on the server and client-side that do the same thing, but just slightly different, this PR corrects that and is a really great bugfix (that should be included in the next release ASAP).

[ronaldbarendse]

I would suggest also removing the AngularJS logic for merging the configured crops BTW, as that's now redundant (it's already done in the editors IValueEditor). Maybe even remove saved crops that aren't configured anymore and ensure empty crops aren't saved to begin with, both to minimize the data that needs to be saved.

[ronaldbarendse]

I've tested the current behavior (version 8.6.1) a little bit more using the following Razor code on an Image media item that's dragged into the media library:

SourceValue: @Json.Encode(image.GetProperty("umbracoFile").GetSourceValue())
ImageCropperValue: @Json.Encode(image.UmbracoFile)
Value: @Json.Encode(image.Value<string>("umbracoFile"))

With zero configured crops, you can see the source value/saved data is already a JSON object containing an empty crops array (but this might be different on media created in older versions) and the string version is automatically converted to the image URL:

SourceValue: "{\"src\":\"/media/yw3jcos3/test.png\",\"crops\":[]}"
ImageCropperValue: {"Src":"/media/yw3jcos3/test.png","FocalPoint":null,"Crops":[]}
Value: "/media/yw3jcos3/test.png"

When this media item is opened in the back-office and saved, the focal point is added (and the saved data now also contains unnecessary white-space):

SourceValue: "{\r\n  \"src\": \"/media/yw3jcos3/test.png\",\r\n  \"focalPoint\": {\r\n    \"left\": 0.5,\r\n    \"top\": 0.5\r\n  },\r\n  \"crops\": []\r\n}"
ImageCropperValue: {"Src":"/media/yw3jcos3/test.png","FocalPoint":{"Left":0.5,"Top":0.5},"Crops":[]}
Value: "/media/yw3jcos3/test.png"

After adding the first 'Header' crop to the Image Cropper configuration, the ImageCropperValueConverter automatically adds this to the value, even though the saved value doesn't contain this crop:

SourceValue: "{\r\n  \"src\": \"/media/yw3jcos3/test.png\",\r\n  \"focalPoint\": {\r\n    \"left\": 0.5,\r\n    \"top\": 0.5\r\n  },\r\n  \"crops\": []\r\n}"
ImageCropperValue: {"Src":"/media/yw3jcos3/test.png","FocalPoint":{"Left":0.5,"Top":0.5},"Crops":[{"Alias":"Header","Width":1920,"Height":700,"Coordinates":null}]}
Value: "{\"src\":\"/media/yw3jcos3/test.png\",\"focalPoint\":{\"left\":0.5,\"top\":0.5},\"crops\":[{\"alias\":\"Header\",\"width\":1920,\"height\":700,\"coordinates\":null}]}"

After saving the media item again, the crop is also added to the saved value (changing the configured crop dimensions won't change this value by now):

SourceValue: "{\r\n  \"src\": \"/media/yw3jcos3/test.png\",\r\n  \"focalPoint\": {\r\n    \"left\": 0.5,\r\n    \"top\": 0.5\r\n  },\r\n  \"crops\": [\r\n    {\r\n      \"alias\": \"Header\",\r\n      \"width\": 1920,\r\n      \"height\": 700,\r\n      \"coordinates\": null\r\n    }\r\n  ]\r\n}"
ImageCropperValue: {"Src":"/media/yw3jcos3/test.png","FocalPoint":{"Left":0.5,"Top":0.5},"Crops":[{"Alias":"Header","Width":1920,"Height":700,"Coordinates":null}]}
Value: "{\"src\":\"/media/yw3jcos3/test.png\",\"focalPoint\":{\"left\":0.5,\"top\":0.5},\"crops\":[{\"alias\":\"Header\",\"width\":1920,\"height\":700,\"coordinates\":null}]}"

After removing the crop from the configuration, none of the values change, as the saved value is unaltered:

SourceValue: "{\r\n  \"src\": \"/media/yw3jcos3/test.png\",\r\n  \"focalPoint\": {\r\n    \"left\": 0.5,\r\n    \"top\": 0.5\r\n  },\r\n  \"crops\": [\r\n    {\r\n      \"alias\": \"Header\",\r\n      \"width\": 1920,\r\n      \"height\": 700,\r\n      \"coordinates\": null\r\n    }\r\n  ]\r\n}"
ImageCropperValue: {"Src":"/media/yw3jcos3/test.png","FocalPoint":{"Left":0.5,"Top":0.5},"Crops":[{"Alias":"Header","Width":1920,"Height":700,"Coordinates":null}]}
Value: "{\"src\":\"/media/yw3jcos3/test.png\",\"focalPoint\":{\"left\":0.5,\"top\":0.5},\"crops\":[{\"alias\":\"Header\",\"width\":1920,\"height\":700,\"coordinates\":null}]}"

After saving the media item without any configured crops, it returns back to:

SourceValue: "{\r\n  \"src\": \"/media/yw3jcos3/test.png\",\r\n  \"focalPoint\": {\r\n    \"left\": 0.5,\r\n    \"top\": 0.5\r\n  },\r\n  \"crops\": []\r\n}"
ImageCropperValue: {"Src":"/media/yw3jcos3/test.png","FocalPoint":{"Left":0.5,"Top":0.5},"Crops":[]}
Value: "/media/yw3jcos3/test.png"

---

So it's clear the saved data can already differ based on the way it's added (or more specifically whether the value is set from the server of by the Image Cropper AngularJS code). There might be code that doesn't set the crops to an empty array (e.g. when created via the RTE, media picker or batch/folder upload), causing issue #5737.

Besides this PR, I think we should make sure the following is also fixed:

• Do not save any unnecessary white-space
• Remove properties with null values or even if it's the default value (e.g. a focal point of 0.5,0.5 or an empty crop array) from the saved data
• Remove crops from the saved data that aren't configured anymore (the ApplyConfiguration method contains a comment about this)
• Do not save crops without coordinates, so the crop dimensions can be updated without re-saving the media item

[nul800sebastiaan]

Yikes! I thought I had long since merged this one, apologies for the delay @cleversolutions! All good! 👍

@ronaldbarendse Those updates sound good to me!

[cleversolutions]

👍

[nul800sebastiaan]

@cleversolutions Oh! I just noticed this is your first merged PR for Umbraco-CMS! 🎉 Congrats! You've earned your contrib badge on Our with this one as well: https://our.umbraco.com/members/cleversolutions 🏅

Great work, thanks again!