>From user's perspective this is a symmetrical problem. This sadness is
carried over to an user in the other side, in need to avoid any detach
logic or processing in his component or model if the target model isn't
detachable. Right know he would have to come up with his own marker e.g.:
IIAmNotActuallyDetachabe interface. Given the symmetry, I would choose the
side that is not semantically wrong.
Also this instanceof test is more common for us, Wicket's core developers.
For the final user the AbstractWrapModel class and a helper function (I can
propose one) should hide this complexity. We can't judge how sad it's for
the final user base on how common it's for *us* to have this instanceof
check, and without even try to abstract this complexity.
Yes, I think this fix should be carried over to FeedbackMessage,
ICellPopulator and IDataProvider.
>The current solution - even if semantically wrong - is simpler.
It's simpler only because of the instanceof test that should be abstracted
from the user?
Pedro Santos
On Wed, Mar 29, 2017 at 11:48 AM, Sven Meier <s...@meiers.net> wrote:
> 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
>>
>>
>
