cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Portier <...@outerthought.org>
Subject Re: cvs commit: cocoon-2.1/src/blocks/woody/samples/forms form_model_gui_binding.xml form_model_gui_data.xml form_model_gui_model.xml form_model_gui_success.xsp form_model_gui_template.xml
Date Mon, 29 Dec 2003 15:14:21 GMT
Hi Tim,

quite a list of changes,
hoping to show due respect for your work by providing a list of remarks 
and questions...

>   Log:
>   Class, New, Struct, and Union added to cforms
>   binding, model, and template implementations.
>   See http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=107230320407513&w=2
>   

In all honesty: the lesson to learn is "early release rocks"... your 
work is a big chunk to swallow right here and now, colaboration could of 
have been higher by spreading these out over the last few months...


>   
>   1.10      +1 -0      cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/DefaultFormManager.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/DefaultFormManager.java.diff?r1=1.9&r2=1.10
>   

(a bit up-front of reading more patches that could explain)
The reason of this resolve() is to slice in the new-class definition stuff?

Given all the time you've already put in (and the published diffs on the 
wiki) I'm a bit late at fealing uneasy on these things, nevertheless:

Thinking about the now and then requested 'catalog' of widget 
definitions it maybe makes more sense to have that declared in the 
woody-form.xconf so we build up one central catalog that is available 
during the building of each form?  In this case the 'resolve' would be 
done immediately during the building rather then afterwards by the 
widgets itself.

In fact, I've been thinking about considering the same private/final 
approach for the members of widget's itself...


>   
>   1.4       +18 -0     cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/Binding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/Binding.java.diff?r1=1.3&r2=1.4
>   

these additions seems to have nothing to do with the functional service 
of binding itself, I would of liked it more if these were defined lower 
in the inheritance hierachy?

also they indicate some 'state' of the binding classes which kind of 
fundamentally breaks with the original vision for those (sorry for so 
much defensiveness, just want to share:)   We did an effort there to 
make all fields private and final so the bindings itself would result in 
  immutables and thus ensure threadsafe use of those objects. 
(setParent brakes the final idea, I understand this kind of 
bidirectional pointers can only be achieved in this way, but then again 
I have a healthy dislike for bidirectional pointers in OO models)

reading some further down I realize this has to do with the class-new 
bindings I guess, so maybe we could overcome this with a similar catalog 
approach as expressed above ?

>   
>   1.5       +33 -0     cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/ComposedJXPathBindingBase.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/ComposedJXPathBindingBase.java.diff?r1=1.4&r2=1.5
>   

Hm, you seem to be building a local 'class' cache on the level of each 
composed-binding

do you rely on the implicit 'scoping' of class-definitions this gives 
you?  I also notice you end up (through its base class) retrieving the 
found class-binding over at its parent and copying over the reference

maybe this is more argumentation for building a more central catalog 
with one shared class right away (but I'm in doubth about the scoping 
thing still)?

allowing other classes to see the getChildBindings() breaks somewhat the 
immutable nature of these classes: someone else can now alter that array 
even while its reference is kept private final
(I know this is nitpitting)

>   
>   1.8       +57 -0     cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/JXPathBindingBase.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/JXPathBindingBase.java.diff?r1=1.7&r2=1.8
>   

have struglled a bit with the usefulness of getClass() on this level
but I take it this is for those Bindings that have dedicated 
sub-bindings but are themselves not subclasses of ComposedJXpathBinding?

>   
>   1.13      +3 -0      cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/RepeaterJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/RepeaterJXPathBinding.java.diff?r1=1.12&r2=1.13
>   

yep, showing my late understanding of above...

hm, afraid this introduces some possible NPE's
(since insert and delete nodes are optional --> those childbindings will 
be zero, fixing...)

>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/CaseJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/CaseJXPathBinding.java?rev=1.1
>   

Pls consider using the super.doLoad() and super.doSave() as shown in 
AggregateJXPathBinding that way you can benefit from the inheritance AND 
  allow to keep getChildBindings() private

(hm, except for the introduction of the NewJXpathBinding further down 
that relies on it to be public too, see further)

>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/CaseJXPathBindingBuilder.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/CaseJXPathBindingBuilder.java?rev=1.1
>   
>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/ClassJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/ClassJXPathBinding.java?rev=1.1
>   
>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/ClassJXPathBindingBuilder.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/ClassJXPathBindingBuilder.java?rev=1.1
>   
>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/NewJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/NewJXPathBinding.java?rev=1.1
>   
>   

