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 11:53:57 GMT


Marc Portier wrote:

> 
> 
> Tim Larson wrote:
> 
>> On Wed, Apr 21, 2004 at 12:29:26AM +0200, Marc Portier wrote:

<snip />

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

working on this, getting there, and makes me wonder about parent
relations again... (see further)

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

2nd thought: we should assign specific semantic meaning to both null and ""

- return null: not allowed for widget, 'this is abstract' for
widget-definition (see also
http://wiki.cocoondev.org/Wiki.jsp?page=WoodyRefactoring)

- return "": this widget is top-level

even if we get into forms with named id's, then there will be still use
for 'anonymous' top-level I suppose

so I'm going for getId on form to return "" (which it was)
and removing getId() == null checks since I favour the NPE here

plus updating javadoc @return to clarify these semantics.

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

apart from this last statement (which is so untrue it should be ignored)
  I'm going for this

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

clarifying a bit more:

we seem to have two distinct cases:
widgets with child-widgets
- i've seen code that doesn't evaluate the parent if one of the kids is
invalid
- but all kids seem to be evaluated even if a previous brother failed

widgets with validation on definition and widget level
- widget level rules stop evaulation if the definition level was not ok,
and as soon as one of the widget-level validations fails

I haven't given it much thought yet, if there are clear positions out
there I'm quite happy to just enforce code wise (and add some javadocs
removing any possible future surprise)

so comments welcome

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

(ah, the pleasure of being at a conference with wiki and overdimensioned
breaks :-) I get to do some work!)

I have to say that I'm struggling allready with the get/setParent in our
code, I fail to see why we need it at all.

Surely at definition time it seems total overkill ATM, I have the
feeling that widget-definitions don't need the way-up link (actually
smells a bit like subversion of control, no?)

On the runtime widget instances I think navigating the widget tree makes
more sense.

 From this angle, I'm on the 'walker' track: but I think the instance
tree already does this naturally, no?

Removing the 'parent' on the widget-definitions also removes the stress
of the 'multiple parents' introduced by the repositories.
(In fact I find it proof of the fact that "widgets don't have a parent"
more then anything else)

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

see 4/: am getting there

-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