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 Sun, 25 Apr 2004 20:34:32 GMT


Sylvain Wallez wrote:

> 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"!
> 

yep, thx for chiming in

>> >   - 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")

> 

aargh, did this already

> 
> 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")
> 

makes sense, but I haven't seen that much so deep nested structures yet, 
but surely one we could add to the virtual todo list :-)

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

done

>> 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).
> 

I had the same feeling in fact, but didn't go so far yet.
would need to check consequences

>> 5/ should we rename ContainerDelegate to simply WidgetList (and the 
>> ContainerDefintionDelegate to WidgetDefinitionList)
> 
> 
> 
> WidgetList is more understandable than ContainerDelegate ;-)
> 

was done

>> 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)
> 

thx for the extra info, tim already explained the first part

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

yeah, that seems to be the consensus, I've dropped the 
event/validation/lifecycle stuff for the moment and am focussing on the 
@extend/@define reuse first

I'll pick this up later by making a catalogue of the validation stuff 
now (I thought I saw some inconsistency to the above rule, but the 
special cases might make sense after all, just need some detail look, 
discussion and additional dev-docs IMO)

>> 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...
> 

haven't touched this yet

removing it on the widget-definition is part of my current work on doing 
the @extend @defines stuff

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

done that

> 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>.
> 

hm, actually since the start/end is always grouped and quite similar to 
all widgets I've made the spilt slightly different:

generateSAXFragment will do the start/end of the containing element, by 
asking getXMLElementName() and getXMLElementAttributes from the derived 
concrete subclass

inserting other stuff in between is done by subclassing 
generateItemSAXFragment

hope you can live with that to get the same flexibility?
(the only flexibility you loose imho is the ability to produce not 
welformed XML by mismatching your end and start events :-))

> 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!
> 

maybe we could just treat it like the display-data?
(we made that move to the wi namespace as well, so it doesn't seem to 
unlogic, no?)

wdyt?
-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