Howard Lewis Ship <hlship <at> gmail.com> writes: > The FormPersist annotation is very clever and useful. I'm a little > troubled that a field's label and disabled flags are persisted in the > form. The label should just be available again, and the disabled flag > is something I'd like to recompute when the form is submitted.
Thanks for looking at the code. The label may have been set in a loop (eg, name1, name2). The container certainly could make it available on form submission, but the component should be told what to do when it is kicked off (rendering) and then automatically do its work, without bothering the container again. I don't understand why the disabled flag should be recomputed. > Also, a persistence strategy of "form" may be preferable to a new > annotation. As I documented in the FormPersist file, I think it doesn't fit in as a persistent field strategy because such persistent fields are restored at the moment the page is attached, which doesn't work for form submissions and doesn't work if the component is in a loop. > It's also very heavily tied to AbstractField implementation (the > collectFormPersistPropNames). Why do you say that? The logic is in RestorePropertiesAction. As long as the component implements FormPersistPropertySource to provide the collectFormPersistPropNames() method, then it will work. For example, The BeanEditForm and the Loop components also implement FormPersistPropertySource and have @FormPersist properties. > Please spell out "property". OK. > This means that it will be harder to create a "composite field" component > that implements the Field interface and delegates out to a number of > real fields. Such a component can implement FormPersistPropertySource and then let RestorePropertiesAction do the work. > I'm not happy with the hooks into Loop to support Forms via > RestorePropertiesAction.storeAction(). This seems like something that > should be done inside Form/FormSupport or make use of the Heartbeat > environmental service in order to occur as part of the Loop without > tying the two classes so closely together. You don't see a lot of > class-calling-class in Tapestry, its usually > implementation-calls-interface. OK. > I particularly don't like the way that BindingExprValueConduit works, > it's not very IoC ... instead, it forces every component to implement > these additional service interfaces (BindingSourceProvider). It looks > like these things get persisted into the form as well (which would > explain the need for lookup, since IoC service proxies don't > serialize). Right. A solution may be to make the infrastructure available through each enhanced component class. It takes a few more byte codes, but if it is not called, it won't slow down anything. > So now containers have to implement a configure_foo() method for each > component. Underscores? Since ids are unique (caselessly) then it > could be configureFoo() which would be more consistent with, say, > Tapestry IOC & etc. Yes, I want configureFoo() a lot, but in order to write a code template in Eclipse, the best I get is: @Component private ${type} _${id}; public void configure${id}() { _${id}.set${name}(); } Then the method will become configurefoo(). > Why a seperate method for each component? Since components appear to > always be instance variables now, a single method per container > component could configure all contained components. Not really. A component could be rendered multiple times in a loop. > Please avoid abbreviated names like "Expr" for "Expression". In fact, > setValueBindingExpr() could be called setValueBinding() or even > bindValue(). OK. > Ouch. RestorePropertiesAction is also based on reflection to read the > property values that will be stored into the form. Much better to > gain access to a PropertyAccess service or better yet, a > PropertyConduit (which avoids reflection in favor of bytecode > generation). OK. > OK ... I'm pulling down large scale diffs to see how everything fits > together. I'm beginning to think that components must re-configure > themselves (invoke a configure method on their parent) as part of > their lifecycle? Yes. > It looks like <at> Parameter is just completely gone. Performance issue: > caching. As fast as methods can be invoked via PropertyConduit (i.e., > via bytecode generation), if the method called is slow, that's a > problem. I guess there's just a tiny minority of "parameters" that are > read/write and those are not read or written very often, so just FUD. Right. > Looks like much more stuff must be serializable (related to > <at> FormPersist). A long term philosophy in Tapestry has been "store only > what cannot be recreated". This approach seems more like "store > everything" (on the client)". I can hear the .Net jokes coming True. Storing only what cannot be recreated has the advantage of better performance. Storing what can be stored has the following advantages: 1) You just kick off the component and then it will do the job without you worrying about helping it later (of course, if the component needs to write values back to you, you have to make sure there is a proper place to write). It means it takes more work to write the component, but it is easier to use the component. 2) This very much reduces the possibility of having a different parameter value on form submission from rendering. For example, it's quite hard to debug if the SelectModel has changed because it depends on some other value. > Nope, looks like the configure_foo methods are invoked via REFLECTION. > Ick. This is someting that could be done by writing code to invoke > the configure method, and doing so with a > ComponentClassTransformationWorker. And this occurs from just before > the SetupRender state during rendering. I tried exactly that (ComponentClassTransformationWorker) without success. It has something to do classloaders. The code generated uses a type cast which fails at runtime. > This basically assumes that all the properties of a component will be > stable for the duration of the component's rendering. Yes. > I guess all the properties will be reset at the end of the request. Yes. It's quite late. I'll reply to the other points later... -- Author of a book for learning Tapestry (http://www.agileskills2.org/EWDT) --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]