Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to media pickers/crop handling and URL generation #10529

Merged
merged 14 commits into from
Jun 28, 2021

Conversation

ronaldbarendse
Copy link
Contributor

@ronaldbarendse ronaldbarendse commented Jun 24, 2021

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #10308, #10402 and #10397.

Description

Building upon PR #10517 that introduces backwards compatibility for the stored data format between Media Picker (legacy) (v2) and the new Media Picker (v3), this PR adds compatibility to the returned models on the front-end (and includes some additional improvements).

Model compatibility with IPublishedContent

This compatibility is added by making MediaWithCrops inherit from PublishedContentWrapped, making it an IPublishedContent instance again, so the following Razor/view code won't break by switching to the new picker:

@if (Model.Image is IPublishedContent image)
{
   <img src="@image.Url()" alt="@image.Name" />
}

One thing to note though: the concrete implementation of the IPublishedContent will change to MediaWithCrops<T>, so you can't cast directly to the Models Builder generated model (e.g. var image = (Image)Model.Image will still break). If you want the concrete media item model, you can either cast the 'wrapped' content to Image or better yet, cast to MediaWithCrops<Image> and access it using the new Content property:

@if (Model.Image?.Content is Image image)
{
    <img src="@image.Url()" width="@image.UmbracoWidth" height="@image.UmbracoHeight" alt="@image.Name" />
}

@if (Model.Image is MediaWithCrops<Image> image)
{
    // Now you still have access to the local crops (image.LocalCrops)
    <img src="@image.Url()" width="@image.Content.UmbracoWidth" height="@image.Content.UmbracoHeight" alt="@image.Name" />
}

New GetCropUrl() extension methods

Using local crops (defined on the Media Picker data type) together with media crops (defined on the Image Cropper data type) wasn't yet possible, so you'd have to ensure you're using the correct crops and aliases. This PR now merges local crops with the media ones, so the calling code can be the same for local and media crops:

@if (Model.Image is MediaWithCrops<Image> image)
{
    // Use the Hero crop either from the local or media crops
    <img src="@image.GetCropUrl("Hero")" width="@image.Content.UmbracoWidth" height="@image.Content.UmbracoHeight" alt="@image.Name" />

    // Use the Hero crop only from the local crops
    <img src="@image.GetCropUrl(image.LocalCrops, "Hero")" width="@image.Content.UmbracoWidth" height="@image.Content.UmbracoHeight" alt="@image.Name" />

    // Use the Hero crop only from the media crops
    <img src="@image.Content.GetCropUrl("Hero")" width="@image.Content.UmbracoWidth" height="@image.Content.UmbracoHeight" alt="@image.Name" />
}

In short: if the local crops doesn't contain the crop alias, it will look in the media crops. Also, if you have the focal point disabled on the Media Picker, it will fall-back to the global focal point (on the media item/Image Cropper).

Only return configured crops in the model

Because the stored data contains all crop information (as JSON), removing a crop from the configuration didn't remove existing crops from the returned model. A previous PR (#6950) already added missing crops (not in the stored data, but later added to the configuration), but because removing a local crop should now fall-back to the media crops, this caused a problem. This PR ensures crops removed from the configuration (either local crops on the Media Picker or media crops on the Image Cropper) are not returned in the model anymore, making sure you can't use outdated crops on the front-end.

I'll make sure to add some comments to the code, as it might be quite a lot to go though 😇

src/Umbraco.Core/Models/MediaWithCrops.cs Show resolved Hide resolved
src/Umbraco.Core/Models/MediaWithCrops.cs Show resolved Hide resolved
src/Umbraco.Core/Models/MediaWithCrops.cs Show resolved Hide resolved
Alias = configuredCrop.Alias,
Width = configuredCrop.Width,
Height = configuredCrop.Height,
Coordinates = crop?.Coordinates
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coordinates are actually the only data we need to store for a crop (together with the alias). We could reduce the size of all stored crop data by removing the width and height!

src/Umbraco.Web/ImageCropperTemplateExtensions.cs Outdated Show resolved Hide resolved

imageCropperValue.Crops = crops;

if (configuration?.EnableLocalFocalPoint == false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check for this configuration setting, as the picker always stores the default focal point (0.5, 0.5). Removing this together with the crop width/height would also reduce the size of data stored!


localCrops.ApplyConfiguration(configuration);

// TODO: This should be optimized/cached, as calling Activator.CreateInstance is slow
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this to-do, because it's also something that's done in the BlockListPropertyValueConverter and might have considerable performance gains by using similar techniques as in PublishedModelFactory (caching emitted constructor functions using ReflectionUtilities). Maybe @Shazwazza can give some pointers on how to best handle/implement this?

@ronaldbarendse
Copy link
Contributor Author

I've created an overview of the changes in this PR, as it contains some breaking and functional changes. These should all be very minimal (or not noticeable at all if you're upgrading from pre 8.14):

Breaking changes:

  • MediaWithCrops model: no parameterless constructor and properties don't have a setter anymore
  • Removed ambiguous method: IHtmlString GetCropUrl(this UrlHelper urlHelper, ImageCropperValue imageCropperValue, string cropAlias, bool htmlEncode = true) (was also available as separate PR #10527)

Functional changes:

  • Local crops are merged with media crops when using the GetCropUrl() extension methods on MediaWithCrops
    • The GetLocalCropUrl() extension methods are now obsoleted because of this
  • The concrete implementation of MediaWithCrops model is now MediaWithCrops<T> and implements IPublishedContent
    • The MediaItem property is obsoleted, as it's now recommended to use the Content property (that is of a specific type when using the generic model)
  • Only configured crops are returned in the ImageCropperValue and MediaWithCrops models

We should make sure to also update the documentation for 8.15: https://our.umbraco.com/Documentation/Fundamentals/Backoffice/property-editors/built-in-property-editors/Media-Picker-3/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8.14-RC Make it possible to switch between Media Picker 2 and Media Picker 3
2 participants