Totally agree with Ernesto. François
> Le 30 mars 2017 à 09:04, Ernesto Reinaldo Barreiro <[email protected]> a > écrit : > > Hi, > > As a wicket user I would hate to have to ask myself if my model are > detachable or not. I work as freelance wicket consultant and I have got to > the opportunity to see quite a few wicket projects, most of them already > started/mature projects, and the hard time some developers have to figure > out how to properly use models. IMHO adding more complexity would not help > at all. > > On Thu, Mar 30, 2017 at 8:33 AM, Martin Grigorov <[email protected]> > wrote: > >> On Wed, Mar 29, 2017 at 11:03 PM, Pedro Santos <[email protected]> >> wrote: >> >>>> -1 >>> >>> is it a veto or a vote against the proposal? >>> >> >> It means "I don't like the proposal" >> >> Honestly I don't really know how vetoing works here at Apache... >> >> >>> >>> Pedro Santos >>> >>> On Wed, Mar 29, 2017 at 4:34 PM, Martin Grigorov <[email protected]> >>> wrote: >>> >>>> 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 < >>>> [email protected]> >>>> 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 <[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 >>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>> >>> >> > > > > -- > Regards - Ernesto Reinaldo Barreiro
