The errata (E70e) now includes "Perform further deferred updates as necessary" as part of the value change sequence. It may not be crystal clear, but this means that if a f.x. xforms-recalculate event is dispatched during an recalc-reval-refresh, we need to peform another xforms-recalculate after finishing the cycle. Right now we just perform it right away I think, and it should actually be quite easy to create an infinite loop... We also need to make sure that any nodes getting marked f.x. dirty as a "side-effect" of the first cycle triggers dispatching of events on the next. This may depend on bug 300586 -- but might be done in the same patch, dunno.

Ok, here's a testcase with what I _think_ is what should happen. the recalc, revalidate, and refresh of the setvalue action(s) should happen after the previous refresh has finished. So if a value changed sequence is running, and another one is triggered, we should delay the handling of the latter until the first is done.

Hmmm, I've been trying to understand what actually should happen with f.x. xforms-recalculate while trying to figure this out. As I read it we're doing it wrong now. Our model is listening for the events and then performing the action. It's the other way around: The model should dispatch the event before performing the action. For xforms-recalculate, the spec reads (sect. 4.3.6): "Dispatched in response to: a request to recalculate all calculations associated with a particular XForms Model." (http://www.w3.org/TR/xforms/slice4.html#evt-recalculate") So upload, setvalue, input, and actions should call functions on the model and the model should then dispatch the events prior to performing the action. But should the model then listen for these events, and do the action? Or should it just do the action and ignore the event. The former let's a form author cancel the event, and thus the action. The latter only the event.

Peter and I talked about it on IRC. Here's my thoughts. 1) the recalc handling in the model needs to copy the dispatch flags from the MDG, and then clear them in the MDG. So any changes to the flags in action handlers, etc. will not be reflected in the current "value changed sequence" 2) the model needs to set a flag upon receiving an event. If it is already handling events it returns immediately, if not it starts the "event handling loop". 3) The event handling loop must loop between the flags for the events in the value changed sequence, until there are no flags set. 4) We should possibly also store the instance node state in the UI controls, and let it handle the dispatching of xforms-readonly, xforms-value-changed, etc. during refresh. If not, it might "fetch" a wrong state at the time of the refresh.

We also need to make sure that we do not set the intrinsic states before we dispatch the events -- it should happen on the same time. The SetStates() function call used by controls might me messing this up.

