> 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.
So you are good here, all those detachable model specializations will remain being detachable (this proposal touches IModel interface only, not its subtypes that were meant to be detachable). Just specialize the variables types you said were typed as IModel. Wicket's core also has such variables typed as IModel and I had no problem invoking the detach method [1]. > 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. It does improve the IModel interface hierarchy by removing an unnecessary superinterface, and by fixing the code semantic by removing an empty method that wasn't default, but was written as one. > 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. No, the caller needs to call detach if the model is detachable only. > 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. You are clearly generalizing a very specific use case, you can't say "still always has to call detach". In this specific use case, where you have a polymorphic type, yes you do have to check if the instance is detachable. This occurs in Wicket's core because it's the place where generic model wrappers abstractions are defined and provided for the final user. If Wicket's core abstractions do their job right, this complexity should be taken away from the final user. Even by having to check for the model implementation, there's no need to guard the detach call inside an instanceof. You can just delegate this job to a helper class, or to bind the model to a component. > 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). What do those models have in common? They are all wrappers to other models. If we pass through this proposal, we will be able to go ahead and propose to remove their IDetachable interface; that was wrongfully ( and unthoughtful because it was forced via IModel ) add as their superinterface. > 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. No, you *do need*, and you *already implemented* it by choosing to implement IModel. You clearly mean here that you successfully hide from the user an empty method, written with a semantically wrong word, enforced by a contract that shouldn't be there in the first place. It's just trickery, not a good interface not enforcing an unneeded method. > If you do need it, you now > need 2 changes: implement IDetachable *and* implement detach(). No, a detachable model *always needed* to implement IDetacheble interface, no changes here. > For all > existing IModel implementations you do need to make a change: either implement > IDetachable or remove detach(). Most of Wicket's detachable model abstractions already add the IDetachable contract in the hierarchy. Yes, you do need to remove the detach method from models that aren't detachable. Wicket's core developers are already doing it just to have a clear code. > 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. This proposal can't possibly be violating the substitution principle, because of one reason: it removes a superinteface, not adds one that could not be substituted by its subtype. But the person who added the IDetachable interface to IModel, did violate this principle, and the need of the IIAmNotActuallyDetachabe interface to test if the model is actually detachable is a clear indication of this violation. 1 - https://github.com/pedrosans/wicket/commit/ca818adcc1df0d8 aae7f65be8ee09777325cf9f1#diff-6032b412b69768c08b1fe4a7b099d574R49 Pedro Santos On Fri, Mar 31, 2017 at 5:29 AM, Emond Papegaaij <[email protected] > wrote: > 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 >
