Bug in CompositeContainer?

Topics: CAB & Smart Client Software Factory
Jul 7, 2005 at 7:22 AM
originally posted by: idlsoft2

I'm looking at the CTP, the dispose method lines 62..71:

if (theItem is IDisposable)
{
((IDisposable)theItem).Dispose();
if (theItem is IComponent)
{
IComponent comp = (IComponent)theItem;
comp.Site = null;
}

}

Shouldn't the order be reverse? Set site to null and then dispose? Otherwise OnSiteChanged will be called after Dispose.
Jul 7, 2005 at 10:58 AM
originally posted by: dcazzulino

The reasoning was that some items need access to services when they are being disposed, for example to unregister themselves from a catalog. If we set the site to null before disposing, that logic is not possible.

I thought about putting such code in the OnSiteChanged by checking for null on the new site, but then you have already lost the ability to query for services. :( ... this sounded like an acceptable compromise. However, I'm thinking that maybe we should avoid calling the OnSiteChanged method altogether in this case... something like:

if (comp is SitedComponent)
{
((SitedComponent)comp).CallOnSiteChanged = false;
}
// set to null...
Jul 7, 2005 at 12:33 PM
originally posted by: idlsoft2

If anything needs to be done while attaching/detaching from the site the "Site" setter can take care of it.
If the old value is null and the new one isn't - register, if it's the other way around - deregister. Since at this point you have both values I can still query for services.
The constructor/dispose should assume that the component is not sited.
I think this is consitent and easier to follow.
Jul 7, 2005 at 1:01 PM
originally posted by: dcazzulino

Precisely, the point is that at the OnSiteChanged method, you DONT have the old value. It's already changed. That means that you would have to override the setter instead, creating a slightly different implementation that nevertheless will be shared between both, as OnSiteChanged with null (not disposing, but being removed) would have the same behavior....

But... Site::set is not virtual in this release. Should it be? And if it is, does it make sense at all to keep the OnSiteChanged method? Or should we keep the later but pass the old and new values? (so that you can still query services if the new one is null).... and in that case, we would call it before disposing...
Jul 8, 2005 at 2:52 AM
originally posted by: idlsoft2

I think OnSiteChange(ISite oldValue, ISite newValue) would definitely be an improvement... but still not good enough.
The problem is that OnSiteChange even with the new signature cannot replace having a virtual Site property. It's not clear if it means OnAfterChange or OnBeforeChange and depending on that SitedComponent.GetService will behave differently within this call.
If for whatever reason you don't like Site being a virtual property I think a better option is to have two methods: OnSite and OnUnsite with no parameters with the assumption that Site is not null.
Something like:
Site { set {
if (site==value) return;
if (site!=null) OnUnsite(); // this happens regardless if the new site is null or not
site = value;
if (value!=null) OnSite();
}}
Jul 8, 2005 at 2:58 AM
originally posted by: dcazzulino

Well, if you provide OnUnsite without arguments, you've lost the previous site to unregister yourself :S...

Maybe it's better to just make the Site virtual and forget about the OnSited? :|
Jul 8, 2005 at 4:14 AM
originally posted by: idlsoft2

OnUnsite would be called before the old value changes so you would have the previous site.
Making the propery virtual and forgetting about the On... methods would do the job too.
Jul 8, 2005 at 4:44 AM
originally posted by: dcazzulino

Sounds more like OnUnSiting...

Ok, we'll think about the options.