Last Comment Bug 300591 - Make recalculate, revalidate, refresh work as deferred update
: Make recalculate, revalidate, refresh work as deferred update
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1, testcase
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
http://www.w3.org/2003/10/REC-xforms-...
Depends on: 300586
Blocks: 301994 332231
  Show dependency treegraph
 
Reported: 2005-07-13 02:15 PDT by Allan Beaufour
Modified: 2006-06-06 06:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (3.71 KB, application/xhtml+xml)
2005-08-25 07:39 PDT, Allan Beaufour
no flags Details
Patch (56.80 KB, patch)
2006-05-17 05:15 PDT, Allan Beaufour
bugs: review+
Details | Diff | Review
Patch with smaug's comments (64.46 KB, patch)
2006-05-18 05:05 PDT, Allan Beaufour
aaronr: review+
Details | Diff | Review

Description Allan Beaufour 2005-07-13 02:15:39 PDT
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.
Comment 1 Allan Beaufour 2005-08-25 07:39:33 PDT
Created attachment 193813 [details]
Testcase

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.
Comment 2 Allan Beaufour 2005-08-26 03:27:11 PDT
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.
Comment 3 Allan Beaufour 2005-09-21 04:22:31 PDT
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.
Comment 4 Allan Beaufour 2006-03-08 02:03:51 PST
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.
Comment 5 Allan Beaufour 2006-03-30 06:38:17 PST
(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.
Comment 6 Allan Beaufour 2006-04-04 04:17:13 PDT
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.
Comment 7 Allan Beaufour 2006-04-25 00:39:07 PDT
(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.
Comment 8 aaronr 2006-04-25 12:30:01 PDT
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.
Comment 9 aaronr 2006-04-25 12:35:52 PDT
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.
Comment 10 Allan Beaufour 2006-04-26 00:14:31 PDT
(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).
Comment 11 Allan Beaufour 2006-05-16 08:48:05 PDT
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.
Comment 12 Allan Beaufour 2006-05-17 05:15:32 PDT
Created attachment 222336 [details] [diff] [review]
Patch
Comment 13 Allan Beaufour 2006-05-17 05:16:00 PDT
(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 14 Olli Pettay [:smaug] 2006-05-18 03:55:38 PDT
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.
Comment 15 Allan Beaufour 2006-05-18 04:33:00 PDT
(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 16 Olli Pettay [:smaug] 2006-05-18 04:35:59 PDT
Comment on attachment 222336 [details] [diff] [review]
Patch

OK, then r=me
(Most of the patch is anyway 'just' clean-ups)
Comment 17 Allan Beaufour 2006-05-18 05:05:48 PDT
Created attachment 222489 [details] [diff] [review]
Patch with smaug's comments
Comment 18 aaronr 2006-05-21 22:39:37 PDT
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
Comment 19 Allan Beaufour 2006-05-22 01:48:15 PDT
(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.
Comment 20 Allan Beaufour 2006-05-22 01:51:46 PDT
Fixed on trunk.
Comment 21 Allan Beaufour 2006-05-22 02:01:53 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.