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 detailed notification for RealmObject #4331

Merged
merged 25 commits into from
Mar 30, 2017
Merged

Conversation

beeender
Copy link
Contributor

  • Add RowObject for wrapping Object Store's Object class.
  • Enabled detailed object notification.
  • Add RealmObjectChangeListener, ObjectChangeSet.
  • Fix false positive change notifications for RealmObject. Now the
    notification will only be sent when the object actually changes.

See #4101

- Add RowObject for wrapping Object Store's Object class.
- Enabled detailed object notification.
- Add RealmObjectChangeListener, ObjectChangeSet.
- Fix false positive change notifications for RealmObject. Now the
  notification will only be sent when the object actually changes.

See #4101
@beeender
Copy link
Contributor Author

beeender commented Mar 16, 2017

For use cases in #4101

  1. Object change listeners will now only be called if the object actually changed. It should reduce the number of unnecessary updates which is happening right now due to the change notification being table based.

  2. It should make it possible to register listeners for a single field value, e.g. if the UI should only be refreshed if a single field changed.

  3. It should be possible to register a listener on an object and be give the old and new values, e.g. useful if you only need to refresh the UI if a String changed in a particular way.

  4. It should now be possible to be notified when an object is deleted. See findFirstAsync is not emitting object when queried object is deleted from Realm #3138. Realm classes only implementing "RealmModel" might be tricky to support.

  5. We should strive for easy integration with Databinding https://developer.android.com/topic/libraries/data-binding/index.html. How this should be done is still undecided.


Currently this will solve 1) 4) need more investigation on 5)

  1. I don't say a strong requirement right now, so we can discuss it when needed in the future. Also based on current API, it is easy to get a 3rd party lib to subscribe on field changes.

  2. Yes, cocoa has it. My major concern is that would require to store the old value in memory and convert to java values. Problems will happen when there is some huge strings/binary existing, then the memory consumption + encoding performance are something really should be considered. Also, generic type for field value might also be a problem.
    I don't see there is case that old value is important, but if we need to implement it, based on current API, maybe we can have something like:

    class FieldChange<T> {
        /**
         * The name of the field which changed.
         */
        final String fieldName;
        final T oldValue;
        final T value;

        public FieldChange(String fieldName, T oldValue, T value) {
            this.fieldName = fieldName;
            this.oldValue = oldValue;
            this.value = value;
        }
    }

@beeender
Copy link
Contributor Author

@realm/java I am still working on this to fix some corner issues. But the API part is ready for review. Please take a look.

Copy link
Member

@kneth kneth left a comment

Choose a reason for hiding this comment

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

Looks promising!

