Fix ActOnEveryObject missing changes when SuppressChangeNotifications is used #1685
Conversation
… suppressed Previously ActOnEveryObject would not handle list changes correctly if the changes were made while SuppressChangeNotifications() was used. With this commit, ActOnEveryObject works as expected. Fixes issue #1675. BREAKING CHANGE: protected virtual method publishResetNotification() is removed. It is not used anymore and current subclasses might be overriding it because it affects Reset() and SuppressChangeNotifications(), which it doesn't anymore .
|
|
| @@ -579,7 +580,10 @@ public IDisposable SuppressChangeNotifications() | |||
|
|
|||
| return new CompositeDisposable(this.suppressChangeNotifications(), Disposable.Create(() => { | |||
| if (Interlocked.Decrement(ref _resetNotificationCount) == 0) { | |||
| publishResetNotification(); | |||
| if (resetEventArgs == null) { | |||
There was a problem hiding this comment.
Potentially a threading issue here, since your dispose may not be called in the same thread as when it was created. Probably should use a Interlocked.Exchange or something?
There was a problem hiding this comment.
It might be alright given that it's inside the Interlocked.Decrement() though.
There was a problem hiding this comment.
As resetEventArgs is set (if it is set) before the disposable is returned, and nothing can change its value until the disposable is called, I don't really see how a race condition could occur? Duplicate/missing changed notifications cannot occur since Interlocked.Decrement is still used. Am I missing something?
There was a problem hiding this comment.
Yeah I agree, was just a initial reaction until I saw everything inside the Decrement statement.
| @@ -481,6 +481,64 @@ public void GetAResetWhenWeAddALotOfItems() | |||
| Assert.Equal(1, reset.Count); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void ActOnEveryObjectShouldHandleAddRemoveAndClear() | |||
There was a problem hiding this comment.
I think from a clarity point of view, this test is doing too much, I think each separate case of ActOnEvery… should be a separate test.
| } | ||
|
|
||
| [Fact] | ||
| public void ActOnEveryObjectShouldNotMissChangesUnderSuppressedNotifications() |
There was a problem hiding this comment.
Likewise, I think this would be clearer as a couple of tests
|
|
||
| if (Interlocked.Increment(ref _resetNotificationCount) == 1) { | ||
| // Changes were not suppressed before, publish a Changing reset notification. | ||
| resetEventArgs = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset); |
There was a problem hiding this comment.
Is there a reason to not use the reset/publishResetNotification in here? Wondering if we can reduce the breaking changes to a minimum.
There was a problem hiding this comment.
Note that with this patch only a Changed, but no Changing notification, is pushed in the disposable. Using publishResetNotification() in here would cause a duplicate Changing notification after the changes have been made, which would be unexpected but perhaps less breaking. I removed publishResetNotification() because it was used only in Reset() and SuppressChangeNotifications() and since it is no longer used there I feel like it would be better to have a compiler error rather than unexpected behaviour when overriding this method.
I guess it comes down to this: Do you prefer a duplicate, technically incorrect, Changing notification but no breaking changes? Or would you rather have the (small) breaking change and no duplicate notifications?
|
Only just spotted, can we change this PR to be against master rather than develop? |
|
That merge we definitely don't want. Now we got mass number of changes. You probably want to do some sort of rebase operation. Another option is close this pr, then you just cherry pick the commits into a new branch from master your changes and submit a new pr. |
|
I'll close this one, and open a cherry-picked new one later. Also, not sure if you always want your PRs against master, but if so you might want to update the guidelines. |
|
Good point. I will do a pr for that later today. It was something that changed with the RxUI 8 release. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
What is the current behavior? (You can also link to an open issue here)
Previously
ActOnEveryObject()would not handle list changes correctly if the changes were made whileSuppressChangeNotifications()was used. This is becauseSuppressChangeNotifications()currently publishes a resetChangingnotification after the changes have been made. See issue #1675.What is the new behavior (if this is a feature change)?
SuppressChangeNotifications()now publishes the resetChangingnotification before the changes are made, when the suppression is first enabled.What might this PR break?
Protected virtual method
publishResetNotification()is removed. It is not used in the class anymore and current subclasses might be overriding it because it affects Reset() and SuppressChangeNotifications(), which it doesn't anymore.Please check if the PR fulfills these requirements
Other information: