+0 on reverting the change.
LDM worked fine for all those years, we don't need the change on 6.x.
Sven
On 25.11.2014 20:44, Martin Grigorov wrote:
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 <
martijn.dasho...@gmail.com> 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 <mgrigo...@apache.org>
wrote:
On Tue, Nov 25, 2014 at 2:01 AM, Martijn Dashorst <
martijn.dasho...@gmail.com> wrote:
On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <s...@meiers.net> 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
