Closed Bug 278209 Opened 20 years ago Closed 19 years ago

Implement <setindex>

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: allan, Assigned: allan)

References

()

Details

Attachments

(2 files, 8 obsolete files)

We need to support the setindex tag.
This is repeat stuff, so I'll look into it.
Blocks: 264329
Status: NEW → ASSIGNED
Need to get nested repeats fixed (bug 278962), before index gives real meaning.
Depends on: 278962
Attached patch setIndex with rough edges (obsolete) — Splinter Review
Here's a beta-release of setindex-funtionality from my side (non XPath) :)

It's to show Aaron where I'm heading. setindex() can get the index from
nsIModelElementPrivate::setRepeatIndex(in AString index).

It "seems to work", but I need to reset children indexes when setting a parent
index for nested repeats. It probably also has some XXX's and debug-crap
around...
... as written above, the child index is not set correctly yet, when parent is
changed.
(In reply to comment #3)
> It's to show Aaron where I'm heading. setindex() can get the index from
> nsIModelElementPrivate::setRepeatIndex(in AString index).

*argh* s/setindex()/index()/ and s/setRepeatIndex/getRepeatIndex/...
Attached patch setindex and nested repeats work (obsolete) — Splinter Review
It still has some todo-stuff, but I'm getting quite close.
Attachment #172262 - Attachment is obsolete: true
Attached file Updated testcase (obsolete) —
with setindex triggers
Attachment #172263 - Attachment is obsolete: true
Attached patch Full patch (obsolete) — Splinter Review
Here goes nothing ... :)

I know there's a todo.txt, and it lacks some documentation. So I'm mostly
interested in a usage-test I guess. But of course, do not hold back code
reviews :)
Attachment #172496 - Attachment is obsolete: true
Attachment #172657 - Flags: review?(aaronr)
Attachment #172497 - Attachment is obsolete: true
Comment on attachment 172657 [details] [diff] [review]
Full patch

+  rv = SetChildIndex(*aIndex - 1, PR_TRUE);
+  NS_ENSURE_SUCCESS(rv, rv);
+  
+  // Unset previous repeat-index
+  if (mCurrentIndex) {
+    // We had the previous selection
+    SetChildIndex(mCurrentIndex  - 1, PR_FALSE);
+  } if (mParent) {
+    // Selection is in another repeat, inform it
+    rv = mParent->SetCurrentRepeat(this);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  // Set current index to new value
+  mCurrentIndex = *aIndex;

Notice that you could possibly return out of here without setting
mCurrentIndex.

+// NB: CloneNode() assumes that this always suceedes

nit: s/suceedes/succeeds.  it is in a couple of your comments.

+  strSize.AppendInt(contextSize);

really?  I have no better suggestions, but seems odd to need a 64 char buffer
for something that isn't like to be over 4 char.

 nsXFormsRepeatElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
 {
 #ifdef DEBUG_XF_REPEAT
   printf("nsXFormsRepeatElement::AttributeSet(aName=%p, aValue=%s)\n", (void*)
aName, NS_ConvertUCS2toUTF8(aValue).get());
 #endif

   if (aName == nsXFormsAtoms::bind ||
       aName == nsXFormsAtoms::nodeset ||
+      aName == nsXFormsAtoms::number ||
       aName == nsXFormsAtoms::model) {
     Refresh();
   }

You don't think a change to setindex should cause a Refresh()?

+nsXFormsRepeatElement::ResetInnerRepeats(nsIDOMNode *aNode)
+{
+  nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aNode);

-      nsCOMPtr<nsIDOMNode> contextNode;
-      rv = result->SnapshotItem(i - 1, getter_AddRefs(contextNode));
-      NS_ENSURE_SUCCESS(rv, rv);
-	 
-      nsCOMPtr<nsIDOMElement> contextElement = do_QueryInterface(contextNode);
-      NS_ENSURE_TRUE(contextElement, NS_ERROR_FAILURE);
-	 
-      rv = riContext->SetContextNode(contextElement);
-      NS_ENSURE_SUCCESS(rv, rv);
+  if (!aNode)
+    return NS_ERROR_FAILURE;
+
+  nsCOMPtr<nsIDOMNodeList> nodeList;

nit: Maybe move the QI for element below the return?

I also thought that it was odd that nsXFormsRepeatElement::SetIndex was 1 based
but nsXFormsRepeatElement::SetChildIndex is 0 based.  if nothing else, should
probably point that out in the comments to SetChildIndex when you get a chance.

+    // Set parent and level on clone
+    PRUint32 level;
+    repSource->GetLevel(&level);
+    repClone->SetLevel(level + 1);
+    repClone->SetParent(parent);

You are setting repClone's level to level+1, but setting repClone's parent to
the topmost parent?

// beaufour: This makes sense, but is not according to the spec. I guess...

Please ask David so that we are in sync with Novell's behavior on this.  Looks
like fP DOES throw a bind exception if index does not evaluate to a number. 
But they don't throw an exception (not visably, at least) if @repeat doesn't
point to a repeat element.  Which I personally think is goofy...that is the
perfect place to throw a binding exception...consistent with the binding
exceptions for @submission not pointing to a submission element.  But just
check with David to make sure that we are both on the same page, please.
Attachment #172657 - Flags: review?(aaronr) → review-
(In reply to comment #10)
> (From update of attachment 172657 [details] [diff] [review] [edit])
> +  rv = SetChildIndex(*aIndex - 1, PR_TRUE);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  // Unset previous repeat-index
> +  if (mCurrentIndex) {
> +    // We had the previous selection
> +    SetChildIndex(mCurrentIndex  - 1, PR_FALSE);
> +  } if (mParent) {
> +    // Selection is in another repeat, inform it
> +    rv = mParent->SetCurrentRepeat(this);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  // Set current index to new value
> +  mCurrentIndex = *aIndex;
> 
> Notice that you could possibly return out of here without setting
> mCurrentIndex.

Huh? Yes, if an error occurs, yes. But that is intended. Maybe I'm not following
you?

> +  strSize.AppendInt(contextSize);
> 
> really?  I have no better suggestions, but seems odd to need a 64 char buffer
> for something that isn't like to be over 4 char.

Check. nsString is better, is it not?

>  nsXFormsRepeatElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
>  {
>  #ifdef DEBUG_XF_REPEAT
>    printf("nsXFormsRepeatElement::AttributeSet(aName=%p, aValue=%s)\n", (void*)
> aName, NS_ConvertUCS2toUTF8(aValue).get());
>  #endif
> 
>    if (aName == nsXFormsAtoms::bind ||
>        aName == nsXFormsAtoms::nodeset ||
> +      aName == nsXFormsAtoms::number ||
>        aName == nsXFormsAtoms::model) {
>      Refresh();
>    }
> 
> You don't think a change to setindex should cause a Refresh()?

I guess you mean startIndex? No, startIndex is only for the first initial
showing of the repeat, so there's no need to watch for changes.

> +nsXFormsRepeatElement::ResetInnerRepeats(nsIDOMNode *aNode)
> +{
> +  nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aNode);
> nit: Maybe move the QI for element below the return?

*doh* Or more correct: check the correct variable...

> I also thought that it was odd that nsXFormsRepeatElement::SetIndex was 1 based
> but nsXFormsRepeatElement::SetChildIndex is 0 based.  if nothing else, should
> probably point that out in the comments to SetChildIndex when you get a chance.

It's not odd, it's stupid. SetChildIndex should be 1-based. 

> +    // Set parent and level on clone
> +    PRUint32 level;
> +    repSource->GetLevel(&level);
> +    repClone->SetLevel(level + 1);
> +    repClone->SetParent(parent);
> 
> You are setting repClone's level to level+1, but setting repClone's parent to
> the topmost parent?

Yeah. Better comments would help here. level = how many repeats are above us in
the anonymous content tree. But the parent is always the original repeat in the
DOM tree.

> // beaufour: This makes sense, but is not according to the spec. I guess...
> 
> Please ask David so that we are in sync with Novell's behavior on this.

I'll do.
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 172657 [details] [diff] [review] [edit] [edit])
> > +  rv = SetChildIndex(*aIndex - 1, PR_TRUE);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  
> > +  // Unset previous repeat-index
> > +  if (mCurrentIndex) {
> > +    // We had the previous selection
> > +    SetChildIndex(mCurrentIndex  - 1, PR_FALSE);
> > +  } if (mParent) {
> > +    // Selection is in another repeat, inform it
> > +    rv = mParent->SetCurrentRepeat(this);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +  }
> > +
> > +  // Set current index to new value
> > +  mCurrentIndex = *aIndex;
> > 
> > Notice that you could possibly return out of here without setting
> > mCurrentIndex.
> 
> Huh? Yes, if an error occurs, yes. But that is intended. Maybe I'm not following
> you?

If that is intended, cool.  Just wanted to make sure that you were aware of it.

> 
> > +  strSize.AppendInt(contextSize);
> > 
> > really?  I have no better suggestions, but seems odd to need a 64 char buffer
> > for something that isn't like to be over 4 char.
> 
> Check. nsString is better, is it not?

Like I said, I don't know.  I'm still learning the ins and outs of stringing in
Mozilla myself :=)

> 
> >  nsXFormsRepeatElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
> >  {
> >  #ifdef DEBUG_XF_REPEAT
> >    printf("nsXFormsRepeatElement::AttributeSet(aName=%p, aValue=%s)\n", (void*)
> > aName, NS_ConvertUCS2toUTF8(aValue).get());
> >  #endif
> > 
> >    if (aName == nsXFormsAtoms::bind ||
> >        aName == nsXFormsAtoms::nodeset ||
> > +      aName == nsXFormsAtoms::number ||
> >        aName == nsXFormsAtoms::model) {
> >      Refresh();
> >    }
> > 
> > You don't think a change to setindex should cause a Refresh()?
> 
> I guess you mean startIndex? No, startIndex is only for the first initial
> showing of the repeat, so there's no need to watch for changes.

Yep, that is what I meant.  Ok.  I guess that is the whole point of setindex,
hu? :=)
Hopefully my design should be possible to understand through the comments in
nsXFormsRepeatElement.cpp.
Attachment #172657 - Attachment is obsolete: true
Attachment #172831 - Flags: review?(aaronr)
As it's also stated in the patch, I haven't really tested what happens if
repeat-index = 0, and the likes. I'll get started on bug 278211 tomorrow
(insert/delete), which will make it easier to see exercise that. But, if it does
not work, let's do a new bug.
Comment on attachment 172831 [details] [diff] [review]
w/Aaron's comments and (better) documentation

+   * Sets whether the repeat parent to nested repeats

nit: "Sets whether the repeat *is a* parent to nested repeats"

Nice job. Can't wait to use it in my XPath stuff!

r=aaronr
Attachment #172831 - Flags: review?(aaronr) → review+
(In reply to comment #15)
> (From update of attachment 172831 [details] [diff] [review] [edit])
> Nice job. Can't wait to use it in my XPath stuff!

Thanks. But I suspect that we will have some refreshing issues, with the
refresh-everything-hack disabled wrt. to index(). But let us see.
Comment on attachment 172831 [details] [diff] [review]
w/Aaron's comments and (better) documentation

I'll fix the nit before check in.
Attachment #172831 - Flags: superreview?(bryner)
Evaluating @index will always result in a number (might be NaN). I don't think 
that should result in a xforms-binding-exception but since the case where 
@index is NaN is undefined in the specification I will take this up with the 
working group.
If the @repeat attribute don't point to a repeat I believe it is correct to 
dispatch an xforms-binding-exception. Since the specification doesn’t state 
this I will take it up with the working group (along with @case in toggle).
Comment on attachment 172831 [details] [diff] [review]
w/Aaron's comments and (better) documentation

>--- xforms/nsXFormsContextContainer.cpp	2005-01-29 10:50:52.000000000 +0100
>+++ xforms.index/nsXFormsContextContainer.cpp	2005-01-29 22:11:35.337115664 +0100
+  // nsXFormsContextContainer
+  void SetRepeatIndex(PRBool aIsIndex);

What's this about?  It doesn't seem to be implemented anywhere.  Leftover
version of IsIndex() ?

>+  // Find our context position
>+  nsString posString;
>+  mElement->GetAttribute(NS_LITERAL_STRING("contextposition"), posString);

Usually we do use nsAutoString in these cases just because stack allocation is
cheaper than heap allocation (and the object is short-lived).


>--- xforms/nsXFormsRepeatElement.cpp	2005-01-29 21:20:50.476004912 +0100
>+++ xforms.index/nsXFormsRepeatElement.cpp	2005-01-29 23:27:06.653226016 +0100
>@@ -304,131 +587,292 @@ nsXFormsRepeatElement::Refresh()
...
>+  nsString strSize;
>+  strSize.AppendInt(contextSize);
>+

nsAutoString here as well.
Attachment #172831 - Flags: superreview?(bryner) → superreview-
He had autostrings originally.  I made him second guess himself with my review
because I wondered about using 64 chars to store something that won't likely to
ever be more than 4 chars long.

SetRepeatIndex looks to be phased out.  Was in his original patch called by
nsXFormsContextContainer::HandleDefault but not subsequent patches.  Looks like
he is using SetIndex now.
I applied the patch to try it out and it looks like you have a bug.

Leave Red at 2.  If you change green to 2 then hit set, it sets green to 2,
fine.  But then if you then change yellow to 3, then green will go back and
highlight the first subcategory under category 2.  If you then press set for
green, trying to set it back to subcategory 2, then the green highlighting drops
down to subcategory 1 under category 3.
(In reply to comment #21)
> I applied the patch to try it out and it looks like you have a bug.
> 
> Leave Red at 2.  If you change green to 2 then hit set, it sets green to 2,
> fine.  But then if you then change yellow to 3, then green will go back and
> highlight the first subcategory under category 2.  If you then press set for
> green, trying to set it back to subcategory 2, then the green highlighting drops
> down to subcategory 1 under category 3.

Try tabbing through the inboxes and set the above numbers as the first thing,
and then set push the triggers. Then it works. The problem is the hack caused by
bug 278368, which makes the repeat refresh and recreate themselves _all_ the
time, and when the middle repeat (green) is recreated it initilizes itself to
the default value.

I have known all the time that bug 278368 affects this bug, but most have
forgotten to write it. Sorry.

I'll see if I cannot find a way around it.
Depends on: 278368
*** Bug 281748 has been marked as a duplicate of this bug. ***
Blocks: 281987
Here's an updated version that fixes Brian's comments and fixes the refresh
bug. 

It also fixes
* startindex (which I had previously misunderstood, and misspelled as
startIndex)
* removes unnecessary getContext in contextcontainer
* removes old debugging code from nsXFormsRepeatElement.cpp
Attachment #172831 - Attachment is obsolete: true
Attachment #174472 - Flags: review?(aaronr)
Comment on attachment 174472 [details] [diff] [review]
w/bryner's comments and fix for refresh bug

Few comments:

>+  if (!type.EqualsLiteral("focus"))
>+    return nsXFormsControlStub::HandleDefault(aEvent, aHandled);
>+

Check somewhere in the HandleDefault() that the event target is right.


>+  mElement->GetAttribute(NS_LITERAL_STRING("contextposition"), posString);
>+  NS_ASSERTION(!posString.IsEmpty(), "@contextposition == '' on repeat child?");

Nit: Not exactly right debug information. 
Either @contextposition is empty or it doesn't exist.


>+nsXFormsContextContainer::SetIndexState(PRBool aHasIndex)
>+{
>+  if (mElement) {
>+    if (aHasIndex) {
>+      mElement->SetAttribute(NS_LITERAL_STRING("repeat-index"),
>+                             NS_LITERAL_STRING("1"));
>+    } else {
>+      mElement->RemoveAttribute(NS_LITERAL_STRING("repeat-index"));
>+    }

Could use NS_NAMED_LITERAL_STRING for repeat-index.


>+
>+#include "math.h"
>+

#include <math.h>
Comment on attachment 174472 [details] [diff] [review]
w/bryner's comments and fix for refresh bug

great comments!

+  mMaxIndex = NS_MIN(contextSize, number);

really?  Why isn't mMaxIndex set to contextSize (the total size of the
nodeset)?  If that is right, then do number = NS_MIN(contextSize, number);

+  for (PRUint32 i = 1; i < mMaxIndex + 1; ++i) {

I think that mMaxIndex here should be 'number' instead.

+  if (!mParent && !mCurrentIndex && mMaxIndex) 

if my above two conclusions are right, then I don't think that you need this
test for mMaxIndex.

Otherwise looks great.	With those changes (or correcting me if I'm wrong)
r=aaronr
Attachment #174472 - Flags: review?(aaronr) → review+
(In reply to comment #25)
> (From update of attachment 174472 [details] [diff] [review] [edit])
> Few comments:
> 
> >+  if (!type.EqualsLiteral("focus"))
> >+    return nsXFormsControlStub::HandleDefault(aEvent, aHandled);
> >+
> 
> Check somewhere in the HandleDefault() that the event target is right.

Actually, the event target is always right. Because we need to change the
repeat-index whenever a child element gets focus. My comment was misleading,
I've changed it now.
(In reply to comment #26)
> (From update of attachment 174472 [details] [diff] [review] [edit])
> great comments!

Sure is.

> +  mMaxIndex = NS_MIN(contextSize, number);
> 
> really?  Why isn't mMaxIndex set to contextSize (the total size of the
> nodeset)?  If that is right, then do number = NS_MIN(contextSize, number);

I'm not sure I understand your comment. The spec says:
"number = Optional hint to the XForms Processor as to how many elements from the
collection to display."
(http://www.w3.org/TR/xforms/slice9.html#ui-repeat)

So, as I'm not going to display more that |number| elements, I define mMaxIndex
to that: the maximum number of elements that I will display. I can ignore the
total size of the set for display purposes.
This one has the nits from smaug, and also implements
nsXFormsRepeatElement::TryFocus (I first noticed the todo today...).

If smaug's ok with it, and Aaron does not have good arguments for mMaxIndex,
this should be ready.
Attachment #174472 - Attachment is obsolete: true
Attachment #174562 - Flags: review?(smaug)
Here's the same test-case with some better styling, so it's possible to easier
see repeats and repeat-items. It also works in the Novell IE plugin.
Attachment #172658 - Attachment is obsolete: true
I agree, you shouldn't display more than 'number'.  But you also use mMaxIndex
as the upper bound during nsXFormsRepeatElement::SetIndex.  Just because there
are only 3 contextcontainers visible at one time, I should still be able to set
the index to 10 if there are 20 total context containers.  So in this case,
mMaxIndex should be 20, right?
I agree, you shouldn't display more than 'number'.  But you also use mMaxIndex
as the upper bound during nsXFormsRepeatElement::SetIndex.  Just because there
are only 3 contextcontainers visible at one time, I should still be able to set
the index to 10 if there are 20 total context containers.  So in this case,
mMaxIndex should be 20, shouldn't it?
sorry for the duplicate comments :=)
(In reply to comment #31)
> I agree, you shouldn't display more than 'number'.  But you also use mMaxIndex
> as the upper bound during nsXFormsRepeatElement::SetIndex.  Just because there
> are only 3 contextcontainers visible at one time, I should still be able to set
> the index to 10 if there are 20 total context containers.  So in this case,
> mMaxIndex should be 20, right?

Setting the index to something that is not shown does not make much sense for
me. But a way to see it could be: I'm only displaying context position 1 to 3
(out of 20), so if I set the index to 7 for example, I should refresh the repeat
and display 7-9?

I'll use my standard answer :) : "Yeah yeah, let's get this one in and create a
new bug for the above, if it is valid."
That's fine.  I've got a testcase somewhere here that shows it off.  I'll just
try it out once this goes in and if it fails, I'll open the bug and then you can
make sure that we handle the visibility like Novell does.
Attachment #174562 - Flags: review?(smaug) → review+
Ok, @number is more tricky than I thought, and it is only a hint, so I killed
the handling of it.

Checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
No longer blocks: 281987
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: