Hi Ernesto, this proposal aims to provide a semantically correct interface. Wicket core will remain being the one responsible to handle user's models lifecycle, as Emond correctly pointed this out:
> Storing state directly in components is > considered bad practise. The same is true for behaviors. Only very few > components need specific detach logic themselves. so you should be good here. While this better semantic will make the code easier to be understood by newcomers that weren't hardened by years of bad semantic, long time users will be forced to remove a bunch of empty e useless methods when migrating to Wicket 8. This undesired effect (force users to remove junk) has also a positive effect; it will get a code that is easier to understand, cleaner and by consequence more maintainable. Even without being forced to do so, we already choose at Apache [1] to remove all those empty detach methods and happily got ~25 less lines of bad code forced into the code base by years of bad semantic. 1 - https://github.com/apache/wicket/commit/e93eb0ae99c0c0257a15 11739512326ecf00cc73 Pedro Santos On Thu, Mar 30, 2017 at 4:04 AM, Ernesto Reinaldo Barreiro < reier...@gmail.com> wrote: > 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 <mgrigo...@apache.org> > wrote: > > > On Wed, Mar 29, 2017 at 11:03 PM, Pedro Santos <pedros...@gmail.com> > > 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 <mgrigo...@apache.org > > > > > 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 < > > > > 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 > > > > > >>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > > > > -- > Regards - Ernesto Reinaldo Barreiro >