(In reply to comment #3) > 2) the model needs to set a flag upon receiving an event. If it is already > handling events it returns immediately, if not it starts the "event handling loop". More thinking on my part, and we need a queue of "event sending", not just flags. That is, the model is asked for a recalculate, and the model sends the events to itself if it is not doing a RRR, if it is it queues it (the sending). At least if you agree on attachment 193813 [details]. The spec. is, once again, not crystal clear, but my rationale is: f.x. the "default action" of xforms-recalculate is to perform the "actual reculate functionality". So I see the event as a "notification" of an action that is about to performed, and the form author can then cancel the event if the action should not be performed at that time. There are tons of unanswered questions / non-specified stuff in this :( I'll start hacking on this now.

I've added a design draft here: http://wiki.mozilla.org/XForms:Model_Concurrent_Changes Many unanswered questions... Please give comments here or on the talk page on the wiki.

(In reply to comment #6) > I've added a design draft here: > http://wiki.mozilla.org/XForms:Model_Concurrent_Changes > > Many unanswered questions... Please give comments here or on the talk page on > the wiki. No comments? If there are no comments, I'll start hacking it one of these days.

Sorry that it took me so long to get to this. Wanted to get my 3 select/select1 patches done since they were all kinda dependent on each other. well, I have no idea what the spec authors were thinking by putting that line (4.6.7 step #4 in the sequence) in w/o some kind of extended comment. It makes no sense on its own. But I personally think that the deferring that it is talking about is the deferred updates that can happen via xf:action's. So I'd say that you should queue up actions that are deferred through xf:action's deferring rules, but if you have a <xf:rebuild ev:event="xforms-value-changed"..../> where the ev:event is declared right on a handler, I think that you have to execute that handler right away.

and I agree...you have to defer the event and action both. The action needs to happen in the normal event processing as the default handler. Well, as long as the event isn't canceled, of course.

(In reply to comment #8) > well, I have no idea what the spec authors were thinking by putting that line > (4.6.7 step #4 in the sequence) in w/o some kind of extended comment. It makes > no sense on its own. But I personally think that the deferring that it is > talking about is the deferred updates that can happen via xf:action's. I'm sure that is not the case, but I was also at the f2f. The idea is that RRR events have the same deferred update behaviour as actions inside xf:action have. > So I'd say that you should queue up actions that are deferred through > xf:action's deferring rules, but if you have a <xf:rebuild > ev:event="xforms-value-changed"..../> where the ev:event is declared right on > a handler, I think that you have to execute that handler right away. Yes, that should be clear because those kind of actions bypass the normal event system. We handle that part wrong today (bug 332231).

I've revised my design a bit: http://wiki.mozilla.org/XForms:Model_Concurrent_Changes The short story is, that I believe we should defer both value-changes and action elements (with possible deferred behavior), and that we should also defer xforms-rebuild. script and <recalculate/>, etc. call the functions exposed nsIXFormsModelElement, which bypasses both events and the deferral.

(In reply to comment #12) > Created an attachment (id=222336) [edit] > Patch This implements the event queue. It does this by making value change functions call the model, instead of sending events. They use these (from nsIModelElementPrivate): void requestRebuild(); void requestRecalculate(); void requestRevalidate(); void requestRefresh(); The update processing is locked by mProcessingUpdateEvent, and the main action is in RequestUpdateEvent(). There is also a loop detection that has its max. set to 600, and can be user-set by xforms.modelLoopMax (which catches bug 301994). It might be nice to popup a dialog asking the user whether to continue or stop processing, but if you vote for that, that's a follow-up bug. Also fixes the action handling to bypass the event system, by calling nsIXFormsModelElement->Rebuild(), etc, directly (bug 332231). Also kills some dead/duplicate code: * We had multiple SetNodeValue() functions lying around * SetSingleNodeBinding() was only (inefficiently) used by upload

Comment on attachment 222336 [details] [diff] [review] Patch >@@ -83,38 +83,37 @@ interface nsIModelElementPrivate : nsIXF >+ * >+ * @param instanceNode The node to set the value for >+ * @param nodeValue The value to set >+ * @param doRefresh Do the value-changed refreshing routine Could you mention which events are dispatched if this is true. Same thing also in void setNodeContent for @param doRebuild >+ >+ /* Request an xforms-rebuild */ >+ void requestRebuild(); >+ >+ /* Request an xforms-recalculate */ >+ void requestRecalculate(); >+ >+ /* Request an xforms-revalidate */ >+ void requestRevalidate(); >+ >+ /* Request an xforms-refresh */ >+ void requestRefresh(); Some more documentation here, please ;) Maybe something about event queue. >+ // Poor man's try-catch to make sure that we set mProcessingUpdateEvent to >+ // false on exit. If we actually bail with an error at some time, something >+ // is pretty rotten, but at least we will not prevent any further updates. >+ class Updating { >+ private: >+ nsXFormsModelElement* mModel; >+ public: >+ Updating(nsXFormsModelElement* aModel) >+ : mModel(aModel) { mModel->mProcessingUpdateEvent = PR_TRUE; }; >+ ~Updating() { mModel->mProcessingUpdateEvent = PR_FALSE; }; >+ } up(this); Are you sure all compilers can handle this? Perhaps better to move the class outside the method. I'd like to get some reply to your mail (w3c xforms mailing list). I'll mark r+/- after that.

(In reply to comment #14) > I'd like to get some reply to your mail (w3c xforms mailing list). > I'll mark r+/- after that. I'd rather not. That could take months. I suggest that we either leave actions be as they were, or we go through with my view of things. I agree with the rest of the comments.

Comment on attachment 222336 [details] [diff] [review] Patch OK, then r=me (Most of the patch is anyway 'just' clean-ups)

Comment on attachment 222489 [details] [diff] [review] Patch with smaug's comments >diff -X patch-excludes -uprN -U8 xforms/nsXFormsMDGEngine.cpp xforms.deferevents/nsXFormsMDGEngine.cpp >--- xforms/nsXFormsMDGEngine.cpp 2006-04-05 10:02:08.000000000 +0200 >+++ xforms.deferevents/nsXFormsMDGEngine.cpp 2006-05-17 14:03:30.000000000 +0200 >@@ -223,41 +223,59 @@ nsXFormsMDGEngine::AddMIP(ModelItemPropN > } > > return NS_OK; > } > > nsresult > nsXFormsMDGEngine::MarkNodeAsChanged(nsIDOMNode* aContextNode) > { >- nsXFormsNodeState* ns = GetNCNodeState(aContextNode); >- NS_ENSURE_TRUE(ns, NS_ERROR_FAILURE); >+ return mMarkedNodes.AppendObject(aContextNode); >+} >+ >+nsresult >+nsXFormsMDGEngine::HandleMarkedNodes(nsCOMArray<nsIDOMNode> *aArray) >+{ >+ NS_ENSURE_ARG_POINTER(aArray); > >- ns->Set(kFlags_ALL_DISPATCH, PR_TRUE); >+ // Handle nodes marked as changed >+ for (PRInt32 i = 0; i < mMarkedNodes.Count(); ++i) { >+ nsCOMPtr<nsIDOMNode> node = mMarkedNodes.ObjectAt(i); >+ nsXFormsNodeState* ns = GetNCNodeState(node); >+ NS_ENSURE_TRUE(ns, NS_ERROR_FAILURE); > >- // Get the node, eMode_type == get any type of node >- nsXFormsMDGNode* n = GetNode(aContextNode, eModel_type, PR_FALSE); >- if (n) { >- while (n) { >+ ns->Set(kFlags_ALL_DISPATCH, PR_TRUE); >+ >+ // Get the node, eMode_type == get any type of node >+ nsXFormsMDGNode* n = GetNode(node, eModel_type, PR_FALSE); >+ if (n) { >+ while (n) { >+ n->MarkDirty(); >+ n = n->mNext; >+ } nit: might as well make this a do-while loop since you already check for 'n' before the loop. >+ } else { >+ // Add constraint to trigger validation of node >+ n = GetNode(node, eModel_constraint, PR_TRUE); >+ if (!n) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } > n->MarkDirty(); >- n = n->mNext; >- } >- } else { >- // Add constraint to trigger validation of node >- n = GetNode(aContextNode, eModel_constraint, PR_TRUE); >- if (!n) { >- return NS_ERROR_OUT_OF_MEMORY; >+ NS_ENSURE_TRUE(mGraph.AppendElement(n), NS_ERROR_OUT_OF_MEMORY); > } >- n->MarkDirty(); >- NS_ENSURE_TRUE(mGraph.AppendElement(n), NS_ERROR_OUT_OF_MEMORY); >+ >+ NS_ENSURE_TRUE(aArray->AppendObjects(mMarkedNodes), >+ NS_ERROR_OUT_OF_MEMORY); >+ >+ mMarkedNodes.Clear(); > } Ummm, do you want to clear mMarkedNodes everytime through the 'for' loop? I'm guessing it is meant to be called outside the loop :-) >diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.h xforms.deferevents/nsXFormsModelElement.h >--- xforms/nsXFormsModelElement.h 2006-05-15 16:11:52.000000000 +0200 >+++ xforms.deferevents/nsXFormsModelElement.h 2006-05-18 13:58:46.000000000 +0200 >@@ -373,60 +382,91 @@ private: > nsXFormsMDGEngine mMDG; > > /** > * List of changed nodes, ie. nodes that have not been informed about > * changes yet > */ > nsCOMArray<nsIDOMNode> mChangedNodes; > >- // This flag indicates whether or not the document fired DOMContentLoaded >- PRBool mDocumentLoaded; >+ /** >+ * This flag indicates whether or not the document has fired >+ * DOMContentLoaded >+ */ >+ PRPackedBool mDocumentLoaded; > >- // This flag indicates whether a xforms-rebuild has been called, but no >- // xforms-revalidate yet >- PRBool mNeedsRefresh; >+ /** >+ * Indicates whether all controls should be rebound on the next Refresh() >+ * run. >+ */ >+ PRPackedBool mNeedsRefresh; nit: controls don't get rebound due to mNeedsRefresh. Just forces all controls bound to this model to refresh. Unless you are making this comment in anticipation of a future change. None of these are major changes. With them, r=me

(In reply to comment #18) > >+ nsXFormsMDGNode* n = GetNode(node, eModel_type, PR_FALSE); > >+ if (n) { > >+ while (n) { > >+ n->MarkDirty(); > >+ n = n->mNext; > >+ } > > nit: might as well make this a do-while loop since you already check for 'n' > before the loop. I need a check for (n) no matter what, since I need the else {} part of things. > >+ mMarkedNodes.Clear(); > > } > > Ummm, do you want to clear mMarkedNodes everytime through the 'for' loop? I'm > guessing it is meant to be called outside the loop :-) Doh! Of course. > >+ /** > >+ * Indicates whether all controls should be rebound on the next Refresh() > >+ * run. > >+ */ > >+ PRPackedBool mNeedsRefresh; > > nit: controls don't get rebound due to mNeedsRefresh. Just forces all controls > bound to this model to refresh. Unless you are making this comment in > anticipation of a future change. No, just me using the wrong word.

Fixed on trunk.

(In reply to comment #19) > (In reply to comment #18) > > >+ nsXFormsMDGNode* n = GetNode(node, eModel_type, PR_FALSE); > > >+ if (n) { > > >+ while (n) { > > >+ n->MarkDirty(); > > >+ n = n->mNext; > > >+ } > > > > nit: might as well make this a do-while loop since you already check for 'n' > > before the loop. > > I need a check for (n) no matter what, since I need the else {} part of things. > > > >+ mMarkedNodes.Clear(); > > > } > > > > Ummm, do you want to clear mMarkedNodes everytime through the 'for' loop? I'm > > guessing it is meant to be called outside the loop :-) > > Doh! Of course. > > > >+ /** > > >+ * Indicates whether all controls should be rebound on the next Refresh() > > >+ * run. > > >+ */ > > >+ PRPackedBool mNeedsRefresh; > > > > nit: controls don't get rebound due to mNeedsRefresh. Just forces all controls > > bound to this model to refresh. Unless you are making this comment in > > anticipation of a future change. > > No, just me using the wrong word. Mondays are great, aren't they? Aparently I was. Bug 331452 changes it... oh well. At least there is some consistency in the checkins.