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

Enable listeners on RealmList #4216

Merged
merged 12 commits into from
Feb 24, 2017
Merged

Enable listeners on RealmList #4216

merged 12 commits into from
Feb 24, 2017

Conversation

beeender
Copy link
Contributor

@beeender beeender commented Feb 21, 2017

The RealmList holds a Collection which is used for listeners.
Other RealmList APIs are still calling from LinkView.

  • Add tests for RealmList and RealmChangeListener.

- Add RealmObservable and RealmCollectionObservable interfaces.
- Enable detailed change information for RealmResults through
  OrderedCollectionChange interface.
- Fix a bug in the ObserverPairList which could cause the removed
  listener gets called if it was removed during previous listener
  iteration.

Fix #989
The listeners should be check by equality.
@beeender
Copy link
Contributor Author

I is quite difficult for me to make all the RealmResults and RealmList listeners related test in one parameterized test because of the generics. So just add the new tests into RealmListTests.

The RealmList holds a Collection which is used for listeners.
Other RealmList APIs are still calling from LinkView.

@Test
@RunTestInLooperThread
public void removeALlChangeListeners() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ALl -> All

@@ -15,8 +15,7 @@

/**
* General implementation for {@link OrderedRealmCollection} which is based on the {@code Collection}.
* Currently only {@link RealmResults} and {@link OrderedRealmCollectionSnapshot} extend this class. But
* {@link RealmList} could also extend this to share the same iterator implementation.
* Currently only {@link RealmResults} and {@link OrderedRealmCollectionSnapshot} extend this class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any value in this line at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. will remove.

*/
@Override
public void removeAllChangeListeners() {
realm.checkIfValid();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if this could be incorporated into the checkForAddRemoveListener method? Perhaps checkForAddRemoveListener(listener, checkForNull) ?

@@ -13,6 +13,7 @@

* Added support for sorting by link's field (#672).
* Added `OrderedRealmCollectionSnapshot` class and `OrderedRealmCollection.createSnapshot()` method. `OrderedRealmCollectionSnapshot` is useful when changing `RealmResults` or `RealmList` in simple loops.
* Added support for adding listeners on `RealmList`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens with the iterators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RealmList's iterators stay the same. They have quite different meanings compared with RealmResults. e.g.: RealmList is actually mutable for users, but RealmResults is not.

@beeender beeender changed the base branch from mc/fine-grained-notification to master February 23, 2017 03:39
realm.commitTransaction();

collection.removeChangeListener(listener1);
collection.removeChangeListener(listener2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep at least one listener in order to make sure that removeChangeListener does not remove all listeners.

@@ -348,6 +348,7 @@ public Collection(SharedRealm sharedRealm, LinkView linkView, SortDescriptor sor
this.context = sharedRealm.context;
this.table = linkView.getTable();
this.context.addReference(this);
this.loaded = true;
Copy link
Contributor

@zaki50 zaki50 Feb 24, 2017

Choose a reason for hiding this comment

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

We should not initialize loaded field twice.
private boolean loaded = false; should be private boolean loaded; and assign value in each constructor.

Copy link
Contributor

@zaki50 zaki50 left a comment

Choose a reason for hiding this comment

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

👍

@beeender beeender merged commit 5b1a47f into master Feb 24, 2017
@beeender beeender removed the S:Review label Feb 24, 2017
@emanuelez emanuelez deleted the mc/realm-list-listener branch February 27, 2017 13:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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

3 participants