> It's difficult to get exact numbers, but here are some: > 48137 java files, 74565 class files, not all wicket related, that's > the entire codebase I'm working on
thanks for sharing > 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. > 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. > 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)? Pedro Santos On Thu, Mar 30, 2017 at 3:55 PM, Emond Papegaaij <emond.papega...@gmail.com> wrote: > It's difficult to get exact numbers, but here are some: > 48137 java files, 74565 class files, not all wicket related, that's > the entire codebase I'm working on > 1674 calls to IDetachable.detach() from our codebase, most for models > 1338 implementations of detach(), I don't know how many come from > IModel implementations and I don't know how many are empty. > > As you can see, this change will probably cause between 1000 and 2000 > errors in my workspace. > > On Thu, Mar 30, 2017 at 8:08 PM, Pedro Santos <pedros...@gmail.com> wrote: > > Emond, > > > > The numbers from wicketstuff: > > > > 2957 classes in total > > 71 empty detaches causing compilation errors > > 34 in wicket-fondation > > 2 detache methods that werent a dummy empty methods; on in a test case in > > LazyModelTest; one in the FutureModel in wicketstuff-minis project > > > > The project I'm currently working: > > > > 331 classes in total > > 2 empty detaches causing compilation errors > > 0 detach logic in an IModel implementation > > > > Wicket-examples: > > > > 391 classes in total > > ~5 empty detaches that would cause compilation error (from my search in > the > > logs [1]) > > > > I don't mind to analyse another project using Wicket if it's available in > > GitHub of some where public if you think it will give me more insights. > > Models just implementing IModel are rarely detachable. > > > > if you don't share any information about your code base, it will be hard > to > > understand the dimension of your problem. > > > > 1 - https://github.com/apache/wicket/commit/ > e93eb0ae99c0c0257a151173951232 > > 6ecf00cc73 > > > > Pedro Santos > > > > On Thu, Mar 30, 2017 at 4:09 AM, Francois Meillet < > > francois.meil...@gmail.com> wrote: > > > >> Totally agree with Ernesto. > >> > >> François > >> > >> > >> > >> > Le 30 mars 2017 à 09:04, Ernesto Reinaldo Barreiro < > reier...@gmail.com> > >> a écrit : > >> > > >> > 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 > >> > >> >
