Last Comment Bug 282828 - Modify repeat-index on insert and delete
: Modify repeat-index on insert and delete
Status: RESOLVED FIXED
: fixed1.8.0.4, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
http://www.w3.org/TR/xforms/slice9.ht...
Depends on:
Blocks: 264329 322255 326556 332853
  Show dependency treegraph
 
Reported: 2005-02-19 01:05 PST by Allan Beaufour
Modified: 2016-07-15 14:46 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Testcase 1 (2.02 KB, application/xhtml+xml)
2005-08-01 06:38 PDT, Allan Beaufour
no flags Details
Testcase 2 (1.44 KB, application/xhtml+xml)
2005-08-01 06:39 PDT, Allan Beaufour
no flags Details
Testcase 3 (1.90 KB, application/xhtml+xml)
2005-08-01 06:39 PDT, Allan Beaufour
no flags Details
Test form to show insert/delete and setindex working (1.82 KB, application/xhtml+xml)
2005-09-02 21:15 PDT, Peter Nunn
no flags Details
Added constants found in files to header (126 bytes, patch)
2005-09-02 21:16 PDT, Peter Nunn
no flags Details | Diff | Splinter Review
Changed common names to use constants in header, added code for 9.3.8 compliance (575 bytes, patch)
2005-09-02 21:16 PDT, Peter Nunn
no flags Details | Diff | Splinter Review
Added support to set index on all repeats that have a relevant row (4.98 KB, patch)
2005-09-02 21:17 PDT, Peter Nunn
no flags Details | Diff | Splinter Review
Patch to resolve issue (10.93 KB, patch)
2005-09-03 02:38 PDT, Peter Nunn
no flags Details | Diff | Splinter Review
Converted file to be unix line endings and no tabs. (10.93 KB, patch)
2005-09-03 14:55 PDT, Peter Nunn
allan: review-
Details | Diff | Splinter Review
Up-to-date patch (8.85 KB, patch)
2006-03-21 08:52 PST, Allan Beaufour
no flags Details | Diff | Splinter Review
Patch (16.90 KB, patch)
2006-03-22 09:00 PST, Allan Beaufour
doronr: review+
Details | Diff | Splinter Review
Patch w/Doron's comments fixed (16.90 KB, patch)
2006-03-22 09:28 PST, Allan Beaufour
aaronr: review+
Details | Diff | Splinter Review

Description Allan Beaufour 2005-02-19 01:05:08 PST
The repeat-index needs to be updated to point to the newly inserted node on an
insert, and also point to the correct node after a delete.
Comment 1 Allan Beaufour 2005-08-01 06:38:20 PDT
Created attachment 191195 [details]
Testcase 1
Comment 2 Allan Beaufour 2005-08-01 06:39:29 PDT
Created attachment 191197 [details]
Testcase 2

