Fully agree with Emond!
As a user I will hate such kind of change.
Several years ago this change might be OKish, but now with Java 8 default
methods it would be insane to break everyone's code when default methods
make it invisible.
Until 7.x I'd have to add empty impl for detach() when I don't need it.
With Wicket 8.x I won't bother at all!
Is your branch complete ? I see no changes in wicket-examples. Maybe it is
because several months ago I've cleaned all empty impls of #detach(). I did
it because I did it. It was compiling even without removing them. But with
your changes everyone will have to go all over his/her code and fix it!
If you remember the days when people migrated from 1.4.x to 1.5.x, or from
1.5.x to 6.x - many users sent a lot of negative feedback for such kind of
changes. Changes without deprecation cycle and changes without real benefit
from user point of view.
-1
On Wed, Mar 29, 2017 at 7: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
> >>>
> >>>
> >>
>
