Saving Macro Container properties to Nested Content doesn't always work

[robertjf]

Scenario:
I have a Document Type with a Macro Container on it as well as a Textbox (for the title of the content) that I'm using as a Nested Content template. If I try to associate the macro and then enter a string in the textbox, the macro will be correctly captured as part of the data.

However, if I fill in all the other details and then select the macro right before saving, the macro is not saved, and if I don't touch any of the other fields the macro will never be captured.

Reason:
The MacroContainer does not assign the macro details back to the model.value until the formSubmitting event has been received:
https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web.UI.Client/src/views/propertyeditors/macrocontainer/macrocontainer.controller.js#L88-L95

    var unsubscribe = $scope.$on("formSubmitting", function (ev, args) {    
        var syntax = [];
        angular.forEach($scope.renderModel, function(value, key){
            syntax.push(value.syntax);
        });

        $scope.model.value = syntax.join("");
    });

This means that the macro isn't updated until after we've saved the NestedContent value
I also tried fixing this by adding in the same sort of unsubscribe handler to try and capture the macro value on formSubmit, but that didn't work because of the sequence the event handlers are triggered in.

In a nutshell: The only way that a macro container will save the macro in a NestedContent property is this:

1. Macro is selected
2. User Saves (or Save and Publishes) the document type (triggering the save back to the model value)
3. User then goes to another property (doesn't matter which node) like a textbox (this triggers the $watch on the nodes, which then rebuilds the full NestedContent model.value)
4. User Saves or Saves and Publishes a second time.

I've spent 5 hours trying to get the NestedContent property to postpone the model.value building until after the MacroContainer has a change to save it's property values properly with no luck. I now know a lot more about $broadcast and $on than I did 6 hours ago.

[mattbrailsford]

What's the use case? I'm tempted to say this is a scenario where it's just pushing too hard. NC is for simple lists and this sounds a bit more complex. If you can explain the use case, maybe I can understand the need.

[robertjf]

I have two Document Types that are used for the one NestedContent list; they both have the same properties (title, background image, fore colour, background colour, etc.) with one exception: 1 DocumentType has a ContentPicker, the other has a MacroContainer.

The intention is that the user can select up to 6 items in the list and choose whether to insert a Content item or a Macro item. These items are then arranged and rendered based on their values as tiles in a variable flow layout.

You can see the result here: http://williamadamsptyltd-dev.azurewebsites.net/ (the 6 tiles + the two forms) under the banner.

Everything works perfectly up until the Macro DocumentType is used (macros render tiny forms in the content instead of linking to the picked content).

I don't see this scenario as overly complicated; and the user loves the fact that everything is quite uniform but also flexible.

The problem is that some of the Umbraco controls only save their values back to the model when the formSubmitted event is broadcast; and because child controls are rendered last (especially if they have an external template in the directive) we aren't able to capture the actual value because by the time it would be available to us it's too late - the content has already been pushed back to the server.

As for simplicity, I could have a single macro container property on the nested document type, and maybe a label - nothing complicated; but the result will be the same - some built-in (and probably more than a few third-party) property editors will not work as expected within Nested Content as it is now.

I actually suspect the same sort of issue was encountered with the Grid editor - there are custom RTE and macro editors in the Grid to combat in part at least this sort of problem; and in part at least, this issue actually lies within Umbraco itself; but it would be good to be able to come up with a solution so we have something robust and just works.

[kgiszewski]

Re: the property editors that use 'formSubmitted' to store values, Archetype had the same issue so we had to add 'yet another' event that runs after 'formSubmitted' to handle these types in order to capture their final values. FWIW.

[robertjf]

@kgiszewski yes I was thinking something along those lines, although I wasn't sure where to add the event etc; I did play around with the unsubscribe, trying to get it to fire last on the formSubmitted event (by effectively unsubscribing and re-subscribing whenever content was added and rendered) but nothing I tried was working.

I might have to have a closer look at Archetype.

[mattbrailsford]

@kgiszewski I don't get how your custom event gets handled? https://github.com/imulus/Archetype/blob/master/app/controllers/controller.js#L405 Does the third party property editor developer have to listen for formSubmitting AND archtypeFormSubmitting? Thus the built in prop editors wouldn't work without modification?

[kjac]

@mattbrailsford see #262 (kgiszewski/Archetype#262) for an explanation on the archetypeFormSubmitting event. In short, archetypeFormSubmitting was introduced so Archetype wouldn't have to rely on the execution order of the other property editor's formSubmitting event handlers.

[kgiszewski]

@mattbrailsford Indeed it appears to have that sort of dependency but @kjac figured out a slick way around that. More and more core editors are using 'onFormSubmitting' so compatibility will diminish if this isn't addressed.

[robertjf]

@mattbrailsford @kgiszewski aha - use a directive after everything else has been added to watch the formSubmit - great idea. Trying to get the last say in as it were is the hardest thing. What's not immediately clear is how do you handle items that are added to the list after the watcher has been rendered (post-linked); because new items would presumably handle the formSubmitting event after the watcher does...

[mattbrailsford]

I think I might ping Shanan to see if he can spare the time to suggest the right way to handle this. Creating a custom event doesn't seem like the right thing to do. It might mean its partly core and partly nc changes that need to happen, but I'd much rather it be done right.

[kjac]

@mattbrailsford @kgiszewski We add a new directive for each property and execute the archetypeFormSubmitting event handler for the very last one added... thus it works with newly added items as well.

[robertjf]

It's a hack, certainly. One way to do it would be to have the formHelper broadcast another event between the formSubmitting event broadcast and the actual form submit action.
Not really sure what this new event should be called though - "lastChanceBeforeFormSubmit" perhaps?

[robertjf]

@kjac aha that makes sense now.

[kjac]

@mattbrailsford remember that if a core change is needed for this to be done the right way (if there indeed is a right way), you'll most likely end up making NC incompatible for the previous versions of Umbraco. Just saying :)

[kgiszewski]

@mattbrailsford At first I thought I'd like a core event to handle this, but with the negativity towards Archetype from the core, I'd avoided even discussing it. My animosity paid off because there is a problem with that approach IMHO.

As @robertjf suggests, but what should it be called? noReallyThisIsYourLastChanceEvent? Then any property editors in the core couldn't use it so it could be used by Archetype, NestedContent and Grid (and future editors).

But then in the case of putting a property editor that does subscribe to noReallyThisIsYourLastChanceEvent into another property editor that also subscribes to noReallyThisIsYourLastChanceEvent, we are back to square one.

@kjac's solution avoids the situation altogether due to the synchronous nature of event handling in Angular.

Just my two cents :)