(filed bug 302922 for the index error in this)
Comment 3 Allan Beaufour 2005-08-01 06:39:49 PDT
Created attachment 191198 [details]
Testcase 3
Comment 4 Allan Beaufour 2005-08-01 06:41:13 PDT
(In reply to comment #2)
> Created an attachment (id=191197) [edit]
> Testcase 2
> 
> (filed bug 302922 for the index error in this)

argh, that is bug 302918.
Comment 5 Peter Nunn 2005-09-02 21:14:09 PDT
Have sent fixes as patch to A.Beaufor for integration, added patches here.
Comment 6 Peter Nunn 2005-09-02 21:15:18 PDT
Created attachment 194738 [details]
Test form to show insert/delete and setindex working
Comment 7 Peter Nunn 2005-09-02 21:16:05 PDT
Created attachment 194739 [details] [diff] [review]
Added constants found in files to header
Comment 8 Peter Nunn 2005-09-02 21:16:59 PDT
Created attachment 194740 [details] [diff] [review]
Changed common names to use constants in header, added code for 9.3.8 compliance
Comment 9 Peter Nunn 2005-09-02 21:17:54 PDT
Created attachment 194741 [details] [diff] [review]
Added support to set index on all repeats that have a relevant row
Comment 10 Olli Pettay [:smaug] 2005-09-03 00:40:51 PDT
(In reply to comment #5)
> Have sent fixes as patch to A.Beaufor for integration, added patches here.

Could you please create one patch and use -u8p:
cvs diff -u8p extensions/xforms/
or
diff -U 8 -p original/mozilla/code/extensions/xforms your/code/extensions/xforms

And use unix line separators '\n', not Windows '\r\n'.
Comment 11 Peter Nunn 2005-09-03 02:38:27 PDT
Created attachment 194751 [details] [diff] [review]
Patch to resolve issue
Comment 12 Peter Nunn 2005-09-03 14:51:51 PDT
Comment on attachment 194751 [details] [diff] [review]
Patch to resolve issue

Index: extensions/xforms/.cvsignore
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/.cvsignore,v
retrieving revision 1.2
diff -u -8 -p -r1.2 .cvsignore
--- extensions/xforms/.cvsignore	13 Oct 2004 09:51:52 -0000	1.2
+++ extensions/xforms/.cvsignore	3 Sep 2005 21:37:01 -0000
@@ -1 +1,5 @@
 Makefile
+xforms.sln
+xforms.vcproj
+xforms.ncb
+xforms.suo
Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInsertDeleteElement.cpp,v
retrieving revision 1.3
diff -u -8 -p -r1.3 nsXFormsInsertDeleteElement.cpp
--- extensions/xforms/nsXFormsInsertDeleteElement.cpp	2 Aug 2005 14:56:42
-0000	    1.3
+++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	3 Sep 2005 21:30:23
-0000
@@ -34,23 +34,26 @@
  *
  * ***** END LICENSE BLOCK ***** */

 #include "nsIDOMEvent.h"
 #include "nsIDOMNode.h"
 #include "nsIDOMElement.h"
 #include "nsIDOMDocument.h"
 #include "nsIDOMNodeList.h"
+#include "nsIXFormsRepeatElement.h"
+#include "nsIXformsControl.h"

 #include "nsString.h"

 #include "nsIInstanceElementPrivate.h"
 #include "nsXFormsActionModuleBase.h"
 #include "nsXFormsActionElement.h"
 #include "nsXFormsUtils.h"
+#include "nsIDOM3Node.h"

 #include "math.h"

 #ifdef DEBUG
 //#define DEBUG_XF_INSERTDELETE
 #endif

 /**
@@ -58,18 +61,16 @@
  *
  * @see http://www.w3.org/TR/xforms/slice9.html#action-insert
  *
  * @todo The spec. states that the events must set their Context Info to:
  * "Path expression used for insert/delete (xsd:string)" (XXX)
  * @see http://www.w3.org/TR/xforms/slice4.html#evt-insert
  * @see https://bugzilla.mozilla.org/show_bug.cgi?id=280423
  *
- * @todo Any \<repeat\> elements need to set their repeat-indexes properly if
- * they are bound to the same nodeset. (XXX)
  */
 class nsXFormsInsertDeleteElement : public nsXFormsActionModuleBase
 {
 private:
   PRBool mIsInsert;

 public:
   NS_DECL_NSIXFORMSACTIONMODULEELEMENT
@@ -216,22 +217,130 @@ nsXFormsInsertDeleteElement::HandleActio
     NS_ENSURE_SUCCESS(rv, rv);
   }

   // Dispatch xforms-insert/delete event to the instance node we have modified
   // data for
   nsCOMPtr<nsIDOMElement> modelElem = do_QueryInterface(model);
   NS_ENSURE_STATE(modelElem);

+  // update all repeats with the inserted/deleted node
+  nsCOMPtr<nsIDOMDocument> document;
+
+  rv = mElement->GetOwnerDocument(getter_AddRefs(document));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // get all of the repeats.
+  nsCOMPtr<nsIDOMNodeList> repeatNodes;
+  document->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
+    NS_LITERAL_STRING(NS_REPEAT), getter_AddRefs(repeatNodes));
+
+  // work over each node and if the node contains the deleted element,
+  // we assume that the node will no longer be in the tree, so apply the rules
+  // defined in 9.3.6
+  PRUint32 nodeCount;
+  rv = repeatNodes->GetLength(&nodeCount);
+
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  for(PRUint32 node = 0; node < nodeCount; node++)
+  {
+    nsCOMPtr<nsIDOMNode> repeatNode;
+
+    rv = repeatNodes->Item(node, getter_AddRefs(repeatNode));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // try cast
+    nsCOMPtr<nsIXFormsRepeatElement>
repeatElement(do_QueryInterface(repeatNode));
+    NS_ENSURE_STATE(repeatElement);
+
+    // each row is bound to the repeat as an anonymous child of the repeat.
+    // to find the context elements we need to get the html element
+    // then find the xforms:contextcontainer element which holds the child
+    // elements, but most importantly the context of the node that
+    // is bound.  If that context is the node being removed then we
+    // adjust the index.  When the node is refreshed, the index will then
point
+    // to the new node.
+
+    if(mIsInsert)
+    {
+      // AN issue here.  If we are inserting into the node, the nodeset will
+      // not have the new node in the list of inserted elements as the XPath
expression has
+      // not been evaluated.  We call the refresh on the repeat so we can
+      // evaluate the node binding after the xpath is reset.  This is not
really nice
+      // as it would be better if this is deferred till a model refresh.
+      nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(repeatElement));
+      NS_ENSURE_STATE(control);
+
+      rv = control->Refresh();
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    nsCOMPtr<nsIXTFVisual> visualElement(do_QueryInterface(repeatNode));
+    NS_ENSURE_STATE(repeatElement);
+
+    nsCOMPtr<nsIDOMElement> visualContext;
+    rv = visualElement->GetVisualContent(getter_AddRefs(visualContext));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // all of the child nodes of the type
+    nsCOMPtr<nsIDOMNodeList> childNodes;
+
+    rv =
visualContext->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
+					       
NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),
+						getter_AddRefs(childNodes));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    PRUint32 childCount;
+    rv = childNodes->GetLength(&childCount);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsCOMPtr<nsIDOMNode> childNode;
+    for(PRUint32 childIndex = 0; childIndex < childCount; childIndex++)
+    {
+      rv = childNodes->Item(childIndex, getter_AddRefs(childNode));
+
+      nsCOMPtr<nsIXFormsContextControl>
riContext(do_QueryInterface(childNode));
+      NS_ENSURE_STATE(riContext);
+
+      // see what the context of the node is.	If it is our deleted node then
+      // we adjust the index so that when the control refreshes,
+      // we see the node prior or following being selected.
+      nsAutoString modelID;
+      PRInt32 position, size;
+      nsCOMPtr<nsIDOMNode> boundNode;
+
+      rv = riContext->GetContext(modelID, getter_AddRefs(boundNode),
&position, &size);
+
+      // if the node is the same node then we have found the deleted node on
this repeat.
+      nsCOMPtr<nsIDOM3Node> changedNode(do_QueryInterface(resNode));
+      NS_ENSURE_STATE(changedNode);
+
+      PRBool isSameNode;
+      changedNode->IsSameNode(boundNode, &isSameNode);
+      if(isSameNode)
+      {
+	 PRUint32 indexValue = childIndex + (mIsInsert ? 1 : 0);
+
+	 // we ask the node t refresh at this point so that the
+	 // inner indices are reset to 1.
+	 rv = repeatElement->SetIndex(&indexValue, PR_FALSE);
+	 NS_ENSURE_SUCCESS(rv, rv);
+
+	 break;        // all done on this repeat
+      }
+    }
+  }
+
   nsCOMPtr<nsIDOMNode> instNode;
   rv = nsXFormsUtils::GetInstanceNodeForData(resNode, model,
getter_AddRefs(instNode));
   NS_ENSURE_SUCCESS(rv, rv);

   rv = nsXFormsUtils::DispatchEvent(instNode,
-				     mIsInsert ? eEvent_Insert :
eEvent_Delete);
+    mIsInsert ? eEvent_Insert : eEvent_Delete);
   NS_ENSURE_SUCCESS(rv, rv);

   // Dispatch refreshing events to the model
   if (aParentAction) {
     aParentAction->SetRebuild(modelElem, PR_TRUE);
     aParentAction->SetRecalculate(modelElem, PR_TRUE);
     aParentAction->SetRevalidate(modelElem, PR_TRUE);
     aParentAction->SetRefresh(modelElem, PR_TRUE);
Index: extensions/xforms/nsXFormsRepeatElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRepeatElement.cpp,v
retrieving revision 1.22
diff -u -8 -p -r1.22 nsXFormsRepeatElement.cpp
--- extensions/xforms/nsXFormsRepeatElement.cpp 23 Aug 2005 11:00:50 -0000     
1.22
+++ extensions/xforms/nsXFormsRepeatElement.cpp 3 Sep 2005 21:30:36 -0000
@@ -448,17 +448,22 @@ nsXFormsRepeatElement::SetIndex(PRUint32
   if (mIsParent) {
     NS_ASSERTION(mCurrentRepeat, "How can we be a repeat parent without a
child?");
     // We're the parent of nested repeats, set through the correct repeat
     return mCurrentRepeat->SetIndex(aIndex, aIsRefresh);
   }

   // Do nothing if we are not showing anything
   if (mMaxIndex == 0)
+  {
+    // 9.3.6 states that he index position becomes 0 if there are
+    // no elements in the repeat.
+    mCurrentIndex = 0;
     return NS_OK;
+  }

   if (aIsRefresh && !mCurrentIndex) {
     // If we are refreshing, get existing index value from parent
     NS_ASSERTION(mParent, "SetIndex with aIsRefresh == PR_TRUE for a
non-nested repeat?!");
     rv = mParent->GetIndex(aIndex);
     NS_ENSURE_SUCCESS(rv, rv);
   }

@@ -486,17 +491,19 @@ nsXFormsRepeatElement::SetIndex(PRUint32
   // Set the repeat-index
   rv = SetChildIndex(*aIndex, PR_TRUE, aIsRefresh);
   NS_ENSURE_SUCCESS(rv, rv);

   // Unset previous repeat-index
   if (mCurrentIndex) {
     // We had the previous selection, unset directly
     SetChildIndex(mCurrentIndex, PR_FALSE, aIsRefresh);
-  } if (mParent) {
+  }
+  
+  if (mParent) {
     // Selection is in another repeat, inform parent (it will inform the
     // previous owner of its new state)
     rv = mParent->SetCurrentRepeat(this, *aIndex);
     NS_ENSURE_SUCCESS(rv, rv);
   }

   // Set current index to new value
   mCurrentIndex = *aIndex;
@@ -717,18 +724,18 @@ nsXFormsRepeatElement::Refresh()
   nsCOMPtr<nsIDOMDocument> domDoc;
   rv = mHTMLElement->GetOwnerDocument(getter_AddRefs(domDoc));
   NS_ENSURE_SUCCESS(rv, rv);

   mMaxIndex = contextSize;
   for (PRUint32 i = 1; i < mMaxIndex + 1; ++i) {
     // Create <contextcontainer>
     nsCOMPtr<nsIDOMElement> riElement;
-    rv =
domDoc->CreateElementNS(NS_LITERAL_STRING("http://www.w3.org/2002/xforms"),
-				  NS_LITERAL_STRING("contextcontainer"),
+    rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
+				 
NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),
				  getter_AddRefs(riElement));
     NS_ENSURE_SUCCESS(rv, rv);

     // Set model as attribute
     if (!modelID.IsEmpty()) {
       riElement->SetAttribute(NS_LITERAL_STRING("model"), modelID);
     }	

Index: extensions/xforms/nsXFormsUtils.h
===================================================================
RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v
retrieving revision 1.30
diff -u -8 -p -r1.30 nsXFormsUtils.h
--- extensions/xforms/nsXFormsUtils.h	23 Aug 2005 11:00:50 -0000	1.30
+++ extensions/xforms/nsXFormsUtils.h	3 Sep 2005 21:30:50 -0000
@@ -56,16 +56,18 @@ class nsString;
 class nsIMutableArray;
 class nsIDOMEvent;

 #define NS_NAMESPACE_XFORMS		  "http://www.w3.org/2002/xforms"
 #define NS_NAMESPACE_XHTML		  "http://www.w3.org/1999/xhtml"
 #define NS_NAMESPACE_XML_SCHEMA	  "http://www.w3.org/2001/XMLSchema"
 #define NS_NAMESPACE_XML_SCHEMA_INSTANCE
"http://www.w3.org/2001/XMLSchema-instance"
 #define NS_NAMESPACE_MOZ_XFORMS_TYPE	 
"http://www.mozilla.org/projects/xforms/2005/type"
+#define NS_REPEAT_CONTEXTCONTAINER	  "contextcontainer"
+#define NS_REPEAT			  "repeat"

 /**
  * XForms event types
  *
  * @see http://www.w3.org/TR/xforms/slice4.html#rpm-events
  *
  * @note nsXFormsModelElement::SetSingleState() assumes a specific order of
  * the events from eEvent_Valid to eEvent_Disabled.
Comment 13 Peter Nunn 2005-09-03 14:55:41 PDT
Created attachment 194787 [details] [diff] [review]
Converted file to be unix line endings and no tabs.

Sorry to all on the mailing list for repeated uploads.	Have got it right now
:-)
Comment 14 Allan Beaufour 2005-09-15 05:28:54 PDT
So which one of all these is the correct patch? Could you please set the review
flag on the correct one?
Comment 15 Allan Beaufour 2005-09-21 06:44:42 PDT
Comment on attachment 194787 [details] [diff] [review]
Converted file to be unix line endings and no tabs.

Please fix issues in:
http://lab.cph.novell.com/~abeaufour/jst-review/?patch=194787

> Index: extensions/xforms/.cvsignore
> ===================================================================
> +xforms.sln
> +xforms.vcproj
> +xforms.ncb
> +xforms.suo

I think you'll have to ignore that yourself.

> Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInsertDeleteElement.cpp,v
> retrieving revision 1.3
> diff -u -8 -p -r1.3 nsXFormsInsertDeleteElement.cpp
> --- extensions/xforms/nsXFormsInsertDeleteElement.cpp	2 Aug 2005 14:56:42 -0000	1.3
> +++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	3 Sep 2005 21:30:23 -0000
> @@ -34,23 +34,26 @@
>   *
>   * ***** END LICENSE BLOCK ***** */

>  #include "nsIDOMEvent.h"
>  #include "nsIDOMNode.h"
>  #include "nsIDOMElement.h"
>  #include "nsIDOMDocument.h"
>  #include "nsIDOMNodeList.h"
> +#include "nsIXFormsRepeatElement.h"
> +#include "nsIXformsControl.h"

nsIXFormsControl.h, proper file systems are case-sensitive you know :)

> +  // get all of the repeats.
> +  nsCOMPtr<nsIDOMNodeList> repeatNodes;
> +  document->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +    NS_LITERAL_STRING(NS_REPEAT), getter_AddRefs(repeatNodes));

NS_REPEAT is overdoing it.

> +  for(PRUint32 node = 0; node < nodeCount; node++)
> +  {
> +    nsCOMPtr<nsIDOMNode> repeatNode;
> +
> +    rv = repeatNodes->Item(node, getter_AddRefs(repeatNode));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // try cast
> +    nsCOMPtr<nsIXFormsRepeatElement> repeatElement(do_QueryInterface(repeatNode));
> +    NS_ENSURE_STATE(repeatElement);
> +
> +    // each row is bound to the repeat as an anonymous child of the repeat.
> +    // to find the context elements we need to get the html element
> +    // then find the xforms:contextcontainer element which holds the child
> +    // elements, but most importantly the context of the node that
> +    // is bound.  If that context is the node being removed then we
> +    // adjust the index.  When the node is refreshed, the index will then point
> +    // to the new node.
> +
> +    if(mIsInsert)
> +    {

curly braces go on same line as the 'if', same goes for switch, case, etc.
Please change that in the entire patch.

> +      // AN issue here.  If we are inserting into the node, the nodeset will

AN? bad CaSiNg? :)

> +      // not have the new node in the list of inserted elements as the XPath expression has
> +      // not been evaluated.  We call the refresh on the repeat so we can
> +      // evaluate the node binding after the xpath is reset.  This is not really nice
> +      // as it would be better if this is deferred till a model refresh.

Add an XXX before the comment when it is something that should be fixed.

For an insert in an action block this is wrong. You should fix it so that you
only rebind the repeat, cache the nodeset, and test for the node in that.


> +      nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(repeatElement));
> +      NS_ENSURE_STATE(control);
> +
> +      rv = control->Refresh();
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }

You refresh _all_ repeats on an insert? This is bad performance-wise.

> +
> +    nsCOMPtr<nsIXTFVisual> visualElement(do_QueryInterface(repeatNode));
> +    NS_ENSURE_STATE(repeatElement);
> +
> +    nsCOMPtr<nsIDOMElement> visualContext;
> +    rv = visualElement->GetVisualContent(getter_AddRefs(visualContext));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // all of the child nodes of the type
> +    nsCOMPtr<nsIDOMNodeList> childNodes;
> +
> +    rv = visualContext->GetElementsByTagNameNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +                                               NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),
> +                                               getter_AddRefs(childNodes));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    PRUint32 childCount;
> +    rv = childNodes->GetLength(&childCount);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIDOMNode> childNode;
> +    for(PRUint32 childIndex = 0; childIndex < childCount; childIndex++)
> +    {
> +      rv = childNodes->Item(childIndex, getter_AddRefs(childNode));
> +
> +      nsCOMPtr<nsIXFormsContextControl> riContext(do_QueryInterface(childNode));
> +      NS_ENSURE_STATE(riContext);
> +
> +      // see what the context of the node is.  If it is our deleted node then
> +      // we adjust the index so that when the control refreshes,
> +      // we see the node prior or following being selected.
> +      nsAutoString modelID;
> +      PRInt32 position, size;
> +      nsCOMPtr<nsIDOMNode> boundNode;
> +
> +      rv = riContext->GetContext(modelID, getter_AddRefs(boundNode), &position, &size);
> +
> +      // if the node is the same node then we have found the deleted node on this repeat.
> +      nsCOMPtr<nsIDOM3Node> changedNode(do_QueryInterface(resNode));
> +      NS_ENSURE_STATE(changedNode);
> +
> +      PRBool isSameNode;
> +      changedNode->IsSameNode(boundNode, &isSameNode);
> +      if(isSameNode)
> +      {
> +        PRUint32 indexValue = childIndex + (mIsInsert ? 1 : 0);
> +
> +        // we ask the node t refresh at this point so that the
> +        // inner indices are reset to 1.
> +        rv = repeatElement->SetIndex(&indexValue, PR_FALSE);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        break;        // all done on this repeat
> +      }
> +    }
> +  }
> +

How about nested repeats? I'm not sure that this works correctly for them...
I'm not actually sure whether the exact behvaviour is clearly defined :(

Doing this through the DOM is expensive. I opt for the above approach of
caching the nodeset from the XPath result.


>    rv = nsXFormsUtils::DispatchEvent(instNode,
> -                                    mIsInsert ? eEvent_Insert : eEvent_Delete);
> +    mIsInsert ? eEvent_Insert : eEvent_Delete);

Don't change the indent.

> Index: extensions/xforms/nsXFormsRepeatElement.cpp
> ===================================================================
>    // Do nothing if we are not showing anything
>    if (mMaxIndex == 0)
> +  {
> +    // 9.3.6 states that he index position becomes 0 if there are
> +    // no elements in the repeat.
> +    mCurrentIndex = 0;
>      return NS_OK;
> +  }

Yes, good catch. Hmmm, but are controls using indexes actually updated? I do
not think so.

>    mMaxIndex = contextSize;
>    for (PRUint32 i = 1; i < mMaxIndex + 1; ++i) {
>      // Create <contextcontainer>
>      nsCOMPtr<nsIDOMElement> riElement;
> -    rv = domDoc->CreateElementNS(NS_LITERAL_STRING("http://www.w3.org/2002/xforms"),
> -                                 NS_LITERAL_STRING("contextcontainer"),
> +    rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +                                 NS_LITERAL_STRING(NS_REPEAT_CONTEXTCONTAINER),

Good fix to use the namespace define, but NS_REPEAT_CONTEXTCONTAINER is
overdoing it.
Comment 16 Matthijs Wensveen 2005-11-28 01:08:57 PST
Don't know if this is the right place to discuss this (if not ignore me), but shouldn't the repeat-index element be selected by a pseudo-element selector (r_l1::repeat-index and r_l2::repeat-index in testcase 3)? See XForms spec appendix F.
Or is this a matter of CSS(3) support for Mozilla/Firefox in general?
Comment 17 Allan Beaufour 2005-11-28 02:08:27 PST
(In reply to comment #16)
> Don't know if this is the right place to discuss this (if not ignore me), but
> shouldn't the repeat-index element be selected by a pseudo-element selector
> (r_l1::repeat-index and r_l2::repeat-index in testcase 3)? See XForms spec
> appendix F.
> Or is this a matter of CSS(3) support for Mozilla/Firefox in general?

It's a matter of general CSS support for Mozilla/Firefox. We do not have support for the pseudoelements yet. Bug 271724 is tracking that. We have temporarily implemented the functionality through classes, in bug 312980.
Comment 18 Allan Beaufour 2006-03-21 08:52:00 PST
Created attachment 215785 [details] [diff] [review]
Up-to-date patch

Attachment 194787 [details] [diff] brought up-to-date, and fixed nits and stuff -- my own review comments basically :)
Comment 19 Allan Beaufour 2006-03-22 09:00:15 PST
Created attachment 215905 [details] [diff] [review]
Patch

Here's a go. As opposed to Peter's approach I moved the handling responsibility of handling to the repeat element itself.

There are still optimizations and fixes that need to be done, but this works for simple use-cases, and should have been fixed ages ago (by me ....). So let's get this in, and hunt for nested repeat bugs later.
Comment 20 Doron Rosenberg (IBM) 2006-03-22 09:19:58 PST
Comment on attachment 215905 [details] [diff] [review]
Patch

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsInsertDeleteElement.cpp xforms.282828/nsXFormsInsertDeleteElement.cpp
>--- xforms/nsXFormsInsertDeleteElement.cpp	2006-02-23 12:19:46.000000000 +0100
>+++ xforms.282828/nsXFormsInsertDeleteElement.cpp	2006-03-22 16:26:22.000000000 +0100
>   
>   // Do nothing if we are not showing anything
>-  if (mMaxIndex == 0)
>+  if (mMaxIndex == 0) {
>+    // 9.3.6 states that he index position becomes 0 if there are
>+    // no elements in the repeat.

s/he/the

>+  SanitizeIndex(aIndex, PR_TRUE);

couldn't aIndex be an uninitialzed variable? Or do we make sure that never happens in the callers?

> 
>+NS_IMETHODIMP
>+nsXFormsRepeatElement::HandleNodeInsert(nsIDOMNode *aNode)
>+{
>+  NS_ENSURE_STATE(mHTMLElement);
>+
>+  nsCOMPtr<nsIDOM3Node> node(do_QueryInterface(aNode));
>+  NS_ENSURE_STATE(node);
>+
>+  nsresult rv;
>+  // XXX, badness^2: If it is a insert we have to refresh before we can
>+  // figure out whether the node is in our nodeset... refactor this so
>+  // repeat actually gets the nodeset in Bind() and then uses it refresh,
>+  // then we can "just" re-evaluate the nodeset, and only refresh if the
>+  // node actually hits this repeat
>+
>+  // XXX, moreover it is also wrong to refresh at this point. It will happen
>+  // in insert processing (and possibly deferred...)
>+  rv = Refresh();
>+  NS_ENSURE_SUCCESS(rv, rv);

why not nsresult rv = Refresh() rather than in 2 lines?

>+  nsCOMPtr<nsIDOMNode> child;
>+  mHTMLElement->GetFirstChild(getter_AddRefs(child));
>+
>+  PRUint32 index = 1;
>+  while (child) {
>+    nsCOMPtr<nsIXFormsContextControl> context(do_QueryInterface(child));
>+    NS_ASSERTION(context,
>+                 "repeat child not implementing nsIXFormsContextControl?!");
>+

Perhaps do a nodeType check if its a ELEMENT_NODE before doing the QI?  Might be faster?

>+    nsAutoString modelID;
>+    PRInt32 position, size;
>+    nsCOMPtr<nsIDOMNode> boundNode;
>+    rv = context->GetContext(modelID, getter_AddRefs(boundNode), &position,
>+                             &size);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    PRBool sameNode = PR_FALSE;
>+    node->IsSameNode(boundNode, &sameNode);
>+    if (sameNode) {
>+      rv = SetIndex(&index, PR_FALSE);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      break;
>+    }

nit: newline after NS_ENSURE_SUCCESS(rv, rv); makes it more readable.
Comment 21 Allan Beaufour 2006-03-22 09:27:05 PST
(In reply to comment #20)
> (From update of attachment 215905 [details] [diff] [review] [edit])
> >+  SanitizeIndex(aIndex, PR_TRUE);
> 
> couldn't aIndex be an uninitialzed variable? Or do we make sure that never
> happens in the callers?

There's an ENSURE_ARG in the start of this function, SanitizeIndex() also starts by checking it.
 
> >+  PRUint32 index = 1;
> >+  while (child) {
> >+    nsCOMPtr<nsIXFormsContextControl> context(do_QueryInterface(child));
> >+    NS_ASSERTION(context,
> >+                 "repeat child not implementing nsIXFormsContextControl?!");
> >+
> 
> Perhaps do a nodeType check if its a ELEMENT_NODE before doing the QI?  Might
> be faster?

Well, it should never happen. Repeat children always implement nsIXFormsContextControl -- we manage them ourselves. So this will just catch (major) design changes to repeat during testing.
Comment 22 Allan Beaufour 2006-03-22 09:28:29 PST
Created attachment 215908 [details] [diff] [review]
Patch w/Doron's comments fixed
Comment 23 Steve Speicher 2006-03-27 08:16:04 PST
1.0 test suite: 9.3.6.a
Comment 24 Allan Beaufour 2006-03-28 23:39:43 PST
Checked into trunk
Comment 25 Steve Speicher 2006-04-13 11:41:12 PDT
I haven't dug through the code, but test case 9.3.6.a shows a index() == 1 when all repeat items have been removed (when it should be == 0).
Comment 26 Steve Speicher 2006-04-13 11:43:38 PDT
likewise for test suite case: 7.8.5.a...should return have: index('norepeat') == NaN, instead get 0
Comment 27 aaronr 2006-04-13 12:03:39 PDT
(In reply to comment #26)
> likewise for test suite case: 7.8.5.a...should return have: index('norepeat')
> == NaN, instead get 0
> 

That is a change in the second edition of the spec.  Used to throw a compute exception.
Comment 28 aaronr 2006-04-13 12:53:49 PDT
(In reply to comment #25)
> I haven't dug through the code, but test case 9.3.6.a shows a index() == 1 when
> all repeat items have been removed (when it should be == 0).
> 

Looks like repeat bails when there are no items to show, so never gets to the point in the code where it sets index.

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