Again, the runtime resolving isn't something I particularly like here, I 
would rather have this done during the building process.

Not completely sure, but the applied trick of getting the childbindings 
of the class-binding limits the use of class-bindings inside 
class-bindings?  (without being sure if such would lead to any usefull 
use cases, just want to understand)

I'm also left to wonder why this thing has childBindings of its own (ie 
is a subclass of ComposedJXPathBinding?)
(since there is no call to super for load or save nor is there a 
reference to its own childBindings at any time.)


>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/NewJXPathBindingBuilder.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/NewJXPathBindingBuilder.java?rev=1.1
>   

When I think about the 'central catalog', then I would like to see the 
assistant to the building process to also allow for a 
makeBindingFromCatalog(id)...

>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/StructJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/StructJXPathBinding.java?rev=1.1
>   

same remark on reuse/inheritance super.doLoad as for the CaseJXPathBinding

additionally Struct, Case, Context now all 3 seem to be very similar.
(later found that Union is in a similar spot)

- Context lacks the ability to narrow down on the widget-side (which we 
could easily add IMHO by introducing an optional @id for the context 
binding: if it is not there: no widget-side narrowing, of it is there: 
widget-side narrowing to the specified widget)
- Struct and Case throw in an additional type-check on the fact that the 
widget is indeed of such type (note that after the downcast - possibly 
yielding a classcastexcpetion - there is just an implicit upcast which 
makes it just any widget again to the child-bindings)

Personally I don't think we need this more thight coupling to the actual 
widget-types in the binding? In other words: I don't think there needs 
to be a 1-1 match there.  The downcast is only there to impose just that 
while not adding additional features?
(the net result being more code to maintain and sync that doesn't really 
add up?)

Hm, actually if we agree on this, then we should even get rid of the 
aggregate-binding?  Have to think some more, comments welcome...

In any case if we do want to have the widget-type checking then it could 
still be achieved by some @type on the context-binding

>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/StructJXPathBindingBuilder.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/StructJXPathBindingBuilder.java?rev=1.1
>   
>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/TempRepeaterJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/TempRepeaterJXPathBinding.java?rev=1.1
>   
>   

what is RAD? (as in //TODO: RAD)  :-)

This reads like the simple-repeater with support for the nested elements 
of the classic (diff) repeater, correct?

I would be +1 to already replace the simple-repeater with this, and then 
work up to a situation where this and the diff-repeater share more code 
(and possibly even configuration with the @strategy idea --> their 
builders are very similar now, no?)


>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/TempRepeaterJXPathBindingBuilder.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/TempRepeaterJXPathBindingBuilder.java?rev=1.1
>   
>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/UnionJXPathBinding.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/UnionJXPathBinding.java?rev=1.1
>   

Same remarks as for Case and Struct (see higher)

>   
>   1.1                  cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/UnionJXPathBindingBuilder.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/binding/UnionJXPathBindingBuilder.java?rev=1.1
>   
>   
>   1.7       +39 -1     cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/formmodel/AbstractWidget.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/formmodel/AbstractWidget.java.diff?r1=1.6&r2=1.7
>   

personally would prefer to avoid 'protected' and a setter on the 
definition member

do we really want to do nothing inside: generateItemSaxFragment()
or could we make it abstract?

>   
>   1.5       +28 -0     cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/formmodel/AbstractWidgetDefinition.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/formmodel/AbstractWidgetDefinition.java.diff?r1=1.4&r2=1.5
>   

introduces the parent/child bidirectional links I'ld rather avoid
just like with the class-binding this would require us to escape into 
some central catalog...


>   
>   1.2       +145 -6    cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/formmodel/ContainerDefinitionDelegate.java
>   
>   http://cvs.apache.org/viewcvs/cocoon-2.1/src/blocks/woody/java/org/apache/cocoon/woody/formmodel/ContainerDefinitionDelegate.java.diff?r1=1.1&r2=1.2
>   

again runtime resolving :-(


the rest of this commit still needs some deeper investigation (have to 
admit I'm not that well into the formmodel yet)

as for the sample --> great new features!


bottom line:
1/ a tad too much at once, let us try to collaborate more by doing 
smaller increments

2/ actually I would like us to reconsider the reuse aspect, I honestly 
think a central catalogue will be more usefull, and less confusing (the 
class thingy's are at least to me confusing: widgets that define widgets 
seem to be more 'builders' then widgets)

but this is just my opnion, wdot?
-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