cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Wallez <sylv...@apache.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 Sun, 25 Apr 2004 13:10:41 GMT
Marc Portier wrote:

<snip/>

> Sorry for the massive commit, however when walking around the code it 
> only looked like the proverbial tip of the iceberg.


Sorry for the delay, but, as we say here "later is better than never"!

> >   - left quite some TODO markers for next sweaps
>
> Maybe some of you have some suggestions on some of them, feel free to 
> step in and comment:
>
> 1/ should getWidget(id) be removed from Widget? It is already on 
> ContainerWidget (which is the true context that makes sense IMHO)


+1 from a theoretical POV, but -1 from a practical one! This will lead 
to many casts to traverse a widget tree, e.g.
    form.getWidget("choice").getWidget("union").getWidget("foo")
will become
    
((UnionWidget)((ChoiceWidget)form.getWidget("choice")).getWidget("union")).getWidget("foo")

Or we may extend getWidget() so that it accepts a path (dotted notation) 
instead of a simple name, which would allow e.g.
    form.getWidget("choice.union.foo")

> 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 to remove

> 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
>
> 4/ same question on getDefinition()


What's the need for getDefinition() for users of a widget? I consider 
this as an implementation concern of widgets and would remove it from 
the public API (i.e. make it protected in AbstractWidget).

> 5/ should we rename ContainerDelegate to simply WidgetList (and the 
> ContainerDefintionDelegate to WidgetDefinitionList)


WidgetList is more understandable than ContainerDelegate ;-)

> 6/ union seems to generate fi:field in stead of fi:union this 
> surprised me a bit, is that the goal?


+1 for fi:field as, for the view, it isn't different from a field and 
avoids duplicating the styling (the same is already done for action, 
submit, repeater-action and row-action)

> 7/ should validation stop as soon as possible or continue to allow all 
> validation errors to be set?


Continue to get all validation errors.

> 8/ setParent() on abstractWidget should be write-once IMHO, possibly 
> yielding RTE (IllegalState?) when someone tries to reset it


Don't know... A sure thing is that definitions don't need a parent (also 
they should be immutable). As for reparenting widgets, I don't know if 
there are some valid use cases...

> 9/ should not all generateSAXFragments include the 
> getDefinition.generateDisplayData() by default


+1

And I would add:

10/ Split generateSAXFragment() into startSAXFragment() and 
endSAXFragment(), which will make it so much easier on the view side to 
handle custom SAX fragments for container widgets and embedding of the 
<wi:styling>.

Note that I'd like also that <wi:styling> could be written in the 
definition also, as defining the styling in the widget definition can be 
a productivity boost with widget repositories!

Sylvain

-- 
Sylvain Wallez                                  Anyware Technologies
http://www.apache.org/~sylvain           http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }


Mime
View raw message