Saving Macro Container properties to Nested Content doesn't always work about umbraco-nested-content HOT 61 CLOSED

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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

@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.

from umbraco-nested-content.

@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?

from umbraco-nested-content.

@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.

from umbraco-nested-content.

@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.

from umbraco-nested-content.

@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...

from umbraco-nested-content.

from umbraco-nested-content.

@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.

from umbraco-nested-content.

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?

from umbraco-nested-content.

@kjac aha that makes sense now.

from umbraco-nested-content.

@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 :)

from umbraco-nested-content.

@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 :)

from umbraco-nested-content.

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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

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

from umbraco-nested-content.

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

from umbraco-nested-content.

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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

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...

from umbraco-nested-content.

👍

from umbraco-nested-content.

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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

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...

from umbraco-nested-content.

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.

from umbraco-nested-content.

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?

from umbraco-nested-content.

@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 () {

from umbraco-nested-content.

@Shazwazza @mattbrailsford this sort of works, but controls that don't update their value until the formSubmitting event won't trigger the watch until it's too late.

from umbraco-nested-content.

yes, i understand the underlying problem to this convo ;)

from umbraco-nested-content.

glad someone does :)

and you are right, that currently is how we maintain our model so I guess that needs to be reviewed then. Thanks for the heads up.

from umbraco-nested-content.

@kjac is the actual expert for fixing the issue. However from how I understand it (loosely) is that Archetype itself subscribes to onFormSubmitting and inside that it broadcasts it's own internal cleanup event. The reason it goes last is due to the fact that event execution is synchronous in the order by which it is registered.

So by that logic, everybody's onFormSubmitting runs first (including Archetype), then the next round of events run which include the internal Archetype event.

https://github.com/imulus/Archetype/blob/8f8078e12f7509a812af95c5cb8abd76563fe29f/app/directives/archetypesubmitwatcher.js#L5-L9

https://github.com/imulus/Archetype/blob/master/app/controllers/controller.js#L410

http://stackoverflow.com/questions/18130369/emit-broadcast-synchronous-or-asynchronous

The biggest thing that @kjac taught me was the synchronous bit :)

from umbraco-nested-content.

@mattbrailsford If that watch is actually monitoring the sub properties of each object inside of your array, it might get a bit slow if you've got lots of sub properties (depending on how complex they are)... I'm assuming it is watching that deep, otherwise it wouldn't work from what i can tell. The only way around that would be to use the formSubmitting event, to update your model value to what is in your children, but of course we need a work around so that either your formSubmitting event happens after all the children have processed their formSubmitting handlers, or you use archetype's method of emitting a custom event back up to your controller from a custom directive that processes the formSubmitting event after your child properties. (pretty sure that's what archetype is doing)

from umbraco-nested-content.

Is this why those events exist in the first place? as a performance enhancement? I guess my understanding of angular / model binding was that you should just update your model and allow other things to listen to those changes, so waiting for formSubmitting sounds a bit "anti-angular" in a way?

from umbraco-nested-content.

@kgiszewski yep that's right, the only question is what's the best way to make sure we're last in the list - the concept of using a directive to do so works well, and it keeps the context of the child controls - i.e., we only care about our child controls, and the way @kjac has built it, the last directive rendered is the one that has the subscription to the formSubmitting. So it's fairly clean; you just end up with a stack of watcher directives that's all

from umbraco-nested-content.

@mattbrailsford My thoughts exactly :)

from umbraco-nested-content.

@robertjf Archetype is slowly becoming self-aware and perhaps we can just ask it at some point ;)

from umbraco-nested-content.

Github needs a like button :) @kgiszewski

from umbraco-nested-content.

Hehe :)

from umbraco-nested-content.

lol I like that :)

from umbraco-nested-content.

@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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

@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.

from umbraco-nested-content.

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.

from umbraco-nested-content.

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 ;)

from umbraco-nested-content.

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

from umbraco-nested-content.

lol

from umbraco-nested-content.

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?

from umbraco-nested-content.

@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...

from umbraco-nested-content.

from umbraco-nested-content.

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.

from umbraco-nested-content.

@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 :)

from umbraco-nested-content.

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.

from umbraco-nested-content.

cheers @robertjf and @Shazwazza, really appreciated #teamwork

from umbraco-nested-content.

@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 :)

from umbraco-nested-content.

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

from umbraco-nested-content.

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

from umbraco-nested-content.

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

from umbraco-nested-content.

lol ah - okay :)

from umbraco-nested-content.

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.

from umbraco-nested-content.

Cheers @Shazwazza, appreciate the feedback

from umbraco-nested-content.