Hi Emond,
> It is an integral part of the lifecycle of IModel
Most of the models in Wicket and Wicketstuff have no detach logic, hardly
an integral part.
> One could say that the sole purpose of the detach traversal is to detach
all models.
Wicket detaches all components in the page in pre-order and each component
is responsible to detach any detachable object used during the request, not
only models. Just to be clear, no changes being proposed here.
> Changing this part of the API will break the world. All
> implementations of IModel need to either implement IDetachable or
> remove detach().
This change is not being proposed to Wicket 7, to break the world is an
exaggeration.
> All calls to detach() need to be guarded with an
> instanceof check and a cast. You can be sure many users will be very
> upset with such a change.
They didn't get upset by adding a bunch of empty detach methods in their
models that weren't detachable. It will be trick to say how sad or upset
users will be. Also I explained in my reply to Sven how this instanceof can
be abstracted from the user.
> What is your usecase for IIAmNotActuallyDetachabe? Why not simply call
> detach and have it do nothing when nothing needs to be done?
If a logic inside the component depends on the model being detachable or
not, it wouldn't be able to determine it testing the current interface. No
specific usecase in mind.
Pedro Santos
On Wed, Mar 29, 2017 at 2:53 PM, Emond Papegaaij <emond.papega...@gmail.com>
wrote:
> Hi Pedro,
>
> I fail to see why it is a problem for IModel to be IDetachable. It is
> an integral part of the lifecycle of IModel. One could say that the
> sole purpose of the detach traversal is to detach all models. A model
> knows how to retrieve data, update data and to detach its internal
> representation from the backend for future request handling. A model
> that does not require special processing to detach, simply omits this
> method: it has a default, empty implementation. Calling this empty
> method is cheap and likely to cause problems.
>
> Changing this part of the API will break the world. All
> implementations of IModel need to either implement IDetachable or
> remove detach(). All calls to detach() need to be guarded with an
> instanceof check and a cast. You can be sure many users will be very
> upset with such a change.
>
> What is your usecase for IIAmNotActuallyDetachabe? Why not simply call
> detach and have it do nothing when nothing needs to be done?
>
> Best regards,
> Emond
>
> On Wed, Mar 29, 2017 at 6:12 PM, Pedro Santos <pedros...@gmail.com> wrote:
> > 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
> >>>
> >>>
> >>
>