[mattbrailsford]

Understood, but I'd rather have changes made in core if that is where they belong such that there is an official way of supporting this, given that more complex/nested property editors will likely be on the horizon. Right now, the likelihood is a fix like this would have to be implemented by every "complex" property editor which seems a huge waste/duplication of code. I'd much rather sacrifice one or two dot releases support for a solution that works for everyone, not just us.

[kgiszewski]

Any ideas on how to avoid event race condition then? i.e. editor1 and editor2 both subscribe to the last event therefore order is not predictable.

[mattbrailsford]

Nope, hence why I'd like to get Shanons/Per's input :)

[kgiszewski]

:) Indeed. I just hope adding yetAnotherEvent isn't the solution, but it's just my opinion.

[robertjf]

yeah I'm not sure that yetAnotherEvent is the way to go either - too prone to misuse.

I tried another tack the other night - subscribed to $includeContentRequested and then attempting to un-sub/re-subscribe to the formsSubmitted event whenever the child content was linked; but that didn't make any difference.

[kgiszewski]

Agreed, because I suppose we're already in that situation with onFormSubmitting because I was shocked that the core properties were using this to update their models. No way to enforce this AFAIK.

[Shazwazza]

Currently there is no good 'fix'. Unfortunately all of these nested editors were created based on the unknown ability that you could even create them... therefore they are all basically a 'hack' in the first place since things were never designed with this intent.

I don't have solution to give you guys at the moment unfortunately. I'm open to suggestions but i agree, adding another event that you cannot force people to use 'properly' is never going to work... because people will do whatever seems to be possible (i.e all of these nested editors :P )

