Trigger collection listeners when commit #4368
Conversation
- 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
|
|
||
| collection.removeAllChangeListeners(); | ||
|
|
||
| // This one is added after removal, so it should be triggered. | ||
| collection.addChangeListener(new RealmChangeListener<RealmList<Dog>>() { |
There was a problem hiding this comment.
Any reason to add this in a test named removeAllChangeListeners() ?
There was a problem hiding this comment.
because of i don't want to rely on the behavior "listeners triggered when commit" in this test. By adding another listener after all listeners removed, it can ensure that removeAllChangeListeners works, and the listener added after will be triggered. Which means the same test should work even when "listeners are triggered in the next event loop."
| break; | ||
| } | ||
| } | ||
| }); | ||
| addRow(sharedRealm); | ||
| assertEquals(collection.size(), 5); | ||
| transactionCommitted.set(true); | ||
| looperThread.testComplete(); |
There was a problem hiding this comment.
This seems to change the flow of this test. Before it was depending on the notification arriving through the looper, now it is triggered synchronously.
Wouldn't it be better to make a new test case listenerTriggeredOnCommit() or something that tests that the change listener is triggered both on beginTransaction() and commitTransaction() ?
There was a problem hiding this comment.
See the diff of the comments at the beginning of the test, the original test is to test the commitTransaction will trigger the listener in the next event loop. But now, the behavior to be tested is changed.
| break; | ||
| } | ||
| } | ||
| }); | ||
| addRow(sharedRealm); | ||
| assertEquals(collection.size(), 5); | ||
| transactionCommitted.set(true); | ||
| looperThread.testComplete(); |
There was a problem hiding this comment.
I think we should check if that the listener is called twice.
I don't think this updated test fails in case of not being called when committed.
| looperThread.testComplete(); | ||
| break; | ||
| default: | ||
| fail(); |
There was a problem hiding this comment.
What will happen when reached to here after looperThread.testComplete(); is called?
But yes, it's difficult to check that the listener is not called after looperThread.testComplete(); is executed.
It's just a question from my curiosity.
There was a problem hiding this comment.
If it reaches here after testComplete called, the test will still fail with assertion.
|
This reminds me of the commit/notification behavior in 0.88.3 |
when the transaction committed on the same thread if the collections
are changed or the async query should return.
merged.
This is to solve the problem for predictive UI animations and local
transactions.
Fix #4245