Re: Proposal to remove IDetachable from IModel hierarchy

[Pedro Santos <pe...@gmail.com>]

> 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 <emond.papegaaij@topicus.nl
> 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
>

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Emond Papegaaij <em...@topicus.nl>]

The vote is closed with:
1x Yes, remove IDetachable from IModel
7x No, keep IModel detachable

So we can conclude that IModel will remain detachable.

Best regards,
Emond

On maandag 3 april 2017 09:38:03 CEST Emond Papegaaij wrote:
> Something went wrong sending this mail. I did write some more, but somehow
> my mail client lost it. So here's the vote again:
> 
> I think we are not going to agree on this proposal. I think it is not an
> improvement and I do not agree with you that IModel should not be
> detachable by default. So lets vote on this.
> 
> [ ] Yes, remove IDetachable from IModel
> [ ] No, keep IModel detachable
> 
> My vote:
> -1 keep IModel detachable
> 
> Best regards,
> Emond

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Emond Papegaaij <em...@topicus.nl>]

On dinsdag 4 april 2017 00:24:56 CEST Pedro Santos wrote:
> > TL;DR Vote at the bottom
> 
> What does it mean? That your email can be skipped to the voting part or
> that I was prolix in my last email?

I suspected the discussing was becoming is a bit lengthy for everyone to 
follow. So to make sure everyone read the most important part of the mail, I 
put that at the top. Unfortunately, my mail client decided to chop off that 
part :)

> > I think we are not going to agree on this proposal.
> 
> No problem. Having different opinions being discussed is just a sign of a
> healthy project.

Indeed it is, and it was a good discussion. But as with all discussions, it 
must come to an end at some time :)

Emond

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Michael Mosmann <mi...@mosmann.de>]

[X] No, keep IModel detachable.

.. i did once a refactoring to build an IReadonlyModel,and failed. I failed 
because of the limited feature set of the java language. So you will end up 
with some kind of compromise,which is not near a perfect solution.

Maybe there is a much better way to handle this leaky request/response 
abstraction... But IMHO removing IDetachable will not be a step in this 
direction.

Michael:)

Mit AquaMail Android
http://www.aqua-mail.com gesendet


Am 4. April 2017 5:25:05 vorm. schrieb Pedro Santos <pe...@gmail.com>:

> Hi
>
> Emond,
>
>> TL;DR Vote at the bottom
>
> What does it mean? That your email can be skipped to the voting part or
> that I was prolix in my last email?
>
>> I think we are not going to agree on this proposal.
>
> No problem. Having different opinions being discussed is just a sign of a
> healthy project.
>
> Carl,
>
> Indeed, and it's really nice to get you option on this. I also see this as
> a tradeoff situation.
>
> Martijn,
>
>> models only live during
>> actual request processing
>
> They live longer. They even implement IClusterable (IDetachable's
> superinterface) to do so. IClusterable being IDetachable's superinterface
> is a living paradox for me.
>
> [x] Yes, remove IDetachable from IModel
>
>
> Pedro Santos
>
> On Mon, Apr 3, 2017 at 10:49 AM, Martijn Dashorst <
> martijn.dashorst@gmail.com> wrote:
>
>> While I appreciate the effort in questioning our fundamentals and
>> trying to improve even the oldest parts of our API, I don't think that
>> the detach method is semantically wrong for models. Semantics are
>> defined by what we say the semantics are. In a request/response
>> oriented environment a detach is an essential part of the lifecycle of
>> a request in general, and for models in particular.
>>
>> Were Wicket a Swing framework, I would consider modifying the API, but
>> as Wicket lives in an environment where the models only live during
>> actual request processing, and are literally detached otherwise,
>> IModel implementations should have detach behavior, and therefore the
>> framework must guarantee that it can call the detach logic at
>> appropriate times. Therefore IModels *are* IDetachable.
>>
>> So I don't think we should remove IDetachable from IModel as it is an
>> essential, integral and semantically correct part of models.
>>
>> [X] No, keep IModel detachable.
>>
>> Martijn
>>
>>
>> On Mon, Apr 3, 2017 at 9:38 AM, Emond Papegaaij
>> <em...@topicus.nl> wrote:
>> > Something went wrong sending this mail. I did write some more, but
>> somehow my
>> > mail client lost it. So here's the vote again:
>> >
>> > I think we are not going to agree on this proposal. I think it is not an
>> > improvement and I do not agree with you that IModel should not be
>> > detachable by default. So lets vote on this.
>> >
>> > [ ] Yes, remove IDetachable from IModel
>> > [ ] No, keep IModel detachable
>> >
>> > My vote:
>> > -1 keep IModel detachable
>> >
>> > Best regards,
>> > Emond
>> >
>> >
>>
>>
>>
>> --
>> Become a Wicket expert, learn from the best: http://wicketinaction.com
>>

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Pedro Santos <pe...@gmail.com>]

Hi

Emond,

> TL;DR Vote at the bottom

What does it mean? That your email can be skipped to the voting part or
that I was prolix in my last email?

> I think we are not going to agree on this proposal.

No problem. Having different opinions being discussed is just a sign of a
healthy project.

Carl,

Indeed, and it's really nice to get you option on this. I also see this as
a tradeoff situation.

Martijn,

> models only live during
> actual request processing

They live longer. They even implement IClusterable (IDetachable's
superinterface) to do so. IClusterable being IDetachable's superinterface
is a living paradox for me.

[x] Yes, remove IDetachable from IModel


Pedro Santos

On Mon, Apr 3, 2017 at 10:49 AM, Martijn Dashorst <
martijn.dashorst@gmail.com> wrote:

> While I appreciate the effort in questioning our fundamentals and
> trying to improve even the oldest parts of our API, I don't think that
> the detach method is semantically wrong for models. Semantics are
> defined by what we say the semantics are. In a request/response
> oriented environment a detach is an essential part of the lifecycle of
> a request in general, and for models in particular.
>
> Were Wicket a Swing framework, I would consider modifying the API, but
> as Wicket lives in an environment where the models only live during
> actual request processing, and are literally detached otherwise,
> IModel implementations should have detach behavior, and therefore the
> framework must guarantee that it can call the detach logic at
> appropriate times. Therefore IModels *are* IDetachable.
>
> So I don't think we should remove IDetachable from IModel as it is an
> essential, integral and semantically correct part of models.
>
> [X] No, keep IModel detachable.
>
> Martijn
>
>
> On Mon, Apr 3, 2017 at 9:38 AM, Emond Papegaaij
> <em...@topicus.nl> wrote:
> > Something went wrong sending this mail. I did write some more, but
> somehow my
> > mail client lost it. So here's the vote again:
> >
> > I think we are not going to agree on this proposal. I think it is not an
> > improvement and I do not agree with you that IModel should not be
> > detachable by default. So lets vote on this.
> >
> > [ ] Yes, remove IDetachable from IModel
> > [ ] No, keep IModel detachable
> >
> > My vote:
> > -1 keep IModel detachable
> >
> > Best regards,
> > Emond
> >
> >
>
>
>
> --
> Become a Wicket expert, learn from the best: http://wicketinaction.com
>

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Martijn Dashorst <ma...@gmail.com>]

While I appreciate the effort in questioning our fundamentals and
trying to improve even the oldest parts of our API, I don't think that
the detach method is semantically wrong for models. Semantics are
defined by what we say the semantics are. In a request/response
oriented environment a detach is an essential part of the lifecycle of
a request in general, and for models in particular.

Were Wicket a Swing framework, I would consider modifying the API, but
as Wicket lives in an environment where the models only live during
actual request processing, and are literally detached otherwise,
IModel implementations should have detach behavior, and therefore the
framework must guarantee that it can call the detach logic at
appropriate times. Therefore IModels *are* IDetachable.

So I don't think we should remove IDetachable from IModel as it is an
essential, integral and semantically correct part of models.

[X] No, keep IModel detachable.

Martijn


On Mon, Apr 3, 2017 at 9:38 AM, Emond Papegaaij
<em...@topicus.nl> wrote:
> Something went wrong sending this mail. I did write some more, but somehow my
> mail client lost it. So here's the vote again:
>
> I think we are not going to agree on this proposal. I think it is not an
> improvement and I do not agree with you that IModel should not be
> detachable by default. So lets vote on this.
>
> [ ] Yes, remove IDetachable from IModel
> [ ] No, keep IModel detachable
>
> My vote:
> -1 keep IModel detachable
>
> Best regards,
> Emond
>
>



-- 
Become a Wicket expert, learn from the best: http://wicketinaction.com

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Sebastien <se...@gmail.com>]

[x] No, keep IModel detachable