The only solution will be a solution that isn't a hack... but again, i'm not sure what that is off the top of my head... need to find some time to think about it, make POCs, etc...

[kgiszewski]

👍

[Shazwazza]

The work around that was done by archetype is on the correct path though. In one way or another, if there's going to be a nested property editor that contains other property editor (that can contain other property editors, and so on ), then the thing that is wrapping them needs to handle the notifications to/from it's children.

[mattbrailsford]

i guess the only thing there is whether something reusable can be built (maybe in core) so that each time a "nested" property editor comes along we don't have to duplicate code a bunch of times.

[robertjf]

perhaps we could implement (in the core) an event that is broadcast after a control has been linked? Maybe in the umbProperty directive? That way properties like NestedContent and Archetype can subscribe to it and then bind to the formSubmitted event after everything else has done it's thing.

Of course, we're only interested in our own child controls...

[Shazwazza]

I don't think events are going to solve the issue. What happens when you have nested content inside nested content inside archetype, and so on... who's listening then?

What needs to happen is that these wrapping property editors need to also do their binding on formSubmitted which must occur 'after' all children (of chilren, of children) have received the event. It's also worth noting that this would be HEAPS better for performance too, currently nested content is doing a gigantic deep watching for model value changes... this cannot be good for performance. (BTW.... if i'm wrong about that, sorry, i'm only just glancing at the code, i'm not familiar with either nested content or archetype codebases ;) )

So the question is how do we achieve this? The way that archetype have done it is not totally unreasonable. You have a controller that is:

• getting data to create child editors
• these child editors 'could' bind to formSubmitting with their $scope

So in theory, so long as the controller creating these child editors has a way of handling the formSubmitting event 'after' the other ones have executed, then everything will work.

an alternative approach is to do something like this:

• create a $scope object that will be used to create the child property editors manage this process instead of just using the syntax (meaning you'd basically be creating a subset directive of the ng-include which IIRC is the thing that creates child scopes)
• then you could hijack the $on method to swap it for your own function which wraps the original one
• you could check for the 'formSubmitting' parameter
• then you have a list of proxies to manage for this particular event
• therefore you can control when they execute

These are just outside of the box thoughts, but they are most likely possible and might give you some inspiration for ideas.

[mattbrailsford]

currently nested content is doing a gigantic deep watching for model value changes

What makes you think that out of interest?

I'll have to look fully into how Archetype achieves this then as I'm not too familiar with it myself, unless you fancy giving it a go @kgiszewski given your knowledge on the subject? Maybe once we see it as a single PR, and the code changes needed, we can see/decide what (if anything) could be in core to make this easier?

[Shazwazza]

@mattbrailsford isn't this how you maintain your model.value: https://github.com/leekelleher/umbraco-nested-content/blob/develop/src/Our.Umbraco.NestedContent/Web/UI/App_Plugins/NestedContent/Js/nestedcontent.controllers.js#L313 ? It's a deep watch for your nodes array. .... ahh, but maybe that is just a deep watch of the 'array' object, not it's sub objects. If you wanted to speed that up a little, you could just watch the array length property:

$scope.$watch(function () { return $scope.nodes.length; }, function () {

[mattbrailsford]

Github needs a like button :) @kgiszewski

[kgiszewski]

Hehe :)

[robertjf]

lol I like that :)

[leekelleher]

@robertjf I'm not sure where this discussion has left you, (in terms of a workable solution) ... but if Archetype can already support Macro Container, I'd go with it.

[Shazwazza]

The formSubmitting event is for a few things, firstly to deal with how we want validation displayed, and to handle custom/manual validation if required and also to allow a developer (or core) to update it's value before being submitted. So yes, it can greatly enhance performance and could be used for that, or to be used for anything a developer wants to do to their model before it's submitted to the server... i don't think it's 'anti-angular'. The more watches = the worse performance. This is why by default angular doesn't want you to watch deep object graphs.

[robertjf]

@leekelleher @mattbrailsford I can submit a workaround similar to the way Archetype does it, that wouldn't be hard now that I've had a look... in the mean time, I have a custom version in my project :)

