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]
