Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a INotifyCollectionChanged adapter around RealmResults #547

Merged
merged 3 commits into from Jun 2, 2016

Conversation

fealebenpae
Copy link
Member

No description provided.

@fealebenpae fealebenpae self-assigned this May 17, 2016
/// <param name="errorCallback">An error callback that will be invoked if the observing thread raises an error.</param>
/// <param name="coalesceMultipleChangesIntoReset">
/// When a lot of items have been added or removed at once it is more efficient to raise <see cref="INotifyCollectionChanged.CollectionChanged" /> once
/// with <see cref="NotifyCollectionChangedAction.Reset" /> instead of multiple times for every single change. Pass <c>true</c> to opt-in to this behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called Reset? Is that a convention for this kind of messaging? It doesn't make sense to me but I'm far from being a Reactive Thinker

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean - NotifyCollectionChangedAction is a framework enum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming as Reset sounds weird, found a reference which explains it "The content of the collection changed dramatically". and I accept it's MS weirdness rather than yours, so no problem.

@nirinchev
Copy link
Member

While I can see the reasoning behind resetting the collection on remove, it appears to be kind of restricting. In a hypothetical scenario, where I have bound a UITableView to the observable collection, to remove a row with a nice animation, only the index of the deleted item would suffice. With the current implementation, I would imagine the removing will be instantaneous at best and flickering at worst.

Obviously this will break the expectations of classic CollectionChanged subscribers, so it could be an opt-in behaviour, similar to coalesceMultipleChangesIntoReset.

@fealebenpae
Copy link
Member Author

@nirinchev oddly enough the Xamarin.Forms ListView does not (noticably) repaint when a CollectionChanged event is raised with a Reset action.

Right now the Xamarin.Forms ListView is the only consumer of INotifyCollectionChanged that is likely to use this adapter, so its functionality is sort of tailor-fit. However, in the UITableView scenario you should use the RealmResults<T>.SubscribeForNotifications API directly which gives you a changeset with the indices that have been added and/or removed which is exactly what UITableView needs.

I'm wary of making ToNotifyCollectionChanged too configurable. I believe SubscribeForNotifications is what developers should use in scenarios outside Xamarin.Forms.

@nirinchev
Copy link
Member

Makes sense. I was thinking about MVVMLight's ObservableTableViewController - if Realm's adapter were to emit delete events, consumers of the MVVMLight toolkit would be able to bind directly to it. As for SubscribeForNotifications, it is aligned with the classic Obj-C way of doing things, but I don't think it fits very well with MVVM where the iOS ViewControllers are dumbed down to views (although I don't know if that's the cool way of doing Xamarin.iOS/Android development).

Anyway, if you're primarily targeting Forms currently, it doesn't make a lot of sense to go out of your way and pollute the API to support a scenario of questionable business value, just wanted to give you some food for thought for future developments :)

@fealebenpae
Copy link
Member Author

@nirinchev thanks, your input is much appreciated.

Personally I hope that the base API provides the tools others would need to come up with specialized adapters for various scenarios, such as when targetting MVVMLight or RxUI.

@kristiandupont
Copy link
Contributor

👍

@fealebenpae fealebenpae force-pushed the yg/collectionchanged-adapter branch from a5a5a84 to fbd1676 Compare June 2, 2016 10:29
@fealebenpae fealebenpae merged commit 9e3c3dc into master Jun 2, 2016
@fealebenpae fealebenpae deleted the yg/collectionchanged-adapter branch June 2, 2016 11:13
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants