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

Reply via email to