This is a migrated thread and some comments may be shown as answers.

RadObservableCollection.AddRange bug

6 Answers 222 Views
General Discussions
This is a migrated thread and some comments may be shown as answers.
Graeme
Top achievements
Rank 2
Graeme asked on 29 Jun 2015, 12:15 AM

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

 

6 Answers, 1 is accepted

Sort by
0
Ivan Ivanov
Telerik team
answered on 29 Jun 2015, 05:05 AM
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
Do you want to have your say when we set our development plans? Do you want to know when a feature you care about is added or when a bug fixed? Explore the Telerik Feedback Portal and vote to affect the priority of the items
0
Graeme
Top achievements
Rank 2
answered on 29 Jun 2015, 05:54 AM

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

0
Ivan Ivanov
Telerik team
answered on 29 Jun 2015, 06:11 AM
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
Do you want to have your say when we set our development plans? Do you want to know when a feature you care about is added or when a bug fixed? Explore the Telerik Feedback Portal and vote to affect the priority of the items
0
Graeme
Top achievements
Rank 2
answered on 29 Jun 2015, 06:26 AM

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

0
Ivan Ivanov
Telerik team
answered on 29 Jun 2015, 06:45 AM
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
Do you want to have your say when we set our development plans? Do you want to know when a feature you care about is added or when a bug fixed? Explore the Telerik Feedback Portal and vote to affect the priority of the items
0
Graeme
Top achievements
Rank 2
answered on 29 Jun 2015, 06:57 AM
Thanks Ivan. I hope that this thread helps others.
Tags
General Discussions
Asked by
Graeme
Top achievements
Rank 2
Answers by
Ivan Ivanov
Telerik team
Graeme
Top achievements
Rank 2
Share this question
or