You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cocoon.apache.org by Timothy Larson <ti...@yahoo.com> on 2003/11/21 14:37:15 UTC
[Woody patch] Class, New, Struct, and Union widgets
A diff adding Class, New, Struct, and Union widgets to Woody is at:
http://wiki.cocoondev.org/Wiki.jsp?page=TimLarson
The diff is named "woody_union.diff". I put it on the Wiki with some
documentation so we could discuss it and improve it.
What do you all think?
--Tim Larson
__________________________________
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Bruno Dumon <br...@outerthought.org>.
On Sat, 2003-11-22 at 21:39, Timothy Larson wrote:
> --- Bruno Dumon <br...@outerthought.org> wrote:
> > On Fri, 2003-11-21 at 14:37, Timothy Larson wrote:
> > I found the naming of the class/new widgets confusing. In fact, I think
> > it is confusing to call them widgets at all, even if they might be
> > implemented as widgets. Maybe calling them "define" and "use" is better,
> > and I wouldn't document them as being widgets.
>
> I am fine with any changes that make it better/clearer.
> You are right, Class and New are not widgets (they only have definitions
> and builders after all). The names "Class" and "New" came from the idea
> that at some point we might need to parameterize the instantiation of the
> class of widgets. For example, we may want to give nested unions a
> different default case than their parents. This could be specified as
> parameters to the "New" element. BTW, the current names do not quite sit
> well with me either.
>
> > I'm not a native english speaker, but I wonder if the struct/union names
> > will say much to non-programmers, or even Java programmers. OTOH, I
> > don't have any good alternatives yet.
>
> The name came from their counterparts in the "C" language.
> More intuitive names would be greatly appreciated.
>
> > Anyway, these are just small polishing remarks, next step is to dive
> > into the code. Could you also provide the complete patch, i.e. which
> > includes changes to all the widgets, so that I can see better how the
> > class/interface hierarchy fits together?
>
> When I modified all of the widgets the abstraction classes were not as
> well developed as they are in the current patch. If the abstraction
> is found beneficial, I will put the work into updating the old
> (unsubmitted) patch. Here is the general idea however:
>
> (Roughly, if I remember right...)
> Form: ContainerWidget, AbstractContainerWidget
> FormDefinition: ContainerDefinition, AbstractContainerDefinition
> Field: ValueWidget, AbstractValueWidget
> FieldDefinition: ValueDefinition, AbstractValueDefinition
> BooleanField: ValueWidget, custom code instead of AbstractValueWidget
> BooleanField: ValueDefinitions, AbstractValueDefinitions
> AggregateField: CompositeWidget, AbstractCompositeWidget
> AggregateFieldDefinition: CompositeDefinition, AbstractCompositeDefinitions
> Struct: ContainerWidget, AbstractContainerWidget
> StructDefinitions: ContainerDefinitions, AbstractContainerDefinition
> Union: CompositeWidget, AbstractCompositeWidget
> UnionDefinitions: CompositeDefinition, AbstractCompositeDefinition
> Repeater: ContainerWidget, custom code instead of AbstractContainerWidget
> RepeaterRow: ContainerWidget, AbstractContainerWidget
> RepeaterDefinition: ContainerDefinitions, AbstractContainerDefinition
> ...etc.
I think it is useful, after all, the alternative is code duplication.
> >From your other email:
> > Need to look into the code yet, but I saw the comment in the TODO list
> > about slow operation on deeper nesting levels, and was curious about
> > what this is caused by? Something on the processRequest or template
> > side?
>
> I did not get a chance to track this down yet. I just noticed it and
> documented it to look into later. If you (or anyone) has a good
> debugging/profiling environment you will probably beat me to it :)
Don't have any either (damn that optimizeit thing is expensive), but
suddenly the thought crossed my mind that this might be caused by the
calls to getFullyQualifiedId(), and adding some little caching code
there solved it indeed.
Another thing I noticed is that in Union.readFromRequest, you first let
the active case do the readFromRequest, and then call
super.readFromRequest, which will let all cases do the readFromRequest.
I don't think this is needed, and it solves the multiple row creation
problem in the repeaters. (same applies to validate() I gues)
Anyway, I'm starting to think that the easiest way to collaborate on
this is if we just commit the code into CVS. Anyone got a problem with
that?
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Bruno Dumon <br...@outerthought.org>.
On Mon, 2003-11-24 at 06:27, Timothy Larson wrote:
> --- Bruno Dumon <br...@outerthought.org> wrote:
> ...slow operation on deeper nesting levels...
> > Don't have any either (damn that optimizeit thing is expensive), but
> > suddenly the thought crossed my mind that this might be caused by the
> > calls to getFullyQualifiedId(), and adding some little caching code
> > there solved it indeed.
>
> Thanks. I fixed one or two other methods like that, but missed that one.
Be careful though, since it's not possible to blindly cache those; as
for a repeater these can change if rows are removed. Maybe resetting the
cached value at the start of readFromRequest would be enough to solve
this.
>
> > Another thing I noticed is that in Union.readFromRequest, you first let
> > the active case do the readFromRequest, and then call
> > super.readFromRequest, which will let all cases do the readFromRequest.
> > I don't think this is needed, and it solves the multiple row creation
> > problem in the repeaters. (same applies to validate() I gues)
>
> Good, that should also solve the problem of nested repeaters losing their
> state when the enclosing union switches cases.
>
> > Anyway, I'm starting to think that the easiest way to collaborate on
> > this is if we just commit the code into CVS. Anyone got a problem with
> > that?
>
> Sounds good, as long as nobody hesitates to change aspects of it as needed.
>
> For example, I have been wondering about changing the Union widget from a
> composite widget to a container widget. The union could have an attribute
> which specifies which widget would act as the discriminant, or it could
> evaluate a general expression, which could reference other widgets, to
> produce the discriminant value. See my next email about subforms for the
> reason for this possible change.
still marked as unread :-)
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Timothy Larson <ti...@yahoo.com>.
--- Bruno Dumon <br...@outerthought.org> wrote:
...slow operation on deeper nesting levels...
> Don't have any either (damn that optimizeit thing is expensive), but
> suddenly the thought crossed my mind that this might be caused by the
> calls to getFullyQualifiedId(), and adding some little caching code
> there solved it indeed.
Thanks. I fixed one or two other methods like that, but missed that one.
> Another thing I noticed is that in Union.readFromRequest, you first let
> the active case do the readFromRequest, and then call
> super.readFromRequest, which will let all cases do the readFromRequest.
> I don't think this is needed, and it solves the multiple row creation
> problem in the repeaters. (same applies to validate() I gues)
Good, that should also solve the problem of nested repeaters losing their
state when the enclosing union switches cases.
> Anyway, I'm starting to think that the easiest way to collaborate on
> this is if we just commit the code into CVS. Anyone got a problem with
> that?
Sounds good, as long as nobody hesitates to change aspects of it as needed.
For example, I have been wondering about changing the Union widget from a
composite widget to a container widget. The union could have an attribute
which specifies which widget would act as the discriminant, or it could
evaluate a general expression, which could reference other widgets, to
produce the discriminant value. See my next email about subforms for the
reason for this possible change.
--Tim Larson
__________________________________
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Bruno Dumon <br...@outerthought.org>.
On Sat, 2003-11-22 at 21:39, Timothy Larson wrote:
> --- Bruno Dumon <br...@outerthought.org> wrote:
> > On Fri, 2003-11-21 at 14:37, Timothy Larson wrote:
> > I found the naming of the class/new widgets confusing. In fact, I think
> > it is confusing to call them widgets at all, even if they might be
> > implemented as widgets. Maybe calling them "define" and "use" is better,
> > and I wouldn't document them as being widgets.
>
> I am fine with any changes that make it better/clearer.
> You are right, Class and New are not widgets (they only have definitions
> and builders after all). The names "Class" and "New" came from the idea
> that at some point we might need to parameterize the instantiation of the
> class of widgets. For example, we may want to give nested unions a
> different default case than their parents. This could be specified as
> parameters to the "New" element. BTW, the current names do not quite sit
> well with me either.
>
> > I'm not a native english speaker, but I wonder if the struct/union names
> > will say much to non-programmers, or even Java programmers. OTOH, I
> > don't have any good alternatives yet.
>
> The name came from their counterparts in the "C" language.
> More intuitive names would be greatly appreciated.
>
> > Anyway, these are just small polishing remarks, next step is to dive
> > into the code. Could you also provide the complete patch, i.e. which
> > includes changes to all the widgets, so that I can see better how the
> > class/interface hierarchy fits together?
>
> When I modified all of the widgets the abstraction classes were not as
> well developed as they are in the current patch. If the abstraction
> is found beneficial, I will put the work into updating the old
> (unsubmitted) patch. Here is the general idea however:
>
> (Roughly, if I remember right...)
> Form: ContainerWidget, AbstractContainerWidget
> FormDefinition: ContainerDefinition, AbstractContainerDefinition
> Field: ValueWidget, AbstractValueWidget
> FieldDefinition: ValueDefinition, AbstractValueDefinition
> BooleanField: ValueWidget, custom code instead of AbstractValueWidget
> BooleanField: ValueDefinitions, AbstractValueDefinitions
> AggregateField: CompositeWidget, AbstractCompositeWidget
> AggregateFieldDefinition: CompositeDefinition, AbstractCompositeDefinitions
> Struct: ContainerWidget, AbstractContainerWidget
> StructDefinitions: ContainerDefinitions, AbstractContainerDefinition
> Union: CompositeWidget, AbstractCompositeWidget
> UnionDefinitions: CompositeDefinition, AbstractCompositeDefinition
> Repeater: ContainerWidget, custom code instead of AbstractContainerWidget
> RepeaterRow: ContainerWidget, AbstractContainerWidget
> RepeaterDefinition: ContainerDefinitions, AbstractContainerDefinition
> ...etc.
I think it is useful, after all, the alternative is code duplication.
> >From your other email:
> > Need to look into the code yet, but I saw the comment in the TODO list
> > about slow operation on deeper nesting levels, and was curious about
> > what this is caused by? Something on the processRequest or template
> > side?
>
> I did not get a chance to track this down yet. I just noticed it and
> documented it to look into later. If you (or anyone) has a good
> debugging/profiling environment you will probably beat me to it :)
Don't have any either (damn that optimizeit thing is expensive), but
suddenly the thought crossed my mind that this might be caused by the
calls to getFullyQualifiedId(), and adding some little caching code
there solved it indeed.
Another thing I noticed is that in Union.readFromRequest, you first let
the active case do the readFromRequest, and then call
super.readFromRequest, which will let all cases do the readFromRequest.
I don't think this is needed, and it solves the multiple row creation
problem in the repeaters. (same applies to validate() I gues)
Anyway, I'm starting to think that the easiest way to collaborate on
this is if we just commit the code into CVS. Anyone got a problem with
that?
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Timothy Larson <ti...@yahoo.com>.
--- Bruno Dumon <br...@outerthought.org> wrote:
> On Fri, 2003-11-21 at 14:37, Timothy Larson wrote:
> I found the naming of the class/new widgets confusing. In fact, I think
> it is confusing to call them widgets at all, even if they might be
> implemented as widgets. Maybe calling them "define" and "use" is better,
> and I wouldn't document them as being widgets.
I am fine with any changes that make it better/clearer.
You are right, Class and New are not widgets (they only have definitions
and builders after all). The names "Class" and "New" came from the idea
that at some point we might need to parameterize the instantiation of the
class of widgets. For example, we may want to give nested unions a
different default case than their parents. This could be specified as
parameters to the "New" element. BTW, the current names do not quite sit
well with me either.
> I'm not a native english speaker, but I wonder if the struct/union names
> will say much to non-programmers, or even Java programmers. OTOH, I
> don't have any good alternatives yet.
The name came from their counterparts in the "C" language.
More intuitive names would be greatly appreciated.
> Anyway, these are just small polishing remarks, next step is to dive
> into the code. Could you also provide the complete patch, i.e. which
> includes changes to all the widgets, so that I can see better how the
> class/interface hierarchy fits together?
When I modified all of the widgets the abstraction classes were not as
well developed as they are in the current patch. If the abstraction
is found beneficial, I will put the work into updating the old
(unsubmitted) patch. Here is the general idea however:
(Roughly, if I remember right...)
Form: ContainerWidget, AbstractContainerWidget
FormDefinition: ContainerDefinition, AbstractContainerDefinition
Field: ValueWidget, AbstractValueWidget
FieldDefinition: ValueDefinition, AbstractValueDefinition
BooleanField: ValueWidget, custom code instead of AbstractValueWidget
BooleanField: ValueDefinitions, AbstractValueDefinitions
AggregateField: CompositeWidget, AbstractCompositeWidget
AggregateFieldDefinition: CompositeDefinition, AbstractCompositeDefinitions
Struct: ContainerWidget, AbstractContainerWidget
StructDefinitions: ContainerDefinitions, AbstractContainerDefinition
Union: CompositeWidget, AbstractCompositeWidget
UnionDefinitions: CompositeDefinition, AbstractCompositeDefinition
Repeater: ContainerWidget, custom code instead of AbstractContainerWidget
RepeaterRow: ContainerWidget, AbstractContainerWidget
RepeaterDefinition: ContainerDefinitions, AbstractContainerDefinition
...etc.
>From your other email:
> Need to look into the code yet, but I saw the comment in the TODO list
> about slow operation on deeper nesting levels, and was curious about
> what this is caused by? Something on the processRequest or template
> side?
I did not get a chance to track this down yet. I just noticed it and
documented it to look into later. If you (or anyone) has a good
debugging/profiling environment you will probably beat me to it :)
Thanks for the interest,
--Tim Larson
__________________________________
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Bruno Dumon <br...@outerthought.org>.
On Fri, 2003-11-21 at 14:37, Timothy Larson wrote:
> A diff adding Class, New, Struct, and Union widgets to Woody is at:
> http://wiki.cocoondev.org/Wiki.jsp?page=TimLarson
> The diff is named "woody_union.diff". I put it on the Wiki with some
> documentation so we could discuss it and improve it.
>
> What do you all think?
I found the naming of the class/new widgets confusing. In fact, I think
it is confusing to call them widgets at all, even if they might be
implemented as widgets. Maybe calling them "define" and "use" is better,
and I wouldn't document them as being widgets.
I'm not a native english speaker, but I wonder if the struct/union names
will say much to non-programmers, or even Java programmers. OTOH, I
don't have any good alternatives yet.
Anyway, these are just small polishing remarks, next step is to dive
into the code. Could you also provide the complete patch, i.e. which
includes changes to all the widgets, so that I can see better how the
class/interface hierarchy fits together?
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Bruno Dumon <br...@outerthought.org>.
On Fri, 2003-11-21 at 19:05, Sylvain Wallez wrote:
> Timothy Larson wrote:
>
> >A diff adding Class, New, Struct, and Union widgets to Woody is at:
> > http://wiki.cocoondev.org/Wiki.jsp?page=TimLarson
> >The diff is named "woody_union.diff". I put it on the Wiki with some
> >documentation so we could discuss it and improve it.
> >
> >What do you all think?
> >
> >
>
> Looks definitely interesting, but seem to have required a lot of
> refactoring. Can you please explain why?
>
> I'll take a deeper look at this ASAP.
ditto
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Timothy Larson <ti...@yahoo.com>.
--- Sylvain Wallez <sy...@apache.org> wrote:
> Timothy Larson wrote:
> >A diff adding Class, New, Struct, and Union widgets to Woody is at:
> > http://wiki.cocoondev.org/Wiki.jsp?page=TimLarson
...
> Looks definitely interesting, but seem to have required a lot of
> refactoring. Can you please explain why?
Most of the refactoring is not really "required".
It was part of a (possibly misguided) attempt to abstract most of the
widget code into common classes. If you look through the widget code
(widgets, definitions, and builders) you will see a lot of duplicate or
nearly duplicate code. I initially modified all of that code, shrinking
the code for each widget to only what made each type of widget different,
with the rest of the code abstracted.
This made for too drastic a patch to submit all at once, so I scaled it
back to where the abstract code was still present (so we could discuss it),
but only the most essential changes were made to the existing widgets to
support the four new widget types.
I do not want or expect this patch to be commited as is, but rather be
torn apart, discussed, modified, and have the most useful parts commited.
--Tim Larson
> I'll take a deeper look at this ASAP.
Thanks for looking at it.
--Tim Larson
__________________________________
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Sylvain Wallez <sy...@apache.org>.
Timothy Larson wrote:
>A diff adding Class, New, Struct, and Union widgets to Woody is at:
> http://wiki.cocoondev.org/Wiki.jsp?page=TimLarson
>The diff is named "woody_union.diff". I put it on the Wiki with some
>documentation so we could discuss it and improve it.
>
>What do you all think?
>
>
Looks definitely interesting, but seem to have required a lot of
refactoring. Can you please explain why?
I'll take a deeper look at this ASAP.
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
Orixo, the opensource XML business alliance - http://www.orixo.com
Re: [Woody patch] Class, New, Struct, and Union widgets
Posted by Bruno Dumon <br...@outerthought.org>.
On Fri, 2003-11-21 at 14:37, Timothy Larson wrote:
> A diff adding Class, New, Struct, and Union widgets to Woody is at:
> http://wiki.cocoondev.org/Wiki.jsp?page=TimLarson
> The diff is named "woody_union.diff". I put it on the Wiki with some
> documentation so we could discuss it and improve it.
>
> What do you all think?
I had a first quick look: applying the patch and rebuilding cocoon went
without problems, which is great. The sample itself is very cool, ice
cold stuff.
Need to look into the code yet, but I saw the comment in the TODO list
about slow operation on deeper nesting levels, and was curious about
what this is caused by? Something on the processRequest or template
side?
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org