On 11/8/06, Martijn Dashorst <[EMAIL PROTECTED]> wrote:
I see two disadvantages: order and exception handling for the detach
methods.
The order /could/ be checked using pmd or aop, but that is something
we can't enforce, from a framework point of view a no-go IMO.
this isnt about component hierarchy - the order there is enforced by the
visitors. this is about class hierarchy.
when overriding onattach() you have to let the parent superclass initialize
first so your implementation must always look like this
public void onAttach() {
super.onAttach(); // always called first
myattachcode();
}
when overriding ondetach() it must be opposite, you must detach yourself and
then let the superclass detach()
public void onDetach() {
mydetachcode();
super.detach(); // always called last
}
but then again this is java 101.
annotations let you sidestep the problem by removing the need to override,
but since we plan on keeping the original onattach()/ondetach()/etc handlers
anyways the problem is still there. and i would also argue against removing
them completely because that seriously cuts down on their visibility.
Now you rely on your users to properly ensure that all detach methods
are called, and especially the super().
we cant enforce the order on any other overrides that we have, so i dont see
why this is a no-go. we must clearly outline in the javadoc that overrides
of this method require a call to super either in the beginning or the end.
sensible users should be able to derive that from the context as well.
Can't we come up with a better, automagic way of ensuring detach is
called, that is independent of the call hierarchy?
If it is only searching for IDetachable and calling detach on it, why
not walk the object graph and find all non-transient fields that
implement IDetachable and call detach?
as i said above, this isnt about the object graph but rather the class
hierarchy.
-igor
Martijn
On 11/9/06, Igor Vaynberg <[EMAIL PROTECTED]> wrote:
> after having implemented the annotations and thought about this some
more, i
> think i ended up patching the problem instead of fixing it.
>
> there are a couple of problems with annotations that i now see:
>
> a) we are not removing the overridible listeners from the api, so
> annotations are just an alternative approach.
>
> b) annotations have poor api visibility - unless you know about them its
> very likely you are not going to find them
>
> the reason why we have things like
> Component.internalAttach
()+Component.internalOnAttach()+Component.onAttach()
> is that for methods like these the call to super() is very important.
>
> in init methods like onattach() it must be the first thing that is
called.
> int cleanup methods like ondetach() it must be the last thing to be
called.
>
> i think a more proper fix to the problem would be to properly document
these
> and leave it to the users to implement properly. that way we can get rid
of
> the annotations and get back some api clarity by remoing
> Component.internalAttach() and Component.internalOnAttach() and moving
that
> code into component.onattach().
>
> now this is problematic because it is easy to forget to call super, and
> there is a lot of existing code out there that doesnt. i think what we
can
> do is either create an aspect to check for the calls to super that is
loaded
> only during devel mode, or create an inspector for a static analysis
tool
> like pmd or findbugs to look for the error.
>
> i think this will be a better solution then relying on annotations.
>
> in the end the user has to know the consquences when overriding a
method. us
> trying to do as much as possible for the user only takes it part way -
to
> the Component level. But what if you extend Component and need to
override
> onAttach()? and then then another user subclasses this component? then
the
> problem of calling super is still there, we just deferred it for our
> framework internal classes.
>
> what do you think?
>
> -igor
>
>
> On 10/27/06, Igor Vaynberg <[EMAIL PROTECTED]> wrote:
> >
> > we have a couple of great candidate callbacks to be implemented with
> > annotations instead of template methods. namely onattach/detach,
> > onbegin/endrender
> >
> > what makes them great candidates is this
> >
> > 1) when implementing one you must always call super()
> > this is not _always_ true. when extending a core component you dont
need
> > to because we have our own internalattach() we can use and onattach()
is
> > just an empty template.
> >
> > but when extending a non-core component you must call super() because
you
> > never know if that component uses it or not and it is very very
unlikely
> > that it is a behavior you want to override
> >
> > 2) implementors of onattach(), if they are good, will make it final
and
> > provide another breakout template to ensure their onattach code cannot
be
> > messed with. this leads you to have
> >
> > public final void onAttach(){ dosomething(); onAttach2(); } public
void
> > onAttach2() {}
> >
> > this is the general java pattern that i find very ugly personally,
take a
> > look at our Component.internalAttach() impl :)
> >
> > so what i propose is that we provide some annotations
> > @OnAttach @OnDetach @OnBeforeRender @OnAfterRender
> >
> > users can annotate any number of methods with these and have them
called.
> >
> > im sure there are other places that can benefit from this, but for now
we
> > can cleanup the mess that is internalAttach(), internalOnAttach()
> > onAttach(), etc
> >
> > of course this is wicket 2.0 only
> >
> > -Igor
> >
> >
>
>
--
<a href="http://www.thebeststuffintheworld.com/vote_for/wicket";>Vote</a>
for <a href="http://www.thebeststuffintheworld.com/stuff/wicket
">Wicket</a>
at the <a href="http://www.thebeststuffintheworld.com/";>Best Stuff in
the World!</a>
