Bug in new SetItem method

Bug in new SetItem method

Old forum URL: forums.lhotka.net/forums/t/305.aspx


miker55 posted on Wednesday, June 07, 2006

I am getting an error when I try to assign to a business object.  To verify that it wasn't my code, I modified the ProjectTracker code like so:

In RolesEdit.cs

    private void SaveButton_Click(object sender, EventArgs e)
    {
      int lastRow = _roles.Count-1;
      _roles[lastRow] = _roles[lastRow];   // crashes!

      ....

   }

The problem is the SetItem method override.  Assuming a 5 item list, you get:

    protected override void SetItem(int index, C item)
    {
      RemoveItem(index);               // removes item 5 in 5 count list
      base.SetItem(index, item);      // tries to assign to item 5 in a now 4 count list, error
    }

It calls RemoveItem at position [index], then immediately tries to assign to the same index that now no longer exists.

As I missing something here?  In the MS help for overriding SetItem, all it does is call base.SetItem().  What is the purpose of the RemoveItem call?

TIA,

Mike Rodriguez

 

RockfordLhotka replied on Wednesday, June 07, 2006

Hmm, interesting. That's a good catch.

The RemoveItem() call is there to support undo. If you "set" an item it replaces the previous item in that location right? But if you later call CancelEdit(), then you'd expect that the new item would go away and the old item would come back. This means that the old item needs to have been moved to deletedList, not just discarded.

But what you have found is certainly an ugly timing issue. It appears that I can't just call RemoveItem() in this case, but will have to do some extra work...

Mark replied on Wednesday, June 07, 2006

How about something like this...

protected override void SetItem(int index, C item)
{
   //Only care about deleting if the objects (IDs) are different!
   C obj = this[index];
   if (!obj.Equals(item))
   {
      DeleteChild(obj);
      INotifyPropertyChanged c = c as INotifyPropertyChanged;
      if (c != null)
        c.PropertyChanged -= new PropertyChangedEventHandler(Child_PropertyChanged);
   }     
   base.SetItem(index, item);
}

This is basically executing the same code that's in RemoveItem - without the actual remove.  I'm assuming the base.SetItem() will overwrite whatever object is already there. 

I also think we need the check to see if the objects (or their IDs) are the same.  If I edit some object and try to push it back into the collection via a Set method (even though it's not necessary), without the check, the object would get marked for deletion as well as exist in the regular collection.  Who knows what kind of craziness would follow... :-)

RockfordLhotka replied on Wednesday, June 07, 2006

Interesting thought about not replacing the existing object. I think though, that checking the reference would be sufficient. Even if the objects are "logically" the same, I think you'd need to replace it if the actual reference is different. So
 
if (ReferenceEquals(obj, item))
 
is probably the better check?
 
Even if there's a good argument for doing a logical compare, the event handler for PropertyChanged needs to be removed, so some work must happen in any case that the reference is different.
 
Can you think of a valid scenario where you'd actually WANT to replace an object with a whole other object that happened to have the same Id?
 
Rocky

Mark replied on Wednesday, June 07, 2006

You're correct (as usual) - ReferenceEquals seems to be the proper check.  I can't think of a reason why we'd need to allow the SetItem to continue if the references are equal - not that it would be doing much, given the references are equal to begin with.  Just move the base.SetItem call into the conditional block and that'd take care of it.  :-)  Final code looks something like this?

protected override void SetItem(int index, C item)
{
   C obj = this[index];
   if (!ReferenceEquals(item, obj))
   {
      DeleteChild(obj);
      INotifyPropertyChanged c = c as INotifyPropertyChanged;
      if (c != null)
        c.PropertyChanged -= new PropertyChangedEventHandler(Child_PropertyChanged);
      base.SetItem(index, item);
   }     
}

jh72i replied on Thursday, February 21, 2008

Hi Mark, wondering about this question you asked some time back - i.e. why would there be a need to carry on with a replace/set if the object references are the same? Thinking that the a second event could be hooked up if the replace code goes ahead - 'child' is never unhooked but maybe item is hooked again in the base.SetItem??

protected override void SetItem(int index, C item)
{
C child =
default(C);
if (!(ReferenceEquals((C)(this[index]), item)))
child =
this[index];
// replace the original object with this new object
bool oldRaiseListChangedEvents = this.RaiseListChangedEvents;
try
{
this.RaiseListChangedEvents = false;
// set parent reference
item.SetParent(this);
// set child edit level
ResetChildEditLevel(item, this.EditLevel);
// reset EditLevelAdded
item.EditLevelAdded = this.EditLevel;
// add to list
base.SetItem(index, item);
}
finally
{
this.RaiseListChangedEvents = oldRaiseListChangedEvents;
}
if (child != null)
CopyToDeletedList(child);

if
(RaiseListChangedEvents)
OnListChanged(
new ListChangedEventArgs(ListChangedType.ItemChanged, index));
}

Copyright (c) Marimer LLC