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 <[email protected]> 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 <[email protected]> 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 >>> >>> >>