But I would like to see this resolved.

[mattbrailsford]

I can get behind the validation stuff, as of course, this shouldn't happen till you submit the form, but when used to tweak the model before saving, I don't get why this shouldn't happen as the change is triggered on input and store it in model.value? (Deep watching aside) I thought the whole point of model-binding was to have that realtime updating across the UI, which this makes it go back to a "maintain your own state and push it back when we save" mentality.

[Shazwazza]

We can argue about this all day if you want ;) but what is in there, is in there. Having a viewmodel is required for plenty of property editors, that does model binding. The viewmodel might be different than the persisted model for all sorts of reasons, so what's the harm in updating the persisted model only when it needs updating. Otherwise your maintaining 2 real-time bound models for no real reason, apart from working nicely with these nested property editors ;)

[mattbrailsford]

Now you get it, so we agree, core is wrong, and it should be fixed, saving me work :)

[robertjf]

lol

[kjac]

Good grief... I leave this place for a few hours and it turns into a debate club :D

@kgiszewski glad to hear it's not only you that taught me stuff :P

On a serious note, tho', the Archetype way of handling this issue is a hack, yes, but it's a viable one. If it will work for Nested Content too, awesome. Can someone clone & own the Archetype solution, or shall I have a look at it tomorrow?

[robertjf]

@kjac debate is right :) I'm happy to submit a pull request with this, just wanted to make sure @mattbrailsford & @leekelleher were open to it first...

[mattbrailsford]

Go for it. Seems to be the logical consensus so I'm happy. Just wanted to get input to make sure there wasn't a better way.

[robertjf]

managed to streamline the implementation a little and avoid broadcasting another event. Have tested it on one of my projects where the original problem exists and it's working nicely.

[kgiszewski]

@robertjf I just looked at the code, good job. I'm just curious how the code knows how to run at the end and not run multiple times :)

[Shazwazza]

I'll have a go at a POC of my own tomorrow... Without looking at how anyone has actually done it and maybe with our powers Combined we can come up with the simplest implementation.

[mattbrailsford]

cheers @robertjf and @Shazwazza, really appreciated #teamwork

[robertjf]

@kgiszewski the same way that @kjac did it - the directive is using the counter to determine whether it is the last one linked or not; and if it is then it's firing the callback to update the model. Because the directive is linked after the node, it's hooking up to the formSubmitted event last. We don't even need to broadcast our own event, as we only care about our own children and nothing else.

Even nestedContent within nestedContent will trigger a model update only once within it's own level since we're not broadcasting anything - the way Archetype has done it, I wouldn't be surprised if you're seeing the archetypeFormSubmitting event being broadcast multiple times. Especially if you have nested Archetypes - this would lead to the nested Archetypes updating their models multiple times on the one formSubmitted event I suspect. Just a suspicion, I haven't played with it to test the theory :)

[kgiszewski]

Yeah that's what I was curious about, multiple broadcasts.

[robertjf]

it's not broadcasting at all; and only triggering the callback once.

[kgiszewski]

Yep, sorry I meant Archetype :) Sorry mind wandering.

[robertjf]

lol ah - okay :)

[Shazwazza]

Ok, didn't spend a heck of a lot of time today fiddling with this but did end up trying things quite similar to both of solutions which is adding a directive after the the directive that renders the property editor. I'd like to point out one thing:

• It's VERY important that this special directive have it's own isolated scope declared, if it didn't, then none of this works due to the way that $broadcast works, it executes subscribers from the inside out, so it doesn't actually matter when you subscribe to formSubmitting, it matters 'where' in the chain of hierarchical $scopes. If you didn't have an isolated scope declared (i.e. you didn't declare a scope on this directive at all), then you'd find that it would consume the event before the sub-properties and we'd have to figure out a different avenue of making this work.

I did investigate 'hijacking' the $on method in the pre-link of the nestedContentEditor so that when sub-properties used $scope.$on it would actually call this hijacked method. This does work but I feel like my quick implementation might be error prone and would eventually be more complex than using this simple special directive used to acquire the last broadcast notification.

[mattbrailsford]

Cheers @Shazwazza, appreciate the feedback