cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Portier <...@outerthought.org>
Subject Re: [cforms] refactoring questions (was Re: cvs commit: cocoon-2.1/src/blocks/forms/java/org/apache/cocoon/forms/formmodel Struct.java Messages.java Repeater.java MultiValueField.java AbstractContainerWidget.java Output.java Upload.java Action.java Form.java ContainerDelegate.java AbstractWidget.java Field.java Union.java BooleanField.java Widget.java)
Date Wed, 21 Apr 2004 06:31:52 GMT


Tim Larson wrote:

> On Wed, Apr 21, 2004 at 12:29:26AM +0200, Marc Portier wrote:
> 
>>mpo@apache.org wrote:
>>1/ should getWidget(id) be removed from Widget? It is already on 
>>ContainerWidget (which is the true context that makes sense IMHO)
> 
> 
> +1 remove it.  It is historical from before we had the Container* code.
> 

ok

> 
>>2/ should getNamespace() exist at all, it seems to return the same thing 
>>as getFullQualifiedId()? Maybe a historical idea waiting to get thrown out?
> 
> 
> +1 historical, just make sure getFullyQualifiedId() does not break.
> 

yep

> 
>>3/ can getId() ever return null or "" on a widget instance? Can't we 
>>carefully asume programming error and allow for the accidental NPE to be 
>>thrown
> 
> 
> +1 after we fix the top-level "form" widget to not return null or "".
> (I don't remember which, but it returns one of those two values).
> Anyway, we somehow need to supply the "form" widget with an id to resolve
> widget naming conflicts when there is more than one form on a page.
> I think it should come from somewhere other than the form definition,
> because it is the page designer's role to prevent form id conflicts, not
> the role of the form designer.  For example, imagine a page containing
> two instances of the same form definition, tied to separate data.  The
> page designer knows there is a conflict, but the form designer has no
> way to prevent it.  Maybe there is a way to auto-generate the id's?
> 

thx for explaining.

I think we should go for returning null today then, and see how it turns 
out in future

> 
>>4/ same question on getDefinition()
> 
> 
> There once was talk about the possibility of widgets without
> definitions: http://marc.theaimsgroup.com/?t=107165245100001&r=1&w=2
> Does this have any relevance to your question?
> 

yep related, and the same solution should do now too:

starting from AbstractWidget down the hierarchy it should be there, and 
provided by the getDefinition()

people overloading AbstractWidget.getDefinition() to return null (or 
setting it during construction with super(null)

should be made aware to also overload getLocation() and 
generateDisplayData() (which was the reason for me asking now)

(although they should probably not use that base class then, since it 
explicitely asks for a definition in its constructor, maybe throwing 
IllegalArgument makes sense on the constructor)

> 
>>5/ should we rename ContainerDelegate to simply WidgetList (and the 
>>ContainerDefintionDelegate to WidgetDefinitionList)
> 
> 
> +1 to something like that.
> 

thx

> 
>>6/ union seems to generate fi:field in stead of fi:union this surprised 
>>me a bit, is that the goal?
> 
> 
> Yes, I was trying to surprise you ;)  No, really I just noticed that
> this part of the "union" widget acted just like a field as far as the
> template layer was concerned, so I used the same name to prevent having
> to copy-and-paste support code in the template handling.  A better route
> might be to change both field and union to use something more generic
> like fi:value in the templating layer.
> 

thx for explaining, adding a comment would take away the surprise

> 
>>7/ should validation stop as soon as possible or continue to allow all 
>>validation errors to be set?
> 
> 
> Don't know which is best.  *If* we cannot decide (due to different needs
> in different usecases) then we will need to add an attribute to let the
> form designer control the behaviour.
> 

hm, we should at least be consistent

> 
>>8/ setParent() on abstractWidget should be write-once IMHO, possibly 
>>yielding RTE (IllegalState?) when someone tries to reset it
> 
> 
> See: http://wiki.cocoondev.org/Wiki.jsp?page=ImmutableWidgetDefinitions
> The widget-definition repositories will allow widget-definitions to have
> multiple parents, so imho we should remove setParent() completely.
> Please see the "Walker solution" on that wiki page for more details.
> 

have to come back on that later (need to run now)

> 
>>9/ should not all generateSAXFragments include the 
>>getDefinition.generateDisplayData() by default
> 
> 
> +1 if my memory of the issues serves me right.
> 

coolio

-marc=
-- 
Marc Portier                            http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
Read my weblog at                http://blogs.cocoondev.org/mpo/
mpo@outerthought.org                              mpo@apache.org

Mime
View raw message