On woensdag 29 maart 2017 17:02:05 CEST Pedro Santos wrote: > Hi Emond, > > > It is an integral part of the lifecycle of IModel > > Most of the models in Wicket and Wicketstuff have no detach logic, hardly > an integral part.
Just because not all implementations need an implementation of detach, does not make it less significant. Also, most of the models I use in our applications do need detach logic: PropertyModel, StringResourceModel, hundreds of implementations of LoadableDetachableModel and many more. Only some very simple AbstractReadOnlyModels and some usecases for Model do not need any detachment. You can compare IModel.detach with InputStream.close: not all implementations actually need it, but it is very significant. In fact, it is so significant that special tooling and language features are used to enforce it is always called. For IModel we also use special tooling to check that all models are actually detached. Removing detach from IModel will most certainly cause models to not be detached, causing errors in our applications. > > One could say that the sole purpose of the detach traversal is to detach > > all models. > > Wicket detaches all components in the page in pre-order and each component > is responsible to detach any detachable object used during the request, not > only models. Just to be clear, no changes being proposed here. Why does wicket detach all components? All components are detached to give them a change to detach their models. Storing state directly in components is considered bad practise. The same is true for behaviors. Only very few components need specific detach logic themselves. > > Changing this part of the API will break the world. All > > implementations of IModel need to either implement IDetachable or > > remove detach(). > > This change is not being proposed to Wicket 7, to break the world is an > exaggeration. The change cannot even be made on Wicket 7 due to semver. I call a change in the API that introduces thousands of compile errors in my workspace a 'break the world' change. I cannot even begin to count the number of places where we call or implement detach() on IModel. All those places will break and require manual fixing. > > 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. > > They didn't get upset by adding a bunch of empty detach methods in their > models that weren't detachable. It will be trick to say how sad or upset > users will be. Also I explained in my reply to Sven how this instanceof can > be abstracted from the user. With Wicket 8 users no longer need to implement an empty detach method. This has already been solved. I can assure you that my collegues and I will be very upset by a change like this. > > What is your usecase for IIAmNotActuallyDetachabe? Why not simply call > > detach and have it do nothing when nothing needs to be done? > > If a logic inside the component depends on the model being detachable or > not, it wouldn't be able to determine it testing the current interface. No > specific usecase in mind. I cannot see how calling an empty method on a model might cause any problems. I don't think a usecase for this exists. Best regards, Emond
