Hi Pedro,
for Wicket your changes look fine.
But I have an objection from a user's perspective:
If I hold a reference to a model (in a component or another wrapping
model), I'll always have to check the instance and do a cast before
detaching it:
class MyComponent {
private IModel<Foo> foo;
@Override
public void detachModels() {
if (foo instanceof IDetachable) {
((IDetachable)foo).detach();
}
}
}
Sad :/.
Furthermore what about FeedbackMessage, ICellPopulator and
IDataProvider? Will their implementations no longer be forced to
implement IDetachable too?
IMHO your missing an important aspect of IDetachable: yes, many
implementations really don't need to detach anything. But many places
*have* to detach these objects, *iff* they are detachable.
The current solution - even if semantically wrong - is simpler.
Best regards
Sven
On 29.03.2017 15:52, Pedro Santos wrote:
Hi team,
Not every model is detachable, but right now all models extends from
IDetachable because this contract is forced to any implementation for
IModel.
The only way users have to communicate their model isn't detachable, for
their own purpose, is by creating their own mechanism; but I think
IDetachable interface should serve its purpose and to mark / be extended by
detachable models, like documented in its own javadoc.
I did the changes necessary for this improvement in the branch
'not-detachable' at [1], and hope you can give your input. While working on
the branch I noticed a few problems in Wicket's code and already added a
fix:
- LabeledWebMarkupContainer called wrapped model detach twice (fixed)
- SessionSizeModel has no detach logic but was being detached at
SessionSizeDebugPanel (fixed)
- The following classes had anonymous model wrappers that weren't extending
from IWrapModel, that would better communicates their purpose(fixed):
LambdaColumn, IModel, LambdaModel, AjaxEditableLabel, LambdaColumn
1 - https://github.com/pedrosans/wicket/tree/not-detachable
Pedro Santos
