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/ [EMAIL PROTECTED] [EMAIL PROTECTED]
