You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cocoon.apache.org by Marc Portier <mp...@outerthought.org> on 2004/04/21 00:29:26 UTC
[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)
mpo@apache.org wrote:
> mpo 2004/04/20 15:19:27
>
> Modified: 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
> Log:
> Another sweap in the cforms refactoring.
> - Added javadoc here and there
> - Realized that Repeater is not a container widget (it's rows are):
> we should split the aspect of containing multiple values (like multivalue) (these are 'repeating' or 'iterating'
> from the aspect of containing child-widgets (being 'composed' seems more appropriate)
> - Removed confusing overloaded versions of generateSAXFragment from the Abstract(Container)Widget
> --> this introduced getXMLElementName on all subclasses, and needs additional cleanup
> (that should remove quite some load of copy-paste)
Sorry for the massive commit, however when walking around the code it
only looked like the proverbial tip of the iceberg.
> - 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)
2/ should getNamespace() exist at all, it seems to return the same thing
as getFullQualifiedId()? Maybe a historical idea waiting to get thrown out?
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()
5/ should we rename ContainerDelegate to simply WidgetList (and the
ContainerDefintionDelegate to WidgetDefinitionList)
6/ union seems to generate fi:field in stead of fi:union this surprised
me a bit, is that the goal?
7/ should validation stop as soon as possible or continue to allow all
validation errors to be set?
8/ setParent() on abstractWidget should be write-once IMHO, possibly
yielding RTE (IllegalState?) when someone tries to reset it
9/ should not all generateSAXFragments include the
getDefinition.generateDisplayData() by default
-marc= (the happy cleaning lady, be warned: she'll be back :-))
--
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
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)
Posted by Joerg Heinicke <jo...@gmx.de>.
On 21.04.2004 13:53, Marc Portier wrote:
> so I'm going for getId on form to return "" (which it was)
> and removing getId() == null checks since I favour the NPE here
Please not waiting for the NPE somewhere latter in the code for the
reasons Ugo pointed out.
>>>> 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
IMO validation should always run on all widgets as it must be possible
for the user to get a whole form fixed in one step:
http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=107962387820282&w=4
http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=107963156530885&w=4
Joerg
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)
Posted by Marc Portier <mp...@outerthought.org>.
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
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)
Posted by Marc Portier <mp...@outerthought.org>.
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
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)
Posted by Tim Larson <ti...@keow.org>.
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.
> 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.
> 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?
> 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?
> 5/ should we rename ContainerDelegate to simply WidgetList (and the
> ContainerDefintionDelegate to WidgetDefinitionList)
+1 to something like that.
> 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.
> 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.
> 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.
> 9/ should not all generateSAXFragments include the
> getDefinition.generateDisplayData() by default
+1 if my memory of the issues serves me right.
> -marc= (the happy cleaning lady, be warned: she'll be back :-))
Yes, please come back. We need to get this house in shape. :)
--Tim Larson
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)
Posted by Marc Portier <mp...@outerthought.org>.
Sylvain Wallez wrote:
> Bruno Dumon wrote:
>
>> On Mon, 2004-04-26 at 10:43, Marc Portier wrote:
>>
>>
>>> Sylvain Wallez wrote:
>>>
>>>
>>>> Marc Portier wrote:
>>>>
>>>>
>>>>> Sylvain Wallez wrote:
>>>>>
>>
>>
>>
>>
>>>> - if we allow "fi:styling" in the definition (which is needed IMO),
>>>> we must still retain the possibility to override it in the template.
>>>> The associated logic on the template side will be much more easy to
>>>> implement.
>>>>
>>>
>>> didn't think of this yet,
>>> in any case we will need some overriding/merging rules for the
>>> @defines/@extends thing as well, I guess similar ones should apply
>>> for letting template override its definnition on certain fields
>>>
>>
>>
>> Just curious: are you planning on doing the "extending" by merging the
>> definitions on the XML level?
>>
>>
>
neuh, not at all, just sounds like that since the fi:* displaydata from
this thread would more use that pattern
I was just refering to the fact that from a user POV the same kind of
rules should apply...
> Sounds weird... I would better consider this by having the definition
> with @extend delegate some calls to the definition it extends.
>
yeah, but I'ld like the definitions to become immutable to make sure
we're not making errors here in this definition-reuse system (you don't
want to be accidentally changing the base-definition when you're just
cloning/extending it)
also it kinda feels more like the business of the builder then of the
definition itself to know about these merging rules... so as far as I
see now I would rather change the interface of the builders to also have
a 'WidgetDefinition base' argument
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
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)
Posted by Sylvain Wallez <sy...@apache.org>.
Bruno Dumon wrote:
>On Mon, 2004-04-26 at 10:43, Marc Portier wrote:
>
>
>>Sylvain Wallez wrote:
>>
>>
>>>Marc Portier wrote:
>>>
>>>
>>>>Sylvain Wallez wrote:
>>>>
>>>>
>
>
>
>>>- if we allow "fi:styling" in the definition (which is needed IMO), we
>>>must still retain the possibility to override it in the template. The
>>>associated logic on the template side will be much more easy to implement.
>>>
>>>
>>didn't think of this yet,
>>in any case we will need some overriding/merging rules for the
>>@defines/@extends thing as well, I guess similar ones should apply for
>>letting template override its definnition on certain fields
>>
>>
>
>Just curious: are you planning on doing the "extending" by merging the
>definitions on the XML level?
>
>
Sounds weird... I would better consider this by having the definition
with @extend delegate some calls to the definition it extends.
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
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)
Posted by Bruno Dumon <br...@outerthought.org>.
On Mon, 2004-04-26 at 10:43, Marc Portier wrote:
> Sylvain Wallez wrote:
> > Marc Portier wrote:
> >
> >> Sylvain Wallez wrote:
> >>
> > - if we allow "fi:styling" in the definition (which is needed IMO), we
> > must still retain the possibility to override it in the template. The
> > associated logic on the template side will be much more easy to implement.
> >
>
> didn't think of this yet,
> in any case we will need some overriding/merging rules for the
> @defines/@extends thing as well, I guess similar ones should apply for
> letting template override its definnition on certain fields
Just curious: are you planning on doing the "extending" by merging the
definitions on the XML level?
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
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)
Posted by Sylvain Wallez <sy...@apache.org>.
Marc Portier wrote:
>> The flexibility introduced by decoupling start/endSAXFragment is not
>> on the widget side, but on the template side: it allows to much more
>> easily handled nested template instructions. This has several uses:
>> - as of today, SAX events for container widgets must be completely
>> implemented on the template side. Decoupling start/end allows
>> container markup to be defined in the widget
>
>
> I don't get this, sample?
Have a look at EffectWidgetReplacingPipe.UnionHandler: production of
"wi:union" is written in the template generator. Having
start/endSAXFragment allows to move this to the widget itself.
Of course, this isn't totally equivalent to today's generateSAXFragment,
since in the case of container widgets it iterates recursively on its
children. But this behaviour is of use only with the FormGenerator (who
uses it?).
>> - if we allow "fi:styling" in the definition (which is needed IMO),
>> we must still retain the possibility to override it in the template.
>> The associated logic on the template side will be much more easy to
>> implement.
>
>
> didn't think of this yet,
> in any case we will need some overriding/merging rules for the
> @defines/@extends thing as well, I guess similar ones should apply for
> letting template override its definition on certain fields
That's a different problem, as it has to be handled at the definition
level, by defining the delegation/overriding policy of @extends widgets.
<snip/>
>> Do you mean that it has been decided to move label/help/hint to the
>> "fi" namespace within the definition? Missed that...
>>
>
> ah, you are right, it wasn't done
> hm, I'm quite sure we decided on this, no?
>
> this is the only thing I can find on this:
> http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=106942146927334&w=2
> I'ld have to agree it doesn't sound like a formal decission though :-(
Not a formal decision, but looks like a general consensus ;-)
Anyway, a formal +1 from me!
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
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)
Posted by Marc Portier <mp...@outerthought.org>.
Sylvain Wallez wrote:
> Marc Portier wrote:
>
>> Sylvain Wallez wrote:
>>
>>> Marc Portier wrote:
>>>
>>> <snip/>
>>>
<snip />
>>>> 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
>
>
>
> Do you mean that you already wrote similar complex code? Were you
> comfortable with this notation? I guess not ;-)
>
indeed not, that was my remark on deep nested structures.
what id 'done' was remove getWidget from the widget and keep it only on
container-widget
>>> 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 :-)
>
> Actually, after some further thoughts, getWidget(path) seems the most
> convenient to me, and doesn't break the architectural beauty of having
> getWidget() only on container widgets.
>
+1
> <snip/>
>
>>> 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 :-))
>
>
>
> The flexibility introduced by decoupling start/endSAXFragment is not on
> the widget side, but on the template side: it allows to much more easily
> handled nested template instructions. This has several uses:
> - as of today, SAX events for container widgets must be completely
> implemented on the template side. Decoupling start/end allows container
> markup to be defined in the widget
I don't get this, sample?
> - if we allow "fi:styling" in the definition (which is needed IMO), we
> must still retain the possibility to override it in the template. The
> associated logic on the template side will be much more easy to implement.
>
didn't think of this yet,
in any case we will need some overriding/merging rules for the
@defines/@extends thing as well, I guess similar ones should apply for
letting template override its definnition on certain fields
> An important point is that startSAXFragment should output all the
> widget's markup except the endElement for the toplevel element, so that
> some filtering can be implemented on the template side (e.g. overriding
> fi:styling as explained above).
>
if I get it correctly then you favour a start/end split cause it would
allow you to capture which styling on the template level has passed so
you can surpress it from the definition? And the end-handling would just
complete with what was 'inherited' from the definition?
makes sense of course
>>> 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?
>
>
>
> +1. wi:styling *is* some display data!
>
yep.
>> (we made that move to the wi namespace as well, so it doesn't seem to
>> unlogic, no?)
>
>
>
> Do you mean that it has been decided to move label/help/hint to the "fi"
> namespace within the definition? Missed that...
>
ah, you are right, it wasn't done
hm, I'm quite sure we decided on this, no?
this is the only thing I can find on this:
http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=106942146927334&w=2
I'ld have to agree it doesn't sound like a formal decission though :-(
do you think we need one?
I'm working on the building process as we speak (to implement the
reuse/repo) this is easy to take up with that work
-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
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)
Posted by Sylvain Wallez <sy...@apache.org>.
Marc Portier wrote:
>
>
> 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
Do you mean that you already wrote similar complex code? Were you
comfortable with this notation? I guess not ;-)
>> 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 :-)
Actually, after some further thoughts, getWidget(path) seems the most
convenient to me, and doesn't break the architectural beauty of having
getWidget() only on container widgets.
<snip/>
>> 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 :-))
The flexibility introduced by decoupling start/endSAXFragment is not on
the widget side, but on the template side: it allows to much more easily
handled nested template instructions. This has several uses:
- as of today, SAX events for container widgets must be completely
implemented on the template side. Decoupling start/end allows container
markup to be defined in the widget
- if we allow "fi:styling" in the definition (which is needed IMO), we
must still retain the possibility to override it in the template. The
associated logic on the template side will be much more easy to implement.
An important point is that startSAXFragment should output all the
widget's markup except the endElement for the toplevel element, so that
some filtering can be implemented on the template side (e.g. overriding
fi:styling as explained above).
>> 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?
+1. wi:styling *is* some display data!
> (we made that move to the wi namespace as well, so it doesn't seem to
> unlogic, no?)
Do you mean that it has been decided to move label/help/hint to the "fi"
namespace within the definition? Missed that...
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
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)
Posted by Marc Portier <mp...@outerthought.org>.
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
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)
Posted by Bruno Dumon <br...@outerthought.org>.
On Mon, 2004-04-26 at 09:51, Sylvain Wallez wrote:
> Bruno Dumon wrote:
>
> >On Sun, 2004-04-25 at 15:10, Sylvain Wallez wrote:
> >>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!
> >>
> >>
> >
> >Should be trivial to store this in the form definition.
> >
> >
>
> Yep. But this brings some namespace-related questions: "styling" is
> obviously in the instance namespace ("fi"), but if we introduce some
> "fi:" in the definition, what about "label"? The CForms machinery does
> nothing with it except copying it in the template output, so we may
> consider moving it also to the "fi" namespace.
+1
label has been put in the definition only because it was thought to be
convenient.
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
[OT] french names (was Re: [cforms] refactoring questions...)
Posted by Sylvain Wallez <sy...@apache.org>.
Joerg Heinicke wrote:
> On 26.04.2004 09:51, Sylvain Wallez wrote:
>
>> :-) This translation is the right one. I'm always amazed to see that
>> French is a foreign language for you and Marc, whose names sound so
>> much more frenchy that many french people's name, including mine ;-)
>
>
> Isn't Sylvain *the* French name? I only met two French men IRL, both
> were named Sylvain - if that's a criteria :) The one were you, the
> other one worked for Elf Aquitane near by here in Leuna.
Well, actually Sylvain _is_ french although not very frequent (strange
coincidence that made you meet two of them), but I was more referring to
my last name, Wallez.
In my family, this name is said to come from Belgium at a time when it
was dominated by Spain (16th century). Wallez comes from "Wallonie" (the
french speaking part of Belgium, and the spanish "-ez" suffix found e.g.
in Martinez, Rodriguez which I've been said to mean "son of". So I'm
actually a spanish belgian ;-)
"Bruno Dumon" and "Marc Portier" sound so much more typically french
both by their names and firstnames!
Who knows, maybe I'm actually one of their distant cousins ;-)
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
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)
Posted by Joerg Heinicke <jo...@gmx.de>.
On 26.04.2004 09:51, Sylvain Wallez wrote:
> :-) This translation is the right one. I'm always amazed to see that
> French is a foreign language for you and Marc, whose names sound so much
> more frenchy that many french people's name, including mine ;-)
Isn't Sylvain *the* French name? I only met two French men IRL, both
were named Sylvain - if that's a criteria :) The one were you, the other
one worked for Elf Aquitane near by here in Leuna.
> This makes me think that we could even use <ft:widget> for repeaters
> instead of <ft:repeater-widget>. The word "repeater" doesn't bring much
> value IMO.
big +1
I often had the case were I copied stuff from the list view (= repeater)
into the detail view (single elements) and had to rename the elements.
>>> 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!
>>
>> Should be trivial to store this in the form definition.
>
> Yep. But this brings some namespace-related questions: "styling" is
> obviously in the instance namespace ("fi"), but if we introduce some
> "fi:" in the definition, what about "label"? The CForms machinery does
> nothing with it except copying it in the template output, so we may
> consider moving it also to the "fi" namespace.
+1
Joerg
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)
Posted by Sylvain Wallez <sy...@apache.org>.
Bruno Dumon wrote:
>On Sun, 2004-04-25 at 15:10, 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"!
>>
>>
>
>mieux vaut tard que jamais! (had to cheat for this one, you don't want to see my first attempt ;-)
>
>
:-) This translation is the right one. I'm always amazed to see that
French is a foreign language for you and Marc, whose names sound so much
more frenchy that many french people's name, including mine ;-)
>>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>.
>>
>>
>
>+1
>
>What do you have in mind with the "custom SAX fragments for container widgets"?
>
>
Well "custom" has to be understood as "widget-specific", i.e. a union
will output something different than a repeater.
For now, markup production for container widgets has to be handled on
the view side since the single "generateSAXFragment" cannot take into
account the nested template included into the widget reference.
This makes me think that we could even use <ft:widget> for repeaters
instead of <ft:repeater-widget>. The word "repeater" doesn't bring much
value IMO.
>>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!
>>
>>
>
>Should be trivial to store this in the form definition.
>
>
Yep. But this brings some namespace-related questions: "styling" is
obviously in the instance namespace ("fi"), but if we introduce some
"fi:" in the definition, what about "label"? The CForms machinery does
nothing with it except copying it in the template output, so we may
consider moving it also to the "fi" namespace.
WDYT?
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
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)
Posted by Bruno Dumon <br...@outerthought.org>.
On Sun, 2004-04-25 at 15:10, 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"!
mieux vaut tard que jamais! (had to cheat for this one, you don't want
to see my first attempt ;-)
> 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>.
+1
What do you have in mind with the "custom SAX fragments for container
widgets"?
>
> 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!
Should be trivial to store this in the form definition.
--
Bruno Dumon http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
bruno@outerthought.org bruno@apache.org
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)
Posted by Sylvain Wallez <sy...@apache.org>.
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 }
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)
Posted by Marc Portier <mp...@outerthought.org>.
Ugo Cei wrote:
> Il giorno 21/apr/04, alle 20:14, Marc Portier ha scritto:
>>
>> Joerg Heinicke wrote:
>>>
>>>> Marc Portier wrote:
>>>>
>>> NPE is really bad - not only here - for the two mentioned reasons.
>>> But if you insist on your NPE you can change Ugo's proposal to
>>> if (id == null) {
>>> throw new NPE("id must not be null");
>>> }
>>> So we have NPE, which is RuntimeE, meaningful message and early
>>> failure :)
>>
>>
>> Sorry guys, be honest: you can't expect every deref in Java to be
>> proceeded by this kind of test?
>
>
> No, but since you brought up the issue and asked for an input ...
> anyway, your latest proposal looks fine to me.
>
yeah, sorry for assuming too much context upfront
thx for acking
-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
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)
Posted by Ugo Cei <ug...@apache.org>.
Il giorno 21/apr/04, alle 20:14, Marc Portier ha scritto:
>
>
> Joerg Heinicke wrote:
>
>> On 21.04.2004 15:34, Ugo Cei wrote:
>>> Marc Portier wrote:
>>>
>> NPE is really bad - not only here - for the two mentioned reasons.
>> But if you insist on your NPE you can change Ugo's proposal to
>> if (id == null) {
>> throw new NPE("id must not be null");
>> }
>> So we have NPE, which is RuntimeE, meaningful message and early
>> failure :)
>
> Sorry guys, be honest: you can't expect every deref in Java to be
> proceeded by this kind of test?
No, but since you brought up the issue and asked for an input ...
anyway, your latest proposal looks fine to me.
Ugo
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)
Posted by Joerg Heinicke <jo...@gmx.de>.
On 21.04.2004 22:01, Marc Portier wrote:
> by the way is what you mention above fixed now?
> I've no recollection on it.
There was no general fix. I only added the null check to all bindings
classes I got a NPE from when having an error in my binding files.
Joerg
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)
Posted by Marc Portier <mp...@outerthought.org>.
Joerg Heinicke wrote:
>
> But you can not give the form developer a NPE IMO as it happened for the
> bindings:
>
> widget = getWidget(nonExistingId);
>
> widget.aFunction();
>
> If this won't be the case for your proposal I'm ok with it.
>
nope, this is entirely different
by the way is what you mention above fixed now?
I've no recollection on it.
(ah this short memory is a bliss most of the time, thx for keeping up
with it, ... euh what was your name again?)
> Joerg
(ah yes, thank you!)
-marc= (in an odd and silly state of mind, specially after reading
Steven's latest blog entry and finding myself in community discussions,
I'ld better get to some more coding :-))
--
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
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)
Posted by Joerg Heinicke <jo...@gmx.de>.
On 21.04.2004 20:14, Marc Portier wrote:
>> NPE is really bad - not only here - for the two mentioned reasons. But
>> if you insist on your NPE you can change Ugo's proposal to
>>
>> if (id == null) {
>> throw new NPE("id must not be null");
>> }
>>
>> So we have NPE, which is RuntimeE, meaningful message and early
>> failure :)
>>
>
> Sorry guys, be honest: you can't expect every deref in Java to be
> proceeded by this kind of test?
>
>
> I admit NPE's are bad, but they indicate programming errors
> this one will surely come up during development test (believe me:
> getId() is not something that will happen only in rare cases, I think
> every sweap through any widget lifecycle is bound to call it at least
> twice IMO)
But you can not give the form developer a NPE IMO as it happened for the
bindings:
widget = getWidget(nonExistingId);
widget.aFunction();
If this won't be the case for your proposal I'm ok with it.
Joerg
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)
Posted by Marc Portier <mp...@outerthought.org>.
Joerg Heinicke wrote:
> On 21.04.2004 15:34, Ugo Cei wrote:
>
>> Marc Portier wrote:
>>
>
> NPE is really bad - not only here - for the two mentioned reasons. But
> if you insist on your NPE you can change Ugo's proposal to
>
> if (id == null) {
> throw new NPE("id must not be null");
> }
>
> So we have NPE, which is RuntimeE, meaningful message and early failure :)
>
Sorry guys, be honest: you can't expect every deref in Java to be
proceeded by this kind of test?
I admit NPE's are bad, but they indicate programming errors
this one will surely come up during development test (believe me:
getId() is not something that will happen only in rare cases, I think
every sweap through any widget lifecycle is bound to call it at least
twice IMO)
by the way: we should be discussing this in the light of getDefinition()
not in the light of getId():
it will not even be used for .equals() IIRC, reconsidering this I think
the returned string will only be used as an argument to
addCDATAAttribute, hm never tried that, does that yield an NPE?
-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
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)
Posted by Joerg Heinicke <jo...@gmx.de>.
On 21.04.2004 15:34, Ugo Cei wrote:
> Marc Portier wrote:
>
>> yeah, but this is something that never needs to be catched it really
>> is a programming error, so I find the test for null unesessary and the
>> possible resulting NPE as more then RTE enough to commincate the
>> coding error.
>
>
> The problem with the NPE is that you'd need to look at the stacktrace to
> find what the exception is about, as you get no meaningful message, and
> the exception might be thrown when you try to dereference the null
> pointer, which could be far from where getId() is called. What I propose
> is to throw a RTE when the widget is built. Fail early, if you can.
>
> Admittedly, this is just a convenience for the developer.
NPE is really bad - not only here - for the two mentioned reasons. But
if you insist on your NPE you can change Ugo's proposal to
if (id == null) {
throw new NPE("id must not be null");
}
So we have NPE, which is RuntimeE, meaningful message and early failure :)
Joerg
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)
Posted by Joerg Heinicke <jo...@gmx.de>.
On 21.04.2004 20:03, Marc Portier wrote:
>>> yeah, but this is something that never needs to be catched it really
>>> is a programming error, so I find the test for null unesessary and
>>> the possible resulting NPE as more then RTE enough to commincate the
>>> coding error.
>>
>> The problem with the NPE is that you'd need to look at the stacktrace
>> to find what the exception is about, as you get no meaningful message,
>> and the exception might be thrown when you try to dereference the null
>> pointer, which could be far from where getId() is called. What I
>> propose is to throw a RTE when the widget is built. Fail early, if you
>> can.
>
> yep that is the idea completely
> id should become a final member of the definition
> as such it needs to be provided during build
>
> it could still be set to null, but would result in a state 'abstract' of
> the definition meaning it is an a state that will make the
> createInstance() on it to throw a RTE (IllegalState IMO)
> (see wiki page WoodyRefactoring)
>
> this will result in instances that can only point back to definitions
> that have an id set != null
>
> so getId() on instance level will effectively lead to always returning a
> non-null
>
> unless we have made a programming error in setting up this chain, this
> is not a circumstance IMO that requires additional RT checking to
> generate specific RTE's
+1 sounds ok.
Joerg
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)
Posted by Marc Portier <mp...@outerthought.org>.
Ugo Cei wrote:
> Marc Portier wrote:
>
>> yeah, but this is something that never needs to be catched it really
>> is a programming error, so I find the test for null unesessary and the
>> possible resulting NPE as more then RTE enough to commincate the
>> coding error.
>
>
> The problem with the NPE is that you'd need to look at the stacktrace to
> find what the exception is about, as you get no meaningful message, and
> the exception might be thrown when you try to dereference the null
> pointer, which could be far from where getId() is called. What I propose
> is to throw a RTE when the widget is built. Fail early, if you can.
>
yep that is the idea completely
id should become a final member of the definition
as such it needs to be provided during build
it could still be set to null, but would result in a state 'abstract' of
the definition meaning it is an a state that will make the
createInstance() on it to throw a RTE (IllegalState IMO)
(see wiki page WoodyRefactoring)
this will result in instances that can only point back to definitions
that have an id set != null
so getId() on instance level will effectively lead to always returning a
non-null
unless we have made a programming error in setting up this chain, this
is not a circumstance IMO that requires additional RT checking to
generate specific RTE's
> Admittedly, this is just a convenience for the developer.
>
more then needed, no?
it seems odd to me to do a runtime check to generate a log message
saying: "The developer of this code couldn't correctly implement his own
design vision, please let him be excused and thank him for the fact that
he could at least log this nicely?"
ah true open source!
we could actually even provide the patch in the error-message and
encourage users to send it to bugzilla :-)
-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
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)
Posted by Ugo Cei <u....@cbim.it>.
Marc Portier wrote:
> yeah, but this is something that never needs to be catched it really is
> a programming error, so I find the test for null unesessary and the
> possible resulting NPE as more then RTE enough to commincate the coding
> error.
The problem with the NPE is that you'd need to look at the stacktrace to
find what the exception is about, as you get no meaningful message, and
the exception might be thrown when you try to dereference the null
pointer, which could be far from where getId() is called. What I propose
is to throw a RTE when the widget is built. Fail early, if you can.
Admittedly, this is just a convenience for the developer.
Ugo
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)
Posted by Marc Portier <mp...@outerthought.org>.
Ugo Cei wrote:
> Marc Portier wrote:
>
>> 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
>
>
> if (id == null || id.equals("")) {
> throw new FormsConfigurationException("Widget id cannot be null.");
> }
>
> where FormsConfigurationException extends RuntimeException, of course ;-).
>
yeah, but this is something that never needs to be catched it really is
a programming error, so I find the test for null unesessary and the
possible resulting NPE as more then RTE enough to commincate the coding
error.
in any case (see other message) there seems to be room for some
semantical meaning to "" and null
>> 7/ should validation stop as soon as possible or continue to allow all
>> validation errors to be set?
>
>
> If you can't make it optional, allow it to continue.
>
again see other message for the actual various conditions to consider
currently I have the same feeling that "by default we should continue",
and actually am not so far to see a real need for making it optional.
-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
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)
Posted by Ugo Cei <u....@cbim.it>.
Marc Portier wrote:
> 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
if (id == null || id.equals("")) {
throw new FormsConfigurationException("Widget id cannot be null.");
}
where FormsConfigurationException extends RuntimeException, of course ;-).
> 7/ should validation stop as soon as possible or continue to allow all
> validation errors to be set?
If you can't make it optional, allow it to continue.
Ugo