You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Matej Knopp <ma...@gmail.com> on 2007/03/26 10:29:07 UTC
onDetach() contract, detachModels
Hi,
i think we should finally define onDetach() contract and decide what
to do with detachModels().
Right now, after the detach attach/refactor the situation is that
onDetach is called only for components which were attached and
detachModels is called for every component.
While this kinda does make sense, there are two possible problems:
- the semantics are not very well defined and when which method is
called can be confusing
- when there is model in component that is not in the default "slot"
(get/setModel), it will not be detached properly unless the whole
component was attached.
The goal of attach/detach refactor was to simplify the things and I
think we can go a little bit further. I propose merging detachModels
with detach. And I also propose that onDetach should be called for
every component. IMHO it would be safer and simpler than what we have
right now.
The onDetach sematics would be defined something like this:
The method is called on the end of request, even if the component was
not attached.
The detach methods are usually rather simple, so I really don't think
there would be a big performance penalty, as we already need to
traverse the component tree.
WDYT?
-Matej
Re: onDetach() contract, detachModels
Posted by Eelco Hillenius <ee...@gmail.com>.
+1
Eelco
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> Yeah, that's my opinion too. I don't care about duplicate onDetach
> calls, and that will only happen if there are multiple request targets
> on stack with same page which should be rather rare.
>
> If there is an onDetach handler that should only be called when
> component is attached, it's triviial to build the check for the
> specific component.
>
> -Matej
>
> On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > if we don't care (there where people complaining about double detach calls..
> > (i think martijn itself was one))
> > then we can drop the if(ATTACHED) completely and just do everything in one
> > method
> >
> > johan
> >
> >
> > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > >
> > > I don't see as a problem that onDetach would be called multiple times.
> > >
> > > Yes, detachModels() is not final, but the goal is to merge it with
> > > detach() thus simplifying the detaching a bit.
> > >
> > > -Matej
> > >
> > > On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > > > detachModels is not final.
> > > > So if you want to detach a model that be done in detachmodels...
> > > >
> > > > The only other solution is that onDetach is always called (but then also
> > > > twice if there are multiply request targets on the stack with the same
> > > page)
> > > >
> > > > But The biggest problem is attach itself. That should really be called
> > > > earlier (not only for rendered components)
> > > > But we can't really change that i am afraid.
> > > >
> > > > johan
> > > >
> > > > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > > > >
> > > > > It only detachs the "default" component model, i.e. the one you set
> > > > > with setModel. if you have something like this
> > > > >
> > > > > class MyComponent ... {
> > > > > IModel myModel = ...
> > > > >
> > > > > }
> > > > >
> > > > > and you clean myModel in onDetach, that won't work. I'm not sure how
> > > > > rare this is and if you assign mymodel to a component it will be
> > > > > detached anyway, but there is the possibility.
> > > > >
> > > > > -Matej
> > > > >
> > > > > On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > > > > > as far as i see it does already what i expect it todo:
> > > > > >
> > > > > >
> > > > > > if (getFlag(FLAG_ATTACHED))
> > > > > > {
> > > > > > // if the component has been previously attached via
> > > > > attach()
> > > > > > // detach it now
> > > > > > setFlag(FLAG_DETACHING, true);
> > > > > > onDetach();
> > > > > > if (getFlag(FLAG_DETACHING))
> > > > > > {
> > > > > > throw new IllegalStateException(
> > > Component.class.getName
> > > > > ()
> > > > > > + " has not been properly detached.
> > > Something in
> > > > > the
> > > > > > hierarchy of "
> > > > > > + getClass().getName()
> > > > > > + " has not called super.onDetach() in the
> > > > > override
> > > > > > of onDetach() method");
> > > > > > }
> > > > > > setFlag(FLAG_ATTACHED, false);
> > > > > > }
> > > > > >
> > > > > > // always detach models because they can be attached without
> > > the
> > > > > > // component. eg component has a compoundpropertymodel and
> > > one
> > > > > of
> > > > > > its
> > > > > > // children component's getmodelobject is called
> > > > > > detachModels();
> > > > > >
> > > > > > // always detach children because components can be attached
> > > > > > // independently of their parents
> > > > > > detachChildren();
> > > > > > }
> > > > > >
> > > > > > So only if attached the onDetached is called
> > > > > > But detachModels and detachChildren are always called.
> > > > > >
> > > > > > isn't that correct? Where does it go wrong now?
> > > > > >
> > > > > > johan
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > i think we should finally define onDetach() contract and decide
> > > what
> > > > > > > to do with detachModels().
> > > > > > >
> > > > > > > Right now, after the detach attach/refactor the situation is that
> > > > > > > onDetach is called only for components which were attached and
> > > > > > > detachModels is called for every component.
> > > > > > >
> > > > > > > While this kinda does make sense, there are two possible problems:
> > > > > > > - the semantics are not very well defined and when which method is
> > > > > > > called can be confusing
> > > > > > > - when there is model in component that is not in the default
> > > "slot"
> > > > > > > (get/setModel), it will not be detached properly unless the whole
> > > > > > > component was attached.
> > > > > > >
> > > > > > > The goal of attach/detach refactor was to simplify the things and
> > > I
> > > > > > > think we can go a little bit further. I propose merging
> > > detachModels
> > > > > > > with detach. And I also propose that onDetach should be called for
> > > > > > > every component. IMHO it would be safer and simpler than what we
> > > have
> > > > > > > right now.
> > > > > > >
> > > > > > > The onDetach sematics would be defined something like this:
> > > > > > > The method is called on the end of request, even if the component
> > > was
> > > > > > > not attached.
> > > > > > >
> > > > > > > The detach methods are usually rather simple, so I really don't
> > > think
> > > > > > > there would be a big performance penalty, as we already need to
> > > > > > > traverse the component tree.
> > > > > > >
> > > > > > > WDYT?
> > > > > > >
> > > > > > > -Matej
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: onDetach() contract, detachModels
Posted by Eelco Hillenius <ee...@gmail.com>.
> Duplicate detaches are not something to worry about, imo too, but they
> look stupid. If they can be prevented, then all the better.
We could hold a temp hash set with the detached components so far and
check against that, but that wouldn't say a thing about how the
detaches cascade to subclasses. And if we can't reliably prevent it,
we probably shouldn't bother with it at all.
> If not, we should at least tell/document that it is expected behavior.
That's a good idea anyway.
Eelco
Re: onDetach() contract, detachModels
Posted by Martijn Dashorst <ma...@gmail.com>.
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> Yeah, that's my opinion too. I don't care about duplicate onDetach
> calls, and that will only happen if there are multiple request targets
> on stack with same page which should be rather rare.
Duplicate detaches are not something to worry about, imo too, but they
look stupid. If they can be prevented, then all the better. If not, we
should at least tell/document that it is expected behavior.
Martijn
--
Learn Wicket at ApacheCon Europe: http://apachecon.com
Join the wicket community at irc.freenode.net: ##wicket
Wicket 1.2.5 will keep your server alive. Download Wicket now!
http://wicketframework.org
Re: onDetach() contract, detachModels
Posted by Matej Knopp <ma...@gmail.com>.
Yeah, that's my opinion too. I don't care about duplicate onDetach
calls, and that will only happen if there are multiple request targets
on stack with same page which should be rather rare.
If there is an onDetach handler that should only be called when
component is attached, it's triviial to build the check for the
specific component.
-Matej
On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> if we don't care (there where people complaining about double detach calls..
> (i think martijn itself was one))
> then we can drop the if(ATTACHED) completely and just do everything in one
> method
>
> johan
>
>
> On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> >
> > I don't see as a problem that onDetach would be called multiple times.
> >
> > Yes, detachModels() is not final, but the goal is to merge it with
> > detach() thus simplifying the detaching a bit.
> >
> > -Matej
> >
> > On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > > detachModels is not final.
> > > So if you want to detach a model that be done in detachmodels...
> > >
> > > The only other solution is that onDetach is always called (but then also
> > > twice if there are multiply request targets on the stack with the same
> > page)
> > >
> > > But The biggest problem is attach itself. That should really be called
> > > earlier (not only for rendered components)
> > > But we can't really change that i am afraid.
> > >
> > > johan
> > >
> > > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > > >
> > > > It only detachs the "default" component model, i.e. the one you set
> > > > with setModel. if you have something like this
> > > >
> > > > class MyComponent ... {
> > > > IModel myModel = ...
> > > >
> > > > }
> > > >
> > > > and you clean myModel in onDetach, that won't work. I'm not sure how
> > > > rare this is and if you assign mymodel to a component it will be
> > > > detached anyway, but there is the possibility.
> > > >
> > > > -Matej
> > > >
> > > > On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > > > > as far as i see it does already what i expect it todo:
> > > > >
> > > > >
> > > > > if (getFlag(FLAG_ATTACHED))
> > > > > {
> > > > > // if the component has been previously attached via
> > > > attach()
> > > > > // detach it now
> > > > > setFlag(FLAG_DETACHING, true);
> > > > > onDetach();
> > > > > if (getFlag(FLAG_DETACHING))
> > > > > {
> > > > > throw new IllegalStateException(
> > Component.class.getName
> > > > ()
> > > > > + " has not been properly detached.
> > Something in
> > > > the
> > > > > hierarchy of "
> > > > > + getClass().getName()
> > > > > + " has not called super.onDetach() in the
> > > > override
> > > > > of onDetach() method");
> > > > > }
> > > > > setFlag(FLAG_ATTACHED, false);
> > > > > }
> > > > >
> > > > > // always detach models because they can be attached without
> > the
> > > > > // component. eg component has a compoundpropertymodel and
> > one
> > > > of
> > > > > its
> > > > > // children component's getmodelobject is called
> > > > > detachModels();
> > > > >
> > > > > // always detach children because components can be attached
> > > > > // independently of their parents
> > > > > detachChildren();
> > > > > }
> > > > >
> > > > > So only if attached the onDetached is called
> > > > > But detachModels and detachChildren are always called.
> > > > >
> > > > > isn't that correct? Where does it go wrong now?
> > > > >
> > > > > johan
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > i think we should finally define onDetach() contract and decide
> > what
> > > > > > to do with detachModels().
> > > > > >
> > > > > > Right now, after the detach attach/refactor the situation is that
> > > > > > onDetach is called only for components which were attached and
> > > > > > detachModels is called for every component.
> > > > > >
> > > > > > While this kinda does make sense, there are two possible problems:
> > > > > > - the semantics are not very well defined and when which method is
> > > > > > called can be confusing
> > > > > > - when there is model in component that is not in the default
> > "slot"
> > > > > > (get/setModel), it will not be detached properly unless the whole
> > > > > > component was attached.
> > > > > >
> > > > > > The goal of attach/detach refactor was to simplify the things and
> > I
> > > > > > think we can go a little bit further. I propose merging
> > detachModels
> > > > > > with detach. And I also propose that onDetach should be called for
> > > > > > every component. IMHO it would be safer and simpler than what we
> > have
> > > > > > right now.
> > > > > >
> > > > > > The onDetach sematics would be defined something like this:
> > > > > > The method is called on the end of request, even if the component
> > was
> > > > > > not attached.
> > > > > >
> > > > > > The detach methods are usually rather simple, so I really don't
> > think
> > > > > > there would be a big performance penalty, as we already need to
> > > > > > traverse the component tree.
> > > > > >
> > > > > > WDYT?
> > > > > >
> > > > > > -Matej
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: onDetach() contract, detachModels
Posted by Johan Compagner <jc...@gmail.com>.
if we don't care (there where people complaining about double detach calls..
(i think martijn itself was one))
then we can drop the if(ATTACHED) completely and just do everything in one
method
johan
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
>
> I don't see as a problem that onDetach would be called multiple times.
>
> Yes, detachModels() is not final, but the goal is to merge it with
> detach() thus simplifying the detaching a bit.
>
> -Matej
>
> On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > detachModels is not final.
> > So if you want to detach a model that be done in detachmodels...
> >
> > The only other solution is that onDetach is always called (but then also
> > twice if there are multiply request targets on the stack with the same
> page)
> >
> > But The biggest problem is attach itself. That should really be called
> > earlier (not only for rendered components)
> > But we can't really change that i am afraid.
> >
> > johan
> >
> > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > >
> > > It only detachs the "default" component model, i.e. the one you set
> > > with setModel. if you have something like this
> > >
> > > class MyComponent ... {
> > > IModel myModel = ...
> > >
> > > }
> > >
> > > and you clean myModel in onDetach, that won't work. I'm not sure how
> > > rare this is and if you assign mymodel to a component it will be
> > > detached anyway, but there is the possibility.
> > >
> > > -Matej
> > >
> > > On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > > > as far as i see it does already what i expect it todo:
> > > >
> > > >
> > > > if (getFlag(FLAG_ATTACHED))
> > > > {
> > > > // if the component has been previously attached via
> > > attach()
> > > > // detach it now
> > > > setFlag(FLAG_DETACHING, true);
> > > > onDetach();
> > > > if (getFlag(FLAG_DETACHING))
> > > > {
> > > > throw new IllegalStateException(
> Component.class.getName
> > > ()
> > > > + " has not been properly detached.
> Something in
> > > the
> > > > hierarchy of "
> > > > + getClass().getName()
> > > > + " has not called super.onDetach() in the
> > > override
> > > > of onDetach() method");
> > > > }
> > > > setFlag(FLAG_ATTACHED, false);
> > > > }
> > > >
> > > > // always detach models because they can be attached without
> the
> > > > // component. eg component has a compoundpropertymodel and
> one
> > > of
> > > > its
> > > > // children component's getmodelobject is called
> > > > detachModels();
> > > >
> > > > // always detach children because components can be attached
> > > > // independently of their parents
> > > > detachChildren();
> > > > }
> > > >
> > > > So only if attached the onDetached is called
> > > > But detachModels and detachChildren are always called.
> > > >
> > > > isn't that correct? Where does it go wrong now?
> > > >
> > > > johan
> > > >
> > > >
> > > >
> > > >
> > > > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > i think we should finally define onDetach() contract and decide
> what
> > > > > to do with detachModels().
> > > > >
> > > > > Right now, after the detach attach/refactor the situation is that
> > > > > onDetach is called only for components which were attached and
> > > > > detachModels is called for every component.
> > > > >
> > > > > While this kinda does make sense, there are two possible problems:
> > > > > - the semantics are not very well defined and when which method is
> > > > > called can be confusing
> > > > > - when there is model in component that is not in the default
> "slot"
> > > > > (get/setModel), it will not be detached properly unless the whole
> > > > > component was attached.
> > > > >
> > > > > The goal of attach/detach refactor was to simplify the things and
> I
> > > > > think we can go a little bit further. I propose merging
> detachModels
> > > > > with detach. And I also propose that onDetach should be called for
> > > > > every component. IMHO it would be safer and simpler than what we
> have
> > > > > right now.
> > > > >
> > > > > The onDetach sematics would be defined something like this:
> > > > > The method is called on the end of request, even if the component
> was
> > > > > not attached.
> > > > >
> > > > > The detach methods are usually rather simple, so I really don't
> think
> > > > > there would be a big performance penalty, as we already need to
> > > > > traverse the component tree.
> > > > >
> > > > > WDYT?
> > > > >
> > > > > -Matej
> > > > >
> > > >
> > >
> >
>
Re: onDetach() contract, detachModels
Posted by Matej Knopp <ma...@gmail.com>.
I don't see as a problem that onDetach would be called multiple times.
Yes, detachModels() is not final, but the goal is to merge it with
detach() thus simplifying the detaching a bit.
-Matej
On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> detachModels is not final.
> So if you want to detach a model that be done in detachmodels...
>
> The only other solution is that onDetach is always called (but then also
> twice if there are multiply request targets on the stack with the same page)
>
> But The biggest problem is attach itself. That should really be called
> earlier (not only for rendered components)
> But we can't really change that i am afraid.
>
> johan
>
> On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> >
> > It only detachs the "default" component model, i.e. the one you set
> > with setModel. if you have something like this
> >
> > class MyComponent ... {
> > IModel myModel = ...
> >
> > }
> >
> > and you clean myModel in onDetach, that won't work. I'm not sure how
> > rare this is and if you assign mymodel to a component it will be
> > detached anyway, but there is the possibility.
> >
> > -Matej
> >
> > On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > > as far as i see it does already what i expect it todo:
> > >
> > >
> > > if (getFlag(FLAG_ATTACHED))
> > > {
> > > // if the component has been previously attached via
> > attach()
> > > // detach it now
> > > setFlag(FLAG_DETACHING, true);
> > > onDetach();
> > > if (getFlag(FLAG_DETACHING))
> > > {
> > > throw new IllegalStateException(Component.class.getName
> > ()
> > > + " has not been properly detached. Something in
> > the
> > > hierarchy of "
> > > + getClass().getName()
> > > + " has not called super.onDetach() in the
> > override
> > > of onDetach() method");
> > > }
> > > setFlag(FLAG_ATTACHED, false);
> > > }
> > >
> > > // always detach models because they can be attached without the
> > > // component. eg component has a compoundpropertymodel and one
> > of
> > > its
> > > // children component's getmodelobject is called
> > > detachModels();
> > >
> > > // always detach children because components can be attached
> > > // independently of their parents
> > > detachChildren();
> > > }
> > >
> > > So only if attached the onDetached is called
> > > But detachModels and detachChildren are always called.
> > >
> > > isn't that correct? Where does it go wrong now?
> > >
> > > johan
> > >
> > >
> > >
> > >
> > > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > i think we should finally define onDetach() contract and decide what
> > > > to do with detachModels().
> > > >
> > > > Right now, after the detach attach/refactor the situation is that
> > > > onDetach is called only for components which were attached and
> > > > detachModels is called for every component.
> > > >
> > > > While this kinda does make sense, there are two possible problems:
> > > > - the semantics are not very well defined and when which method is
> > > > called can be confusing
> > > > - when there is model in component that is not in the default "slot"
> > > > (get/setModel), it will not be detached properly unless the whole
> > > > component was attached.
> > > >
> > > > The goal of attach/detach refactor was to simplify the things and I
> > > > think we can go a little bit further. I propose merging detachModels
> > > > with detach. And I also propose that onDetach should be called for
> > > > every component. IMHO it would be safer and simpler than what we have
> > > > right now.
> > > >
> > > > The onDetach sematics would be defined something like this:
> > > > The method is called on the end of request, even if the component was
> > > > not attached.
> > > >
> > > > The detach methods are usually rather simple, so I really don't think
> > > > there would be a big performance penalty, as we already need to
> > > > traverse the component tree.
> > > >
> > > > WDYT?
> > > >
> > > > -Matej
> > > >
> > >
> >
>
Re: onDetach() contract, detachModels
Posted by Johan Compagner <jc...@gmail.com>.
detachModels is not final.
So if you want to detach a model that be done in detachmodels...
The only other solution is that onDetach is always called (but then also
twice if there are multiply request targets on the stack with the same page)
But The biggest problem is attach itself. That should really be called
earlier (not only for rendered components)
But we can't really change that i am afraid.
johan
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
>
> It only detachs the "default" component model, i.e. the one you set
> with setModel. if you have something like this
>
> class MyComponent ... {
> IModel myModel = ...
>
> }
>
> and you clean myModel in onDetach, that won't work. I'm not sure how
> rare this is and if you assign mymodel to a component it will be
> detached anyway, but there is the possibility.
>
> -Matej
>
> On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> > as far as i see it does already what i expect it todo:
> >
> >
> > if (getFlag(FLAG_ATTACHED))
> > {
> > // if the component has been previously attached via
> attach()
> > // detach it now
> > setFlag(FLAG_DETACHING, true);
> > onDetach();
> > if (getFlag(FLAG_DETACHING))
> > {
> > throw new IllegalStateException(Component.class.getName
> ()
> > + " has not been properly detached. Something in
> the
> > hierarchy of "
> > + getClass().getName()
> > + " has not called super.onDetach() in the
> override
> > of onDetach() method");
> > }
> > setFlag(FLAG_ATTACHED, false);
> > }
> >
> > // always detach models because they can be attached without the
> > // component. eg component has a compoundpropertymodel and one
> of
> > its
> > // children component's getmodelobject is called
> > detachModels();
> >
> > // always detach children because components can be attached
> > // independently of their parents
> > detachChildren();
> > }
> >
> > So only if attached the onDetached is called
> > But detachModels and detachChildren are always called.
> >
> > isn't that correct? Where does it go wrong now?
> >
> > johan
> >
> >
> >
> >
> > On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > i think we should finally define onDetach() contract and decide what
> > > to do with detachModels().
> > >
> > > Right now, after the detach attach/refactor the situation is that
> > > onDetach is called only for components which were attached and
> > > detachModels is called for every component.
> > >
> > > While this kinda does make sense, there are two possible problems:
> > > - the semantics are not very well defined and when which method is
> > > called can be confusing
> > > - when there is model in component that is not in the default "slot"
> > > (get/setModel), it will not be detached properly unless the whole
> > > component was attached.
> > >
> > > The goal of attach/detach refactor was to simplify the things and I
> > > think we can go a little bit further. I propose merging detachModels
> > > with detach. And I also propose that onDetach should be called for
> > > every component. IMHO it would be safer and simpler than what we have
> > > right now.
> > >
> > > The onDetach sematics would be defined something like this:
> > > The method is called on the end of request, even if the component was
> > > not attached.
> > >
> > > The detach methods are usually rather simple, so I really don't think
> > > there would be a big performance penalty, as we already need to
> > > traverse the component tree.
> > >
> > > WDYT?
> > >
> > > -Matej
> > >
> >
>
Re: onDetach() contract, detachModels
Posted by Igor Vaynberg <ig...@gmail.com>.
same here
-igor
On 3/26/07, Martijn Dashorst <ma...@gmail.com> wrote:
>
> On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > and you clean myModel in onDetach, that won't work. I'm not sure how
> > rare this is and if you assign mymodel to a component it will be
> > detached anyway, but there is the possibility.
>
> In our application it is not rare :-)
>
> Martijn
>
> --
> Learn Wicket at ApacheCon Europe: http://apachecon.com
> Join the wicket community at irc.freenode.net: ##wicket
> Wicket 1.2.5 will keep your server alive. Download Wicket now!
> http://wicketframework.org
>
Re: onDetach() contract, detachModels
Posted by Martijn Dashorst <ma...@gmail.com>.
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> and you clean myModel in onDetach, that won't work. I'm not sure how
> rare this is and if you assign mymodel to a component it will be
> detached anyway, but there is the possibility.
In our application it is not rare :-)
Martijn
--
Learn Wicket at ApacheCon Europe: http://apachecon.com
Join the wicket community at irc.freenode.net: ##wicket
Wicket 1.2.5 will keep your server alive. Download Wicket now!
http://wicketframework.org
Re: onDetach() contract, detachModels
Posted by Matej Knopp <ma...@gmail.com>.
It only detachs the "default" component model, i.e. the one you set
with setModel. if you have something like this
class MyComponent ... {
IModel myModel = ...
}
and you clean myModel in onDetach, that won't work. I'm not sure how
rare this is and if you assign mymodel to a component it will be
detached anyway, but there is the possibility.
-Matej
On 3/26/07, Johan Compagner <jc...@gmail.com> wrote:
> as far as i see it does already what i expect it todo:
>
>
> if (getFlag(FLAG_ATTACHED))
> {
> // if the component has been previously attached via attach()
> // detach it now
> setFlag(FLAG_DETACHING, true);
> onDetach();
> if (getFlag(FLAG_DETACHING))
> {
> throw new IllegalStateException(Component.class.getName()
> + " has not been properly detached. Something in the
> hierarchy of "
> + getClass().getName()
> + " has not called super.onDetach() in the override
> of onDetach() method");
> }
> setFlag(FLAG_ATTACHED, false);
> }
>
> // always detach models because they can be attached without the
> // component. eg component has a compoundpropertymodel and one of
> its
> // children component's getmodelobject is called
> detachModels();
>
> // always detach children because components can be attached
> // independently of their parents
> detachChildren();
> }
>
> So only if attached the onDetached is called
> But detachModels and detachChildren are always called.
>
> isn't that correct? Where does it go wrong now?
>
> johan
>
>
>
>
> On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> >
> > Hi,
> >
> > i think we should finally define onDetach() contract and decide what
> > to do with detachModels().
> >
> > Right now, after the detach attach/refactor the situation is that
> > onDetach is called only for components which were attached and
> > detachModels is called for every component.
> >
> > While this kinda does make sense, there are two possible problems:
> > - the semantics are not very well defined and when which method is
> > called can be confusing
> > - when there is model in component that is not in the default "slot"
> > (get/setModel), it will not be detached properly unless the whole
> > component was attached.
> >
> > The goal of attach/detach refactor was to simplify the things and I
> > think we can go a little bit further. I propose merging detachModels
> > with detach. And I also propose that onDetach should be called for
> > every component. IMHO it would be safer and simpler than what we have
> > right now.
> >
> > The onDetach sematics would be defined something like this:
> > The method is called on the end of request, even if the component was
> > not attached.
> >
> > The detach methods are usually rather simple, so I really don't think
> > there would be a big performance penalty, as we already need to
> > traverse the component tree.
> >
> > WDYT?
> >
> > -Matej
> >
>
Re: onDetach() contract, detachModels
Posted by Johan Compagner <jc...@gmail.com>.
as far as i see it does already what i expect it todo:
if (getFlag(FLAG_ATTACHED))
{
// if the component has been previously attached via attach()
// detach it now
setFlag(FLAG_DETACHING, true);
onDetach();
if (getFlag(FLAG_DETACHING))
{
throw new IllegalStateException(Component.class.getName()
+ " has not been properly detached. Something in the
hierarchy of "
+ getClass().getName()
+ " has not called super.onDetach() in the override
of onDetach() method");
}
setFlag(FLAG_ATTACHED, false);
}
// always detach models because they can be attached without the
// component. eg component has a compoundpropertymodel and one of
its
// children component's getmodelobject is called
detachModels();
// always detach children because components can be attached
// independently of their parents
detachChildren();
}
So only if attached the onDetached is called
But detachModels and detachChildren are always called.
isn't that correct? Where does it go wrong now?
johan
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
>
> Hi,
>
> i think we should finally define onDetach() contract and decide what
> to do with detachModels().
>
> Right now, after the detach attach/refactor the situation is that
> onDetach is called only for components which were attached and
> detachModels is called for every component.
>
> While this kinda does make sense, there are two possible problems:
> - the semantics are not very well defined and when which method is
> called can be confusing
> - when there is model in component that is not in the default "slot"
> (get/setModel), it will not be detached properly unless the whole
> component was attached.
>
> The goal of attach/detach refactor was to simplify the things and I
> think we can go a little bit further. I propose merging detachModels
> with detach. And I also propose that onDetach should be called for
> every component. IMHO it would be safer and simpler than what we have
> right now.
>
> The onDetach sematics would be defined something like this:
> The method is called on the end of request, even if the component was
> not attached.
>
> The detach methods are usually rather simple, so I really don't think
> there would be a big performance penalty, as we already need to
> traverse the component tree.
>
> WDYT?
>
> -Matej
>
Re: onDetach() contract, detachModels
Posted by Matej Knopp <ma...@gmail.com>.
This is no longer true. All request targets that deal with page call
page.detach, and the page is detached before serialization, even
during ajax requests.
Trigerring detachment of serialization is imho not a very good idea,
as it wouldn't work for httpsessionstore. And right now, it's not even
necessary, as the request targets do that.
Also in 1.x, each component's detach should be called only once,
because the only place where page.detach is (should in worst case :))
called is RequestTarget.detach
-Matej
On 3/26/07, Martijn Dashorst <ma...@gmail.com> wrote:
> On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> > The onDetach sematics would be defined something like this:
> > The method is called on the end of request, even if the component was
> > not attached.
> > The detach methods are usually rather simple, so I really don't think
> > there would be a big performance penalty, as we already need to
> > traverse the component tree.
>
> If we can make it so that they are only called just once per request I
> think it would be an improvement.
>
> I think this is the right thing to do: the following case is quite typical:
>
> public MyPage extends Page {
> IModel someModel;
> /* ... */
> void onDetach() {
> someModel.detach();
> }
> }
>
> Now if some model is attached during an Ajax request, I remember that
> the ondetach of the page is not called (at least not some time ago),
> causing serialization errors.
>
> Hmm. Thinking of this... can't we trigger detachment on serialization?
> We already go through the component tree there...
>
> Martijn
>
> --
> Learn Wicket at ApacheCon Europe: http://apachecon.com
> Join the wicket community at irc.freenode.net: ##wicket
> Wicket 1.2.5 will keep your server alive. Download Wicket now!
> http://wicketframework.org
>
Re: onDetach() contract, detachModels
Posted by Martijn Dashorst <ma...@gmail.com>.
On 3/26/07, Matej Knopp <ma...@gmail.com> wrote:
> The onDetach sematics would be defined something like this:
> The method is called on the end of request, even if the component was
> not attached.
> The detach methods are usually rather simple, so I really don't think
> there would be a big performance penalty, as we already need to
> traverse the component tree.
If we can make it so that they are only called just once per request I
think it would be an improvement.
I think this is the right thing to do: the following case is quite typical:
public MyPage extends Page {
IModel someModel;
/* ... */
void onDetach() {
someModel.detach();
}
}
Now if some model is attached during an Ajax request, I remember that
the ondetach of the page is not called (at least not some time ago),
causing serialization errors.
Hmm. Thinking of this... can't we trigger detachment on serialization?
We already go through the component tree there...
Martijn
--
Learn Wicket at ApacheCon Europe: http://apachecon.com
Join the wicket community at irc.freenode.net: ##wicket
Wicket 1.2.5 will keep your server alive. Download Wicket now!
http://wicketframework.org