> -1
is it a veto or a vote against the proposal?
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
> > >>>
> > >>>
> > >>
> >
>
