RadObservableCollection.AddRange bug

[Graeme]

Hi Guys,

 

I'm doing a lot of work with data in multiple threads and the RadObservableCollection is a great little gem.

 

However I have discovered an a small bug in AddRange method of the RadObservableCollection that has major implications. The AddRange method does not honour the manually set SuspendNotifications state and calls ResumeNotifications inside the method without first checking if already suspended. Any UI thread bindings monitoring for updates hit a fatal error condition.

 

As a  paid subscriber I can change the code manually but this would mean I would have to manually patch every time when new releases are made available. Would it be possible to fix this oversight?

 

Thanks,

 

Graeme

 

[Ivan Ivanov]

Hello Graeme,

Thank you for the kind words for our little collection. I am posting the current implementation of AddRange here, so that you can confirm the changes that you would like to apply:

public virtual void AddRange(IEnumerable<T> items)
        {
            if (items == null)
            {
                throw new ArgumentNullException("items");
            }

            this.SuspendNotifications();

            foreach (T item in items)
            {
                this.Add(item);
            }

            this.ResumeNotifications();
        }

You want to avoid calling ResumeNotifications in the method, in case that the notifications were manually suspended before invoking the method, relying that you will resume them manually later?

Regards,
Ivan Ivanov
Telerik

[Graeme]

Hi Ivan,

 

Thanks for responding so quickly. I refrained from posting Telerik source code in the public forums due to IP restrictions.

 

As you can see, the last line calls this.ResumeNotifications(); without checking if the user had already requested SuspendNotifications() on the collection.

 

Can a check be added and the SuspendNotifications/ResumeNotifications not be called if the collection is already in a SuspendNotifications state. My implementation is stores current state in a bool & only calls SuspendNotifications/ResumeNotifications if not true.

bool alreadyNotificationsSuspended = this.notificationsSuspended;

if (!alreadyNotificationsSuspended) this.SuspendNotifications();

// do work here...

if (!alreadyNotificationsSuspended) this.ResumeNotifications();

 

This would fix any cross-thread issues and stop fatal exceptions from being thrown.

Thanks,

 

Graeme

[Ivan Ivanov]

Hi Graeme,

I can clearly see your point and I would change it without any hesitation, if it was the first version of this collection. However, changing it in the way that you suggested may open a little behavioral trap with other clients' legacy code. Intentionally, or not some code may rely on the fact that calling AddRange resumes the notifications, even if they were initially suspended, which will not be true with the new version. While I agree with you on this one, we are often made to abstain from actual improvements, in order to keep our API consistant with the former versions.
As an alternatiove approach, would inheriting from RadObservableCollection<T> be a valid option for you? I believe that all of the members that are needed to modify the behavior are properly accessible.

Regards,
Ivan Ivanov
Telerik

[Graeme]

Hi Ivan,

 I hear what you are saying and understand. It wasn't until I looked at the Telerik source code before I realize why. As app development is moving more towards multi-threaded development and asynchronous programming (async ... await), this will become more of a headache for others.

 I'll implement what you are suggesting moving forward.

 

Thanks,

 Graeme

[Ivan Ivanov]

Hi,

Thanks for your understanding. In case a modification for this behavior gets future demand, we will consider introducing an alternative API for it, without compromising the previous behavior. Thank you for bringing it to our attention. I added 1000 Telerik points to your account, in accordance to your report.

Regards,
Ivan Ivanov
Telerik

[Graeme]

Thanks Ivan. I hope that this thread helps others.