repeat index() doesn't reset to 0 when last repeat item is deleted

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: Steve Speicher, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

1.20 KB, application/xhtml+xml
Details
2.07 KB, application/xhtml+xml
Details
2.10 KB, application/xhtml+xml
Details
5.60 KB, patch
smaug
: review+
aaronr
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060411 Firefox/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060411 Firefox/3.0a1

1.0 2ed test suite case 9.3.6.a shows a index() == 1 when
all repeat items have been removed (when it should be == 0)

See related bug 282828

Reproducible: Always
(Reporter)

Comment 1

11 years ago
Created attachment 218443 [details]
testcase

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

11 years ago
Blocks: 322255
(Assignee)

Updated

11 years ago
Blocks: 331209

Comment 2

11 years ago
An output that uses @value="index('repeater')" currently isn't guarenteed to have the correct value.

Looks like the problem is that output evaluates the xpath during ::Bind (though not displayed until ::Refresh) and repeat doesn't set index until its ::Refresh.  But even if output evaluated it at refresh, it still might not be right, depending if its refresh was before repeats or not.

Seems like something for the xpath dependency stuff.  Otherwise a hack would be that any control who asks for index() should get registered with the repeat somehow?  Yuck.  I don't think that would be easy to keep that hack on the XForms side of the fence w/o doing string parsing.  And it isn't just controls, also xf:bind.

I don't know if this is related to the other index bugs.  Didn't seem like it when I read the other abstracts.  For what it is worth, the repeat index seems to be right.

Comment 3

11 years ago
Created attachment 220090 [details]
testcase 2

This testcase also shows the problem.  Notice that adding items seems ok, but then deleting items will always have index showing as 1 off.  Again, the index is right on the repeat by the time its refresh method returns.  But since the output asked for the index before repeat had changed it, the value the user sees is wrong.

Updated

11 years ago
Attachment #220090 - Attachment mime type: text/html → application/xhtml+xml
(Assignee)

Comment 4

11 years ago
(In reply to comment #2)
> Seems like something for the xpath dependency stuff.  Otherwise a hack would be
> that any control who asks for index() should get registered with the repeat
> somehow?  Yuck.  I don't think that would be easy to keep that hack on the
> XForms side of the fence w/o doing string parsing.

It is yuck, but it is unavoidable, and we have it already. It is not handled by the MDG, because the index is this "magic thing" hanging around without being bound to the instance data. The same goes for switch state. There are probably amplications, but they should possibly have been done like instance('internal')/repeat_index/<id> and instance('internal')/swith_state/<id>

> And it isn't just controls, also xf:bind.

Bug 292333.

The problem is probably that the IndexHasChanged() code is not run when index reached zero. /me wonders it this is fixed by bug 331452.
(Assignee)

Updated

11 years ago
Assignee: aaronr → allan
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 5

11 years ago
There seems to be generic index problems. Probably regressed from bug 331452.
(Assignee)

Comment 6

11 years ago
Created attachment 223013 [details]
setindex on empty nodeset testcase

Related problem showing up, when we actually manage to get index to be 0.
(Assignee)

Comment 7

11 years ago
Created attachment 223014 [details] [diff] [review]
Patch

Also fixes the setindex problem (testcase in attachment 223013 [details]).
(Assignee)

Updated

11 years ago
Attachment #223014 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 8

11 years ago
Comment on attachment 223014 [details] [diff] [review]
Patch

I think I have a better patch coming up, which also takes care of bug 318779 too.
Attachment #223014 - Attachment is obsolete: true
Attachment #223014 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 9

11 years ago
Created attachment 223027 [details] [diff] [review]
Patch

Problems were that I forgot to handle the empty nodeset case, and that I forgot to inform about index changes on all cases.

(the main problem is that the code is awful, but I do not have time to rewrite it, as it should be... :( )
Attachment #223027 - Flags: review?(Olli.Pettay)

Comment 10

11 years ago
Comment on attachment 223027 [details] [diff] [review]
Patch

r=me, I think :)
Attachment #223027 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

11 years ago
Attachment #223027 - Flags: review?(aaronr)

Updated

11 years ago
Attachment #223027 - Flags: review?(aaronr) → review+
(Assignee)

Comment 11

11 years ago
Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Blocks: 318779
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.