U4-7891 - Image Cropper needs a property value converter to return a strongly typed model, json or a string when requested.

Created by Shannon Deminick 03 Feb 2016, 15:04:46 Updated by Jeavon Leopold 11 Feb 2016, 17:46:00

Relates to: U4-7695

To keep things simple with 7.4.0 - since we are shipping the upload field for media as the Image Cropper by default - we want to ensure that rendering this image is as simple as possible. The Image Cropper stores it's values as JSON and we don't want the user to have to write any parsing logic just to display an image.

GetPropertyValue - the json version of the value GetPropertyValue - the single url of the media item GetPropertyValue - would be the new strongly typed object GetPropertyValue - the new strongly typed object

This is marked as a breaking change since this was not the behavior exhibited by the image cropper in previous versions.

Comments

Jeavon Leopold 04 Feb 2016, 10:47:20

@Shandem Can you help me understand the use case here, if you're rendering a image from media you use .Url anyway which returns the image path even when umbracoFile is the Image Cropper....?


Shannon Deminick 04 Feb 2016, 11:08:05

Yes, that will still be fine but if you are accessing the property directly, or maybe if you have more than one cropper assigned, or if you have an image cropper property on any content or doc type - you still need to retrieve the value.

The change is:

  • Based on this change: http://issues.umbraco.org/issue/U4-7695 - which is considered a breaking change, the Image Cropper's default output will be JObject - because it stores it's values as JSON. We don't want to have to have people re de-serializing this.
  • If the cropper data type has no crops defined, then the value returned from the ImageCropperValueConverter will be string = the image path stored = the same as what would be stored from the upload field - this requires no additional work to get the value
  • If the developer chooses to use GetPropertyValue<string>, it makes more sense to return the normal image path then a json string, if the developer wants json they would use GetPropertyValue<JObject> - or just GetPropertyValue since that will be the default type returned when there are crops.

I am thinking however that if we are going to make this breaking change then maybe it would just be way better to have the default type returned as a strongly typed object. We can still also support:

  • GetPropertyValue - the json version of the value
  • GetPropertyValue - the single url of the media item
  • GetPropertyValue - would be the new strongly typed object
  • GetPropertyValue - the new strongly typed object

thoughts?


Jeavon Leopold 04 Feb 2016, 11:16:49

Yes I totally agree, I think if you're going to make the braking change, default should return ImageCropDataSet, better for Model Builder too, we have been doing this for a long time for Ditto. I have been sitting on adding to my value converters for ever, hope I don't need to now! https://github.com/Jeavon/Umbraco-Core-Property-Value-Converters/blob/v3/Our.Umbraco.PropertyConverters/ImageCropperPropertyConverter.cs


Jeroen Breuer 04 Feb 2016, 16:01:29

Nice to see this getting added to the core. For the Models Builder I always created my own property value converter for the Image Cropper: https://github.com/jbreuer/Hybrid-Framework-for-Umbraco-v7-Best-Practises/blob/master/Umbraco.Extensions/PropertyConverters/ImageCropperConverter.cs

Than I could do things like this: https://github.com/jbreuer/Hybrid-Framework-for-Umbraco-v7-Best-Practises/blob/master/Umbraco.Site/Views/Content.cshtml#L15

Guess if I upgrade I could disable this default converter and enable my own converter again so nothing breaks.


Stephan 04 Feb 2016, 16:12:00

Also note that at the moment, the property value converter "plumbing" does not really handle situations where you could GetPropertyValue with different T. The converter converters to ''one'' type T, full stop. So I'm not 100% sure how we are going to do this?


Shannon Deminick 04 Feb 2016, 16:13:04

PR is here for review: https://github.com/umbraco/Umbraco-CMS/pull/1098

Changes:

  • Fixes TextStringValueConverter to include the text box and multi-textbox (since it's like that in the Core one that this overrides)
  • adds new converter for Image cropper including ToString converter & custom type converter to convert from the strongly typed object to JObject
  • The ToString() will return just the image path if no crops are defined, otherwise it returns the JSON value as a string
  • adds test to support this
  • adds equatable methods for the strongly typed model so it can be easily compared
  • Ensures that the webboot manager does not include the core value converter for image cropper (there are two value converters, the core + web (which inherits from core), we remove the core one when the web starts up since there can only be one)

To test:

  • Run the unit tests
  • Create a new image cropper data type without any crops defined
  • Change your media image content type to use this data type as the 'umbracoFile' property
  • Create a media item (note the id)
  • Add an image to this
  • In your view render this:
@{
//use your own id
var media = Umbraco.TypedMedia(123);
}
<h5>media.GetCropUrl("blah")</h5>
<img src="@media.GetCropUrl("blah")">
<h5>Url.GetCropUrl(media, "umbracoFile", "blah")</h5>
<img src="@Url.GetCropUrl(media, "umbracoFile", "blah")">
<h5>media.GetPropertyValue("umbracoFile")</h5>
<img src="@media.GetPropertyValue("umbracoFile")">
<h5>(media.GetPropertyValue&lt;string&gt;("umbracoFile"))</h5>
<img src="@(media.GetPropertyValue<string>("umbracoFile"))">
<h5>media.Url</h5>
<img src="@media.Url">
  • You will get 3 x images for the last 3 - the first 2 will render nothing because there is no crop 'blah' defined
  • Now, go to your image cropper data type and add a crop called "blah"
  • refresh your front-end page, you will see 4 x iamges - the first 2 will be the crops, the others will be the full size and the 2nd last one will not render - because when you do @(media.GetPropertyValue<string>("umbracoFile")) when the item has crops it will output the full json


Shannon Deminick 04 Feb 2016, 16:13:50

@zpqrtbnk we have Core property converters, we've had this for a very long time. If people plugin their own, they will 'override' the core ones without issue.

This is what the DefaultPropertyValueConverterAttribute is for


Jeavon Leopold 05 Feb 2016, 09:56:36

@Shandem I f*****g love this! It's truly brilliant!

I found one issue in that when I've added a couple of crops "blah" and "blah2" to the data type if I return to the media item I cannot see or manually edit the crops (I was going to test that the manual crops still work as expected)

Also I know this is a breaking change so it's ok, but there are some people who have been iterating the JSON using the dynamic object, they will have to capitalise their properties, e.g. @foreach (var crop in mediaItemDyn.umbracoFile.crops) { <img src="@CurrentPage.GetCropUrl("image", crop.alias)"> }
Will have to become @foreach (var crop in mediaItemDyn.umbracoFile.Crops) { <img src="@CurrentPage.GetCropUrl("image", crop.Alias)"> }


Shannon Deminick 05 Feb 2016, 10:27:42

Yup, so it is a breaking change ( a good one ) but we have one last chance to determine if this is 'too breaking' - we need to make this decision now-ish. There's really no way i can make the dynamic part work the way it was since we don't have different value converters for dynamic vs normal usage. ... actually, it might be possible - we might be able to inherit from DynamicObject for the strongly typed model to faciliate that. Hrm, might be worth it ?

I'm not sure i understand the manual crop issue though (might be cuz i don't know a ton about this cropping stuff!) But if i add blah2 as a crop to the data type, return to a cropper property type, i can see the blah2 crop, i can click on it and change it's focal point


Shannon Deminick 05 Feb 2016, 12:52:38

@crumpled_jeavon I've allowed for dynamic case insensitive methods and properties on the strongly typed model now: 96b180225200bee21093b1060b79b27f82879660 so your example should work. Just let me know what you meant by 'manual crops'


Jeavon Leopold 05 Feb 2016, 17:06:24

Cool! I couldn't see the crops at all in the UI but I then deleted that media item and made new ones and everything was exactly as it should be. I'll have another go from scratch to see if I can recreate the issue otherwise it was a random gremlin!


Shannon Deminick 10 Feb 2016, 10:10:37

@crumpled_jeavon Thanks for your feedback so far, just want to know if you think that with all of the changes we've made here, have we taken into account enough to make this less of a breaking change than a huge one?


Jeavon Leopold 10 Feb 2016, 11:50:26

@Shandem hell yes! I'm just checking out the latest commits on your temp branch, will report back shortly


Jeavon Leopold 10 Feb 2016, 12:14:41

@Shandem have checked everything again everything is good except I still have the crop refreshing issue I had before, I will create a screen cast now for you.


Stephan 10 Feb 2016, 12:15:46

FWIW am currently testing & experimenting w/Models Builder...


Jeavon Leopold 10 Feb 2016, 12:20:17

Screencast here https://screencast-o-matic.com/watch/cDn6lbhR4J


Stephan 10 Feb 2016, 12:59:55

As far as my tests go, it's all good to me. The models builder generates a property that returns an ImageCropDataSet so it's OK to write eg @Model.UmbracoFile.Crops.

The only thing that "feels" weird to me is GetPropertyValue("umbracoFile") switching from returning the source image path, to the full JSON, when crops are defined. Do we need it (backward compat maybe) or could we always return the source image path?


Jeavon Leopold 10 Feb 2016, 13:18:11

@zpqrtbnk yes it's for backwards compatibility as that is what is used at the moment


Stephan 10 Feb 2016, 13:21:48

@crumpled_jeavon OK - having a look at the screencast now


Jeroen Breuer 10 Feb 2016, 13:26:48

Not sure if this is a good idea, but maybe the name UmbracoFile here isn't fitting if it returns an ImageCropDataSet. I always rename it to Cropper: https://github.com/jbreuer/Hybrid-Framework-for-Umbraco-v7-Best-Practises/blob/master/Umbraco.Extensions/Models/Generated/Image.cs


Shannon Deminick 10 Feb 2016, 13:31:53

umbracoFile is the standard name in Umbraco, we cannot just change that.


Stephan 10 Feb 2016, 13:33:46

I can reproduce @crumpled_jeavon screencast issue


Shannon Deminick 10 Feb 2016, 14:09:00

I've rebased my branch and pushed it... can't seem to replicate the issue


Shannon Deminick 10 Feb 2016, 14:11:00

Hrm, but i have another issue, i think that http://issues.umbraco.org/issue/U4-7695 is causing some issues, i'll check now


Shannon Deminick 10 Feb 2016, 14:32:33

OK, have pushed a fix now. The issue was due to not merging crop pre-values correctly (i didn't actually know we saved individual coordinates)


Jeavon Leopold 10 Feb 2016, 15:21:03

@Shandem using the latest v7-dev branch https://github.com/umbraco/Umbraco-CMS/commit/cf0b72fd77fa87cc8b1805efcffd96b67ab0b1bf I reliably still have the issue I showed in the screencast, @zpqrtbnk do you?


Shannon Deminick 10 Feb 2016, 15:41:53

If you've manually change the individual crops then they won't be affected, it will only work if you haven't adjusted the individual ones AFAIK.


Shannon Deminick 10 Feb 2016, 15:47:43

It defo works for me with the latest dev branch


Stephan 10 Feb 2016, 15:53:34

will test


Jeavon Leopold 10 Feb 2016, 17:36:56

I've done a Git Clean -f -d -x rebuilt and reinstalled and still it happens :( Seems that the cache is not updated as also restarting updates to correct crop New screencast at http://screencast-o-matic.com/watch/cDn6b4hRJ9


Sebastiaan Janssen 11 Feb 2016, 12:43:14

Yup I can repro this cache issue too, care to have a look @zpqrtbnk ?

Setup:

  • datatype Image Cropper has a crop on it called "Tester" (100x300)
  • document type with media picker, picked an image in the media section
  • change focal point in image and save, you see it update in the preview on the right Template looks like this quick and dirty thing:

@ <img src="@mediaItem.GetCropUrl("umbracoFile", "Tester")" />


Shannon Deminick 11 Feb 2016, 12:45:37

We need to also check with browser cache disabled (i.e. have dev tools open)


Shannon Deminick 11 Feb 2016, 13:17:50

I've also just realized i was testing with a content property as a cropper, not a media item, so that might be the difference


Stephan 11 Feb 2016, 13:21:07

it's only with medias

basically, saving a media clears the published media cache, but MediaModelMapper gets the media from that cache before the Examine stuff has been updated, so it re-populates the published media cache with the obsolete media


Sebastiaan Janssen 11 Feb 2016, 15:00:44

Fixed in: https://github.com/umbraco/Umbraco-CMS/commit/17ff952b52f50d38d7d482a1e13bd9d9cfc29c6f

No longer getting the media URLs from TypedMedia but using mediaService


Jeavon Leopold 11 Feb 2016, 15:48:56

Yay! Great job!


Sebastiaan Janssen 11 Feb 2016, 15:58:09

@crumpled_jeavon Thanks again for discovering and reporting!


Shannon Deminick 11 Feb 2016, 16:35:18

I've also fixed more, @crumpled_jeavon if you want to have a look: 016a14e5c70c9a2b8db548106cee6c234d795eeb

The issue is:

  • You have no crops defined
  • You save an image to the cropper - everything is OK
  • You create a new crop
  • That crop doesn't show up on the already saved item (or the front-end... but this particular issue is if you started from no crops)
  • With this fix, you'll see the new crop in the back office without saving

I think this is an issue with the grid too, we've fixed the issue of merging pre-values with front-end data but not for the back office. Will need to test that for 7.4.1.


Jeavon Leopold 11 Feb 2016, 17:46:00

@Shandem awesome, that is actually the exact issue I had at first http://issues.umbraco.org/issue/U4-7891#comment=67-26367 but couldn't work out how to recreate!

I think this is great step forward and as you say I'm sure the logic can be reapplied to different editors such as the grid.


Priority: Normal

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 7.4.0

Sprint: Sprint 8

Story Points:

Cycle: