269132 - Implementation of repeat without attribute-based repeats

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Comment 1

[Allan Beaufour]

Attachment: First patch for repeat implementation

Here's a go at repeat. It is implemented by expanding its children with the use
of a pseudo repeatitem element. This is explained in nsXFormsRepeatElement.cpp.


It does not support attribute-based repeats, that's a further step that might
need some support from XTF.

A couple of notes:
* it adds a setContextNode() to nsIXFormsContextControl, so repeat can set
context for its children
* has a GetIntAttr() that probably should go into nsXFormsUtils?

Comments anyone?

Comment 2

[aaronr]

There needs to be a way to query the current index from the repeat element. 
Need this for the XForms XPath extension function index().

When you are cloning the repeat template into repeatitems, any particular reason
that you are calling GetNodeName()?  Doesn't look like you do anything with the
result.  Handy for debug, though :=)

OnDestroyed - you are setting mElement and mHTMLElement to nsnull.  Is there a
reason that you aren't setting mContextNode to nsnull?

GetIntAttr - The logic to do such validation lives in the schema code that Doron
is doing.  So after Doron's code is in, GetIntAttr should just get the attr
string and pass it off along with the schema type to his schema code for
validation.  No sense duplicating the work.  Please get with Doron and make sure
his code that does this will be public and put in a todo comment for GetIntAttr
so that this code will use Doron's code once it is available.  Thanks.

Comment 3

[Allan Beaufour]

(In reply to comment #2)
> There needs to be a way to query the current index from the repeat element. 
> Need this for the XForms XPath extension function index().

I do not think you need to query these directly. You will get them as arguments
to Evaluate(). That was the way I was planning to support that. I didn't
acutallu pass that along, but I will include that in the new patch for bug 265216.

> When you are cloning the repeat template into repeatitems, any particular
> reason that you are calling GetNodeName()?  Doesn't look like you do
> anything with the result.  Handy for debug, though :=)

GetNodeName()? Suddenly I cannot find that anywhere? *ahem* :)

> OnDestroyed - you are setting mElement and mHTMLElement to nsnull.  Is there a
> reason that you aren't setting mContextNode to nsnull?

Nope, I'll do that to.

> GetIntAttr - The logic to do such validation lives in the schema code that Doron
> is doing.  So after Doron's code is in, GetIntAttr should just get the attr
> string and pass it off along with the schema type to his schema code for
> validation.  No sense duplicating the work.  Please get with Doron and make sure
> his code that does this will be public and put in a todo comment for GetIntAttr
> so that this code will use Doron's code once it is available.  Thanks.

Ah, naturally, yes. I'll put in a todo comment, but is there a bug I can refer to?

Comment 4

[Allan Beaufour]

Attachment: Fixed comments, etc.

Fixed Aaron's comments and also comments on bug 265216, that applied here too.

Comment 5

[aaronr]

I think that Doron's making the capability available via his patch to bug 223097

Comment 6

[Allan Beaufour]

Attachment: Updated to current CVS

Comment 7

[Allan Beaufour]

Attachment: Updated to current CVS

Comment 8

[Allan Beaufour]

Something is rotten in the state of Denmark .... found a few bugs, and thus some
more work for next week...

Comment 9

[Allan Beaufour]

Attachment: Fixed the problem

input and output needed to refresh on DocumentChanged() too. If not, they were
never Refresh()'ed when they were finally inserted into the correct document.
But now it should work.

Comment 10

[aaronr]

Comment on attachment 167330 [details] [diff] [review]
Fixed the problem

A couple of minor things.

In nsXFormsOutputElement::ParentChanged, please use same comment that you made
in nsXFormsInputElement::ParentChanged about the context possibly changing.

Something to think about: since repeatitemelement has its own factory, maybe
you should do a little extra checking in case one gets created outside of where
they get created in nsXFormsRepeatElement.  Right now you are assuming that
repeat item elements have contextsize and contextposition attrs.

neither of these should hold up checkin, but would be nice to have.

r=me

Comment 11

[Darin Fisher]

I offered to check in this patch (to free up bryner).  Please post a revised
patch, and I'll see that it gets checked in promptly.

Comment 12

[Allan Beaufour]

Attachment: Fixed Aaron's comments

* Added comment in nsXFormsOutputElement
* Set context size and position to 1, if the attributes are not present (or
cannot be converted to an integer)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

How do you handle the case where someone is modifying the subtree of the 
repeat element dynamically? Shouldn't you listen DOMSubtreeModified and
on event call Refresh() or something like that.

Comment 14

[aaronr]

(In reply to comment #13)
> How do you handle the case where someone is modifying the subtree of the 
> repeat element dynamically? Shouldn't you listen DOMSubtreeModified and
> on event call Refresh() or something like that.

I'd say treat that as a seperate bug now.  Let's get the code in to make
subsequent reviews smaller and spend less time maintaining the patch.

Comment 15

[Brian Ryner (not reading)]

checked in