On donderdag 30 maart 2017 17:49:40 CEST Pedro Santos wrote: > > 1674 calls to IDetachable.detach() from our codebase, most for models > > hard to conclude anything from this number, because this proposal didn't > change the most commonly used models abstractions: > LoadableDetachableModel, GenericBaseModel, Model; neither changed the > interface IWrapModel. They all remain being, as they were meant to be, > detachable.
By far, the most common case is an instance field in a page or component of type IModel with a call to detach in detachModels() or onDetach(). Being typed IModel, these will all cause compile errors. > > 1338 implementations of detach(), I don't know how many come from > > IModel implementations and I don't know how many are empty. > > that would be an amazing info. Can't you take a sample of 10 randomly > IModel implementations and tell how much of them actually have detach > logic? As I reported before, this number is surprisingly low in > wicket-examples and wicketstuff. Most of our models have detach logic. However, we do not have that many custom model implementations. Most of these detach implementations are custom types implementing IDetachable and those methods are not empty either. > > As you can see, this change will probably cause between 1000 and 2000 > > errors in my workspace. > > Yes, but you have a codebase of 48137 java files! Were you truly expecting > an easy migration with a codebase that big? And expecting it at the expense > of a wrong interface hierarchy for the rest of Wicket developers, > including new comers that shouldn't be having to understand a so tangled > and meaningless set of interfaces (IModel, as Sven pointed, isn't the only > semantically wrong interface, and there's even a ticket proposing more > errors - WICKET-6347)? Yes, I do expect easy migration. So far, the migration from 7 to 8 only introduced about 500 errors, most of them caused the change in IRequestListener, AjaxFallbackLink and some pre-generic code regarding Link. This change alone will introduce 2 to 4 times that number of errors. The major concern I have with this change is that it does not improve anything. This change has impact on both the calling and implementing side of detach. Neither side becomes easier. The caller (i.e. component) still always has to call detach on all models (now guared by an instanceof), because it can never be sure that the actual implementation will or will not be detachable. For example, if the type is MyCustomModel (which does not implement IDetachable), a subclass of this type might implement IDetachable. Also, the implementation of MyCustomModel might change in the future. This problem is already visible in your branch with models like PropertyModel, GenericBaseModel en Model. Why are these IDetachable? Because the content might be (but often is not). On the side of the implementation for new models not much changes. You don't need to implement detach() when you don't need it. If you do need it, you now need 2 changes: implement IDetachable *and* implement detach(). For all existing IModel implementations you do need to make a change: either implement IDetachable or remove detach(). Your change violates the Liskov substitution principle. A component should be able to handle all types of IModel in the same way, regardless of its implementation. The fact that you now need an instanceof check is an indication of this violation. Best regards, Emond