CHANGELOG.md Outdated
@@ -15,6 +15,8 @@
* Migration for linking objects is not yet supported.
* Backlink verification is incomplete. Evil code can cause native crashes.
* [ObjectServer] In case of a Client Reset, information about the location of the backed up Realm file is now reported through the `ErrorHandler` interface (#4080).
* The listener on `RealmObject` will only be triggered if the object changes (#3894).
* Added `RealmObjectChangeListener` to get detailed information about `RealmObejct` changes.
Copy link
Member

Choose a reason for hiding this comment

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

"RealmObejct" -> "RealmObject"

/**
* Java wrapper for Object Store's {@code Object} class. Currently it is only used for object notifications.
*/
public class RowObject implements NativeObject {
Copy link
Member

Choose a reason for hiding this comment

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

Is RowObject really the best name? I know that RealmObject is taken but that's what it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am really out of ideas ... First i tried Object, but apparently it will conflict a lot with java's Object ... But ObjectStoreObject sounds really stupid... any suggestions are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just throwing ideas at the wall: OsObject, NativeRealmObject, InternalObject, OsRealmObject ... nothing is really great, so RowObject is probably fine for now.

But perhaps we should find a common prefix for all the ObjectStore classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

common prefix makes sense. So OsObject? and Collection -> OsResults?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and OsSharedRealm I assume?

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

I only looked at the public API parts. Generally it looks good I think. Only pain point is finding out what to do about FieldChange and what methods to have in the ObjectChangeset class. Generally, I'm not worried about long strings or byte arrays, since you probably wouldn't use field change listeners for those cases? But we should probably document the problem in any case.

CHANGELOG.md Outdated
@@ -15,6 +15,8 @@
* Migration for linking objects is not yet supported.
* Backlink verification is incomplete. Evil code can cause native crashes.
* [ObjectServer] In case of a Client Reset, information about the location of the backed up Realm file is now reported through the `ErrorHandler` interface (#4080).
* The listener on `RealmObject` will only be triggered if the object changes (#3894).
* Added `RealmObjectChangeListener` to get detailed information about `RealmObejct` changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

RealmObjct -> RealmObject

* Information about the changes made to an object.
* @see RealmObject#addChangeListener(RealmObjectChangeListener) .
*/
public interface ObjectChangeSet {
Copy link
Contributor

@cmelchior cmelchior Mar 16, 2017

Choose a reason for hiding this comment

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

Mirroring the CollectionChangeSet seems like the correct approach 👍

/**
* @return {@code null} if the object has been deleted. Otherwise returns the information about changed fields.
*/
FieldChange[] getFieldChanges();
Copy link
Contributor

@cmelchior cmelchior Mar 16, 2017

Choose a reason for hiding this comment

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

This wouldn't work very well if you had e.g an int and a String field, as this would basically be type-erased to FieldChange<Object>.

Figuring out how to represent the changes are probably the crux of the problem, since the rest of the API's fall fairly natural from what we did for Collection notifications. Also, we probably need to consider what is most likely if interested in multiple changes. Do you register 1 listener and do switches or register multiple listeners?

What about this, it adds a few more helper methods people can use, that could provide a little more type safety.

public interface ObjectChangeset {
  boolean isDeleted();
  // I'm not sure this is very useful in practical code. I cannot think of any generic code
  // where you wouldn't need the type information somehow?
  FieldChange[] getChanges(); 
  String[] getFieldNames;
  boolean didChange(String fieldName);
  <E> FieldChange<E> getChange(E type, String fieldName);

   /// Alternatively
   FieldChange<Long> getLongChange(String fieldName);
   FieldChange<String> getStringChange(String fieldName);
   FieldChange<Double> getDoubleChange(String fieldName);
   FieldChange<Float> getFloatChange(String fieldName);
   FieldChange<Boolean> getBooleanChange(String fieldName);
   FieldChange<Date> getDateChange(String fieldName);
   <E> FieldChange<E> getRealmObjectChange(E type, String fieldName);
   <E> FieldChange<RealmList<E>> getRealmListChange(E type, String fieldName);
}

Generally, I think we should try to make it as easy as possible for people to get the typed information since that is most likely what you want to operate in client code.

Copy link
Contributor

Choose a reason for hiding this comment

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

FieldChange getChange(E type, String fieldName);

E as type, how? You'd be able to use int.class and the like, but that falls apart with RealmList<E> which requires the ugly hack new TypeToken<RealmList<E>>() {};

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to use generics we are forced to use the boxed types, e.g Integer instead of int. Alternatively you are forced to create a FieldChange class for each type we support, which also seems like massive overkill: IntegerFieldChange, intFieldChange ....

Copy link
Contributor Author

@beeender beeender Mar 20, 2017

Choose a reason for hiding this comment

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

  1. I prefer one listener on the RealmObject instead of multiple listeners on the object's fields. It makes the API simpler and it is still quite simple for real use cases, eg.: databinding. We/Users can always inherit the RealmObjectChangeListener for specific use cases. See my example code of using it in data binding https://github.com/realm/realm-android-adapters/pull/99/files#diff-58a0e399a032fe72057d0e65269ebdb6

  2. I would choose something with less methods, so probably:

public interface ObjectChangeset {
  boolean isDeleted();
  String[] getChangedField();
 
  public static class FieldChange<T> {
    T oldValue;
    T newValue;
  }

  // This will always return null for RealmList field.
  <E> FieldChange<E> getChange(E type, String fieldName);
}
  1. FieldChange and getChange() will not be implemented in this PR. Also if we are going to have them, there are some restrictions:
  • For local transactions, there is no way to get the old values.
  • store the old values will be slow and consuming memory when the old value is a big String or Binary -- The listener is on the RealmObject instead of on the field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having isDeleted(), getChangedFields(), isChanged(String fieldName) and getChange(type, fieldName) probably covers the use cases I can think of 👍

It seems a rather big (and arbitrary) limitation that this doesn't work for local transactions. Any reason why?

IMO we should just document that you need to be careful if using this with objects with big field values. You shouldn't register 100's of these anyway, but doing it on an object with a 16MB image is probably a bad idea (if it changes). Cocoa has a feature request for ignoring certain fields I believe, but this is currently out of scope.

* When this gets called when the object is modified, {@code changeSet.isDeleted()} will return {@code false} and
* {@code changeSet.getFieldChanges()} will return the detailed information about the fields' changes.
* <p>
* This will be called if the {@link RealmObject} field gets set or removed, but it won't be called if the
Copy link
Contributor

@cmelchior cmelchior Mar 16, 2017

Choose a reason for hiding this comment

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

Perhaps rewriting this would make it a bit more clear:

If a field points to another RealmObject this listener will only be triggered if the field is set to a new object or null. Updating the referenced RealmObject will not trigger this listener. 

If a field points to a RealmList, this listener will only be triggered if a objects are inserted, removed or moved within the List. Updating the objects in the RealmList will no trigger this listener.

@@ -1593,7 +1590,7 @@ public void run() throws Exception {

//noinspection TryFinallyCanBeTryWithResources
try {
dog.addChangeListener(null);
dog.addChangeListener((RealmChangeListener)null);
Copy link
Contributor

Choose a reason for hiding this comment

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

dog.addChangeListener((RealmChangeListener) null);

@zaki50 zaki50 changed the title Add detailed notifcation for RealmObject Add detailed notification for RealmObject Mar 18, 2017
Backlinked object won't be notified when their parents object changed.
- Add ObjectChangeSet & RealmObjectChangeListener.
- Add OsObject to wrap ObjectStore's Object for notifications.
- No more false positive notifications for RealmObject.
- Use ObserverPairList in ProxyState instead of normal list to solve the
  potential listener removal problems which is handled well by the
  ObserverPairList.
- Fix tests.
* @return the names of changed fields if the object still exists and there are field changes. Returns {@code null}
* if the object has been deleted.
*/
String[] getChangedFields();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmelchior I am not quite convinced that boolean isFieldChanged(String fieldName) is useful here:

  • If user wants, it is very easy to do with Arrays.asList(getChangedFields()).contains(fieldName)
  • It would be ambiguous when the object is delete, what should it return in that case? false?

But I am totally fine if you strongly think it is needed. 🍏

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think a very common pattern is:

if (changeset.isFieldChanged("name")) {
  updateName(changeSet.getChange(String.class, "name").newValue());
}

That should be as easy as possible.

Agree, that it gets slightly ambigious with deleted objects, but getChangedFields() would also return nothing, so I think that will be acceptable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, i will add it in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make a list of outstanding issues? or known problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, see #4366

@beeender
Copy link
Contributor Author

@realm/java This is ready for review.
@bmeike I removed some tests from backlink test set because of those notification on backlinked object should not be triggered when the parent changes. They passed before simply because of we fire lots of false positive notification on RealmObject before. But let me know if there are any tests related to this PR need to be added to the backlink tests.

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Didn't have time for a full review. So only got halfway through it. Mostly minor stuff.

// A listener registered on the backlinked object should be called when a commit adds a backlink
@Test
@RunTestInLooperThread
public void notification_onCommitModelObject() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like a mistake to delete this test? Perhaps a restriction in the Object Store?

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, it is not. And it is expected behavior from object store.
Line 175 called parent.setFieldObject(child);, the changed object is parent, but the original test added listener on child which won't be trigger since nothing changed on the child. I am not quite sure what is the purpose of this test, maybe @bmeike can help to clarify this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically the RealmResults field on child is a field with a (backlink) reference to the parent, so one could argue it should trigger the same type of notification as when a RealmList changes. I'm fine with leaving it as it is right now, but we should probably document that somewhere.

Copy link
Contributor

@bmeike bmeike Mar 24, 2017

Choose a reason for hiding this comment

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

Yeah... I'm with Christian on this. Something has, most definitely, changed, for the child. child.backLinkedField.size(), for instance, is different. That is, clearly, a change.

If notifications on backlinks are not supported, that's life. We need clear documentation of the fact, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified in the javadoc of LinkingObjects.

// A listener registered on the backlinked object should be called when a backlinked object is deleted
@Test
@RunTestInLooperThread
public void notification_onDeleteModelObject() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

// This test exists only to document existing (but odd) behavior.
@Test
@RunTestInLooperThread
public void notification_notSentOnUnrelatedChangeModelObject() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was only here to document the old behaviour so 👍

// A JSON update should generate a notifcation
@Test
@RunTestInLooperThread
public void json_jsonUpdateCausesNotification() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should be deleted?


@Test
@RunTestInLooperThread(before = PopulateOneAllTypes.class)
public void changeDoubleField() {
Copy link
Contributor

Choose a reason for hiding this comment

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

All types doesn't seem to be covered. Missing is at least: boolean, float, byte[], Date

Copy link
Collaborator

Choose a reason for hiding this comment

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

and int since the first test is covering long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

long is just the same as int, and there is no int field in the AllTypes :(

boolean isDeleted();

/**
* @return the names of changed fields if the object still exists and there are field changes. Returns {@code null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return String[0] if no fields changed IMO. No reason to force a bunch of null-checks onto people.

*
* @param listener the change listener to be notified.
* @throws IllegalArgumentException if the change listener is {@code null} or the object is an unmanaged object.
* @throws IllegalArgumentException if object is an unmanaged RealmObject.
Copy link
Contributor

Choose a reason for hiding this comment

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

You just stated that above.

* When this gets called to return the results of an asynchronous query made by {@link RealmQuery#findFirstAsync()},
* {@code changeSet} will be {@code null}.
* <p>
* When this gets called because of the object is deleted, {@code changeSet.isDeleted()} will return {@code true}
Copy link
Contributor

Choose a reason for hiding this comment

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

because the object was deleted

* When this gets called because of the object is deleted, {@code changeSet.isDeleted()} will return {@code true}
* and {@code changeSet.getFieldChanges()} will return {@code null}.
* <p>
* When this gets called because of the object is modified, {@code changeSet.isDeleted()} will return {@code false}
Copy link
Contributor

Choose a reason for hiding this comment

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

because the object was modified

* @return the names of changed fields if the object still exists and there are field changes. Returns {@code null}
* if the object has been deleted.
*/
String[] getChangedFields();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make a list of outstanding issues? or known problems?

@Zhuinden
Copy link
Contributor

Databinding support requires an ignored field. But that is a question of treating transient the same as @Ignore, and also that inheriting from a parent that has only ignored fields needs to be allowed by the annotation processor

@cmelchior cmelchior added this to the 3.1 milestone Mar 23, 2017
looperThread.testComplete();
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

What if multiple fields are changed in the same transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test case.

{
JNIEnv* env = JniUtils::get_env(false);
if (!env || env->ExceptionCheck()) {
// JVM dettached or java exception has been thrown before.
Copy link
Member

Choose a reason for hiding this comment

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

"dettached" -> "detached"

@@ -313,11 +313,22 @@ public final boolean load() {
}

/**
* Adds a change listener to this RealmObject to get detailed information about the changes.
Copy link
Member

Choose a reason for hiding this comment

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

"about the" -> "about"


/**
* {@code RealmObjectChangeListener} can be registered on a {@link RealmModel} or {@link RealmObject} to receive
* detailed notification when a object changes.
Copy link
Member

Choose a reason for hiding this comment

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

"notification" -> "notifications"

* Realm instances on a thread without an {@link android.os.Looper} cannot register a {@code RealmObjectChangeListener}.
* <p>
*
* @param <T> The type of {@link RealmModel} where your listener will be registered on.
Copy link
Member

Choose a reason for hiding this comment

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

"where" -> "on which"?

@@ -428,6 +429,38 @@ void invalidateIterators() {
iterators.clear();
}

// addPendingRow, removePendingRow and executePendingRow queries is to solve that the listener cannot be added
Copy link
Member

Choose a reason for hiding this comment

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

"is" -> "are"

@@ -428,6 +429,38 @@ void invalidateIterators() {
iterators.clear();
}

// addPendingRow, removePendingRow and executePendingRow queries is to solve that the listener cannot be added
// inside a transaction. For the findFirstAsync(), listener is registered on a Object Store Results first, then move
Copy link
Member

Choose a reason for hiding this comment

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

"a Object" -> "an Object"

// inside a transaction. For the findFirstAsync(), listener is registered on a Object Store Results first, then move
// the listeners to the Object when the query for Results returns. When beginTransaction() called, all listeners'
// on the results will be triggered first, that leads to the registration of listeners on the Object which will
// will throw because of the transaction has began already. So here we execute all PendingRow queries first before
Copy link
Member

Choose a reason for hiding this comment

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

remove "will"
"has began already" -> "has already begun"

@bmeike
Copy link
Contributor

bmeike commented Mar 24, 2017

I am just getting to this. I think that '@' to me, somehow did not actually notify me.
So, I'm not qualified to talk about "expected behavior" as far as core is concerned. I definitely would expect to be notified when the contents of a backlinked field changes. If that is not the case, then I think we need documentation that says that notifications do not work for backlinks.
I would prefer that either those UTs get re-enabled and pass or that we add documentation saying that backlink fields do not get notifications, before this commit goes in

@bmeike bmeike self-requested a review March 24, 2017 18:13
Copy link
Contributor

@bmeike bmeike left a comment

Choose a reason for hiding this comment

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

I believe that either those UT should be made to work, or we need to document the fact that notifications do not work for backlinks.

public final RunInLooperThread looperThread = new RunInLooperThread();

@Before
public void setUp() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused delete

}

@After
public void tearDown() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused delete

if (field.equals(name)) {
break;
}
fail("Cannot find field " + name + " in field changes.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the fail be outside the for-loop?
current impl runs at most 1 iteration (either exit through break or fail)

public void run() {
looperThread.testComplete();
}
}, 100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're sure that the testComplete will be the last Runnable in the looper queue, then no need to use the delayed 100 ms. Just looperThread .postRunnable should suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The event might be sent through the notifier thread and we have no control for that thread unfortunately. I don't like the delay here, and I don't like to rely on the fact we are triggering on the commitTransaction will trigger the listener either . So post delay is more "common" solution here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically calling listeners for local commit in commitTransaction() will fix some silly bugs like #4384 so it is a good thing.


@Test
@RunTestInLooperThread(before = PopulateOneAllTypes.class)
public void changeIntField() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the actual test is testing long not int maybe rename to changeLongField


/**
* {@code RealmObjectChangeListener} can be registered on a {@link RealmModel} or {@link RealmObject} to receive
* detailed notification when a object changes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

a -> an object

* detailed notification when a object changes.
* <p>
* Realm instances on a thread without an {@link android.os.Looper} cannot register a {@code RealmObjectChangeListener}.
* <p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cannot also register within a transaction

* and {@code changeSet.getFieldChanges()} will return {@code null}.
* <p>
* When this gets called because the object was modified, {@code changeSet.isDeleted()} will return {@code false}
* and {@code changeSet.getFieldChanges()} will return the detailed information about the fields' changes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

fields' -> field's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it means fields's here.

* If a field points to another RealmObject this listener will only be triggered if the field is set to a new object
* or null. Updating the referenced RealmObject will not trigger this listener.
* <p>
* If a field points to a RealmList, this listener will only be triggered if a objects are inserted, removed or
Copy link
Collaborator

Choose a reason for hiding this comment

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

if a -> if one or many objects were inserted, removed or ..

* or null. Updating the referenced RealmObject will not trigger this listener.
* <p>
* If a field points to a RealmList, this listener will only be triggered if a objects are inserted, removed or
* moved within the List. Updating the objects in the RealmList will no trigger this listener.
Copy link
Collaborator

Choose a reason for hiding this comment

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

will no -> will not

beeender added a commit that referenced this pull request Mar 27, 2017
- The listeners on RealmList/RealmResults will be tiggered immediately
  when the transaction committed on the same thread if the collections
  are changed or the async query should return.
- Listeners on the RealmObject will have same behaviour after #4331
  merged.

This is to solve the problem for predictive UI animations and local
transactions.
Fix #4245
@@ -57,6 +57,7 @@
* <li>They are ignored when doing a `copyToRealm().`</li>
* <li>They are ignored when doing a `copyFromRealm().`</li>
* <li>They are ignored when using the various `createObjectFromJson*` and `createAllFromJson*` methods.</li>
* <li>The listeners on parent object won't be triggered when the linking objects changes.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps rephase. I especially find "parent object" confusion since I assume it means the object pointing to this object.

Suggestion:
Listeners on an object with a @LinkingObject field will not be triggered if the linking objects change, e.g if another object drops a reference to this object.

@@ -354,6 +365,30 @@ public final boolean load() {
}

/**
* Adds a change listener to a RealmObject.
Copy link
Contributor

Choose a reason for hiding this comment

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

That is how the current change listeners work? The fine-grained object notifications work slightly different that way

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Only some minor doc comments, otherwise this looks shippable.

// the listeners to the Object when the query for Results returns. When beginTransaction() called, all listeners'
// on the results will be triggered first, that leads to the registration of listeners on the Object which will
// will throw because of the transaction has began already. So here we execute all PendingRow queries first before
// throw because of the transaction has already began. So here we execute all PendingRow queries first before
Copy link
Member

Choose a reason for hiding this comment

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

"began" -> "begun"

@@ -57,7 +57,6 @@
* <li>They are ignored when doing a `copyToRealm().`</li>
* <li>They are ignored when doing a `copyFromRealm().`</li>
* <li>They are ignored when using the various `createObjectFromJson*` and `createAllFromJson*` methods.</li>
* <li>The listeners on parent object won't be triggered when the linking objects changes.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, so they are triggered? Cool.

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, but the next line in this file is talking about the same thing :p

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, so I should learn how to read 😄

@bmeike
Copy link
Contributor

bmeike commented Mar 29, 2017

Thanks for adding the doc. I think that will reduce the number of surprised developers!

@beeender beeender merged commit 6361326 into master Mar 30, 2017
@beeender beeender deleted the mc/object-notification branch March 30, 2017 02:59
@beeender beeender removed the S:Review label Mar 30, 2017
@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

7 participants