Even I understand Pedro's concern, Emond raised one of the most important
point here: detaching is entirely part of the model life-cycle.

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Martin Grigorov <mg...@apache.org>]

[ X ] No, keep IModel detachable

Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov

On Mon, Apr 3, 2017 at 9:38 AM, Emond Papegaaij <em...@topicus.nl>
wrote:

> Something went wrong sending this mail. I did write some more, but somehow
> my
> mail client lost it. So here's the vote again:
>
> I think we are not going to agree on this proposal. I think it is not an
> improvement and I do not agree with you that IModel should not be
> detachable by default. So lets vote on this.
>
> [ ] Yes, remove IDetachable from IModel
> [ ] No, keep IModel detachable
>
> My vote:
> -1 keep IModel detachable
>
> Best regards,
> Emond
>
>
>

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Carl-Eric Menzel <cm...@wicketbuch.de>]

[X] No, keep IModel detachable

I can see Pedro's point. A Model really is only something that can get
and set. But detaching is such an important part of the lifecycle of
many Wicket things (including Models) that I think it is an acceptable
tradeoff in clarity to have IModel extend IDetachable.

Carl-Eric

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Sven Meier <sv...@meiers.net>]

[-1] No, keep IModel detachable

Sven


On 03.04.2017 09:38, Emond Papegaaij wrote:
> Something went wrong sending this mail. I did write some more, but somehow my
> mail client lost it. So here's the vote again:
>
> I think we are not going to agree on this proposal. I think it is not an
> improvement and I do not agree with you that IModel should not be
> detachable by default. So lets vote on this.
>
> [ ] Yes, remove IDetachable from IModel
>
>
> My vote:
> -1 keep IModel detachable
>
> Best regards,
> Emond
>
>

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Emond Papegaaij <em...@topicus.nl>]

Something went wrong sending this mail. I did write some more, but somehow my 
mail client lost it. So here's the vote again:

I think we are not going to agree on this proposal. I think it is not an
improvement and I do not agree with you that IModel should not be
detachable by default. So lets vote on this.

[ ] Yes, remove IDetachable from IModel
[ ] No, keep IModel detachable

My vote:
-1 keep IModel detachable

Best regards,
Emond

Re: [VOTE] Proposal to remove IDetachable from IModel hierarchy

[Martin Grigorov <mg...@apache.org>]

On Mon, Apr 3, 2017 at 9:20 AM, Emond Papegaaij <em...@topicus.nl>
wrote:

> TL;DR Vote at the bottom
>
> On zondag 2 april 2017 00:25:12 CEST Pedro Santos wrote:
> > > 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.
>
> I do not agree here, as I see detach as an integral part of the IModel
> lifecycle. But I don't
> think we are going to agree on that.
>
> > > 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.
>
> Types in Java can always be polymorphic (except when they are final). A
> well written
> component does not depend on the specifics of its models. Therefore a
> component should
> be built on IModels *and* detach these model. That's the only way it will
> work for all types
> of models.
>
> > 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.
>
> What I mean here is that you do not have to write any code.
>
> > > 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.
>
> What I mean here is that the author of a model that requires detach() now
> also has to add
> 'implements IDetachable'. This was not needed before, because it was
> inherited from
> IModel.
>
> I think we are not going to agree on this proposal. I think it is not an
> improvement and I do
> not agree with you that IModel should not be detachable by default. So
> lets vote on this.
>
> [ ] Yes, remove IDetachable from IModel



You don't give us any options ;-)

[VOTE] Proposal to remove IDetachable from IModel hierarchy

[Emond Papegaaij <em...@topicus.nl>]

TL;DR Vote at the bottom

On zondag 2 april 2017 00:25:12 CEST Pedro Santos wrote:
> > 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.

I do not agree here, as I see detach as an integral part of the IModel lifecycle. But I don't 
think we are going to agree on that.

> > 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.

Types in Java can always be polymorphic (except when they are final). A well written 
component does not depend on the specifics of its models. Therefore a component should 
be built on IModels *and* detach these model. That's the only way it will work for all types 
of models.

> 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.

What I mean here is that you do not have to write any code.

> > 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.

What I mean here is that the author of a model that requires detach() now also has to add 
'implements IDetachable'. This was not needed before, because it was inherited from 
IModel.

I think we are not going to agree on this proposal. I think it is not an improvement and I do 
not agree with you that IModel should not be detachable by default. So lets vote on this.

[ ] Yes, remove IDetachable from IModel