Then let's revert the change and add javadoc explaining that if load() fails and you want to re-try then you should manually detach the model before that ?!
Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Tue, Nov 25, 2014 at 1:34 PM, Martijn Dashorst < [email protected]> wrote: > A common thing to do is to add more models to one model: > > class FooModel extends LDM<Foo> { > private IModel<Bar> barModel; > @Inject private FooService fooService; > > public FooModel(IModel<Bar> barModel) { > this.barModel = barModel; > NonContextual.of(FooModel.class).get().inject(this); > } > > @Override public Foo load() { > SomeValue value = barModel.getObject().getSomeValue(); > return fooService.findBySomeValue(value); > } > @Override public void onDetach() { > barModel.detach(); > } > } > > Attachment/detachment of both FooModel and BarModel are handled by the > FooModel. > > The issue is that if FooModel doesn't get detached anymore, BarModel > will remain attached as well. > > Martijn > > > > On Tue, Nov 25, 2014 at 10:01 AM, Martin Grigorov <[email protected]> > wrote: > > On Tue, Nov 25, 2014 at 2:01 AM, Martijn Dashorst < > > [email protected]> wrote: > > > >> On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <[email protected]> wrote: > >> > Hi Martin, > >> > > >> >> isAttached() is a public method and it is valid to call inside load() > >> > > >> > that's a valid point to consider: But what purpose could it have to > call > >> > isAttached() in load()? > >> > >> Probably none, I'm playing devil's advocate here. It is one model we > >> promote heavily to our users so I imagine it being quite popular. What > >> could break if we move the attached = true; and people rely on it to > >> be true at the beginning of load()? > >> > > > > I also don't see a use case for relying on attached being true during > > load(). > > > > > >> > >> But load() is probably not the problem here... detach is... > >> > >> > >> /** > >> * @see org.apache.wicket.model.IDetachable#detach() > >> */ > >> @Override > >> public void detach() > >> { > >> if (attached) > >> { > >> try > >> { > >> onDetach(); > >> } > >> finally > >> { > >> attached = false; > >> transientModelObject = null; > >> > >> log.debug("removed transient object for {}, requestCycle > {}", > >> this, > >> RequestCycle.get()); > >> } > >> } > >> } > >> > >> now that attached = true comes after the call to load, the model is no > >> longer attached. This means that any state built up during load() will > >> no longer be detached (removed) when something goes awry during > >> load(). > >> > > > > Usually models are used to get the underlying model object, i.e. > > myLDM.getObject(). > > So I don't expect an application to do something like: > > MyLDM myLDM = .... > > SomethingElse sthgElse = myLDM.getSomethingElse(); > > > > If sthgElse depends on load() then the only way to guarantee it is > properly > > initialized is to use #isAttached() in #getSomethingElse(). > > With the new improvement this should be even better than before. > > > > We can add explanation to load()'s javadoc that the application should > > clean up anything custom stored in the LDM itself > > > > > >> > >> Martijn > >> > > > > -- > Become a Wicket expert, learn from the best: http://wicketinaction.com >
