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 > > > >>> > > > >>> > > > >> > > > > > >
