RadObservableCollection.AddRange bug

7 posts, 0 answers
  1. Graeme
    Graeme avatar
    21 posts
    Member since:
    May 2012

    Posted 28 Jun 2015 Link to this post

    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

     

  2. Ivan Ivanov
    Admin
    Ivan Ivanov avatar
    1127 posts

    Posted 29 Jun 2015 Link to this post

    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
  3. UI for WPF is Visual Studio 2017 Ready
  4. Graeme
    Graeme avatar
    21 posts
    Member since:
    May 2012

    Posted 29 Jun 2015 in reply to Ivan Ivanov Link to this post

    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

  5. Ivan Ivanov
    Admin
    Ivan Ivanov avatar
    1127 posts

    Posted 29 Jun 2015 Link to this post

    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
  6. Graeme
    Graeme avatar
    21 posts
    Member since:
    May 2012

    Posted 29 Jun 2015 in reply to Ivan Ivanov Link to this post

    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

  7. Ivan Ivanov
    Admin
    Ivan Ivanov avatar
    1127 posts

    Posted 29 Jun 2015 Link to this post

    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
  8. Graeme
    Graeme avatar
    21 posts
    Member since:
    May 2012

    Posted 29 Jun 2015 in reply to Graeme Link to this post

    Thanks Ivan. I hope that this thread helps others.
Back to Top
UI for WPF is Visual Studio 2017 Ready