ReactiveCollection issues #76
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OnCollectionChanged and OnPropertyChanged now check areChangeNotificationsEnabled. cleared is now included in Changed since aboutToClear is also included in Changing. Removed the Rerset call which was always called when change notification were reenabled. Conflicts: ReactiveUI/ReactiveCollection.cs
OnCollectionChanged and OnPropertyChanged now check areChangeNotificationsEnabled. cleared is now included in Changed since aboutToClear is also included in Changing. Removed the Rerset call which was always called when change notification were reenabled. Conflicts: ReactiveUI/ReactiveCollection.cs
I think this is probably a good idea, though reimplementing all of ObservableCollection will be tons of fun, I'm sure :) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dear Paul,
We are currently using ReactiveUI in a WinRT project and we experienced some issues with ReactiveCollection bound to a Gridview. We experienced reloading of the items when clearing the collection which causes flickering and animations to restart.
Because of the issues we temporarily switched to using ObservableCollection which does not have this issue and I decided to look into the ReactiveCollection class to see what was causing our issues. This pull request is not necessarily meant to be included in ReactiveUI, but more to discuss some changes I made and to improve my understanding of the ReactiveCollection class.
Some of the things I noticed was the difference in notifications fired by the ObservableCollection class and the ReactiveCollection class. I'm not sure if these differences are intentionally or not. Also the code that should disable change notifications doesn't seem to work since all notifications were fired normally. I noticed there is actually also an issue at the issue tracker, which has something to do with change notifications. Also the reset notification was fired twice because the Reset() method is automatically called when change notifications are reenabled and almost everywhere in the code where this happens the Reset() method is called for a second time. Therefore I decided to remove the reset call when notifications are reenabled since the comments do not mention this effect.
Another thing I noticed was the code in the ClearItems method. The reason why base.ClearItems() is not called is not completely clear to me. My initial understanding of the while loop removing the items is to fire the ItemsRemoved observable. Is this correct since the suppressing of notifications would suggest this should not happen.
And how about the Changing and Changed observables? Their function is not entirely clear to me. They just seem to aggregate events already available via the other observables. I also noticed that Changed was not including the cleared observable while Changing was including the aboutToClear observable.
Since you are breaking backwards compatibility with RxUI v4, maybe this is a good time to also do some cleanup and fixing in this class. Have you ever considered not inheriting the ObservableCollection class? That would probably give you much better control of the change notifications.
I hope you can answer some of my questions and I'm also glad to help improving the ReactiveCollection class. Looking forward to hear what you think about it.
Regards,
Johan