Closed
Bug 278209
Opened 20 years ago
Closed 19 years ago
Implement <setindex>
Categories
(Core Graveyard :: XForms, defect)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: allan, Assigned: allan)
References
()
Details
Attachments
(2 files, 8 obsolete files)
54.02 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.89 KB,
application/xhtml+xml
|
Details |
We need to support the setindex tag.
Assignee | ||
Comment 1•20 years ago
|
||
This is repeat stuff, so I'll look into it.
Blocks: 264329
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•20 years ago
|
||
Need to get nested repeats fixed (bug 278962), before index gives real meaning.
Depends on: 278962
Assignee | ||
Comment 3•19 years ago
|
||
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...
Assignee | ||
Comment 4•19 years ago
|
||
... as written above, the child index is not set correctly yet, when parent is changed.
Assignee | ||
Comment 5•19 years ago
|
||
(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/...
Assignee | ||
Comment 6•19 years ago
|
||
It still has some todo-stuff, but I'm getting quite close.
Attachment #172262 -
Attachment is obsolete: true
Assignee | ||
Comment 7•19 years ago
|
||
with setindex triggers
Attachment #172263 -
Attachment is obsolete: true
Assignee | ||
Comment 8•19 years ago
|
||
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)
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #172497 -
Attachment is obsolete: true
Comment 10•19 years ago
|
||
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-
Assignee | ||
Comment 11•19 years ago
|
||
(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.
Comment 12•19 years ago
|
||
(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? :=)
Assignee | ||
Comment 13•19 years ago
|
||
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)
Assignee | ||
Comment 14•19 years ago
|
||
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 15•19 years ago
|
||
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+
Assignee | ||
Comment 16•19 years ago
|
||
(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.
Assignee | ||
Comment 17•19 years ago
|
||
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)
Comment 18•19 years ago
|
||
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 19•19 years ago
|
||
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-
Comment 20•19 years ago
|
||
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.
Comment 21•19 years ago
|
||
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.
Assignee | ||
Comment 22•19 years ago
|
||
(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
Assignee | ||
Comment 23•19 years ago
|
||
*** Bug 281748 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 24•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Attachment #172831 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #174472 -
Flags: review?(aaronr)
Comment 25•19 years ago
|
||
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 26•19 years ago
|
||
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+
Assignee | ||
Comment 27•19 years ago
|
||
(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.
Assignee | ||
Comment 28•19 years ago
|
||
(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.
Assignee | ||
Comment 29•19 years ago
|
||
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)
Assignee | ||
Comment 30•19 years ago
|
||
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
Comment 31•19 years ago
|
||
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?
Comment 32•19 years ago
|
||
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?
Comment 33•19 years ago
|
||
sorry for the duplicate comments :=)
Assignee | ||
Comment 34•19 years ago
|
||
(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."
Comment 35•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #174562 -
Flags: review?(smaug) → review+
Assignee | ||
Comment 36•19 years ago
|
||
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
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•