Enhancing RealmResults & RealmObject notifications #1809

nhachicha · 2015-11-22T19:58:45Z

Adding support for async queries for Dynamic Realm API
Fixing a bug where findFirstAsync was not rerunning the query if the initial value was empty
Adding more tests for type-based notifications
Callbacks (RealmChangeListeners) are now called on both synchronous & asynchronous RealmResults & RealmObject
Using Table version to reduce the number of callback invocations (this will filter out all commits non impacting the query used to obtain the RealmResults/RealmObject)
Some tests involving RealmObject callbacks are failing, we're waiting for Expose private m_version for table realm-core#1323 implementation

nhachicha · 2015-11-26T11:25:31Z

Hi @rgrinberg
No since we'll be using internally a weak reference listeners for Adapter

nhachicha · 2015-11-29T18:40:38Z

@realm/java

cmelchior · 2015-11-30T08:52:28Z

changelog.txt

@@ -1,3 +1,6 @@
+0.87.0
+* BREAKING CHANGE: Realm.addChangeListener, RealmObject.addChangeListener and RealmResults.addChangeListener will throw an IllegalStateException when invoked on a non-Looper thread. This is to prevent registering listeners that will never be invoked (since we rely on Handler/Looper).


Not sure about the comment in ()...but not a big issue

cmelchior · 2015-11-30T10:59:37Z

realm/realm-library/src/main/java/io/realm/RealmResults.java

@@ -826,9 +828,16 @@ public void removeChangeListeners() {
     * Notifies all registered listeners.
     */
    void notifyChangeListeners() {
-        realm.checkIfValid();


This is checked before calling this method?

notifyChangeListeners is always in a safe path (called internally) or preceded by a check realm.checkIfValid()

cmelchior · 2015-11-30T11:04:47Z

Mind-melting to read 2000 lines of unit tests, but love the thoroughness of them. Generally looks good, mostly comments about style issues. I am a bit concerned about the complexity of the implementation though, but in general there is a lot of comments so that is good. Would it make sense to add a high-level description on how async work in general on the HandlerController (since that is the central class) akin to what was done for https://github.com/realm/realm-java/blob/master/realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProcessor.java ?

zaki50 · 2015-12-01T15:00:38Z

realm/realm-jni/src/io_realm_internal_table.cpp

+    bool valid = (TBL(nativeTablePtr) != NULL);
+    if (valid) {
+        if (!TBL(nativeTablePtr)->is_attached()) {
+            ThrowException(env, TableInvalid, "The Realm has been closed and is no longer accessible");


. at the end of message.

cmelchior · 2015-12-01T15:35:53Z

Really like the refactoring of HandlerController. HandlerController could probably use an extended description of what is happening, but it doesn't have to be part of this PR. 👍

zaki50 · 2015-12-01T16:22:02Z

realm/realm-library/src/main/java/io/realm/DynamicRealmObject.java

@@ -64,6 +65,11 @@ public DynamicRealmObject(RealmObject obj) {
        this.row = (row instanceof CheckedRow) ? (CheckedRow) row : ((UncheckedRow) row).convertToChecked();
    }

+    DynamicRealmObject(BaseRealm realm, String className) {


we can remove the argument of realm since it will be assigned by RealmQuery.

zaki50 · 2015-12-01T17:09:22Z

If a DynamicRealmObject is accessed before async query is complete, getObject(String) seems to throw IllegalStateException and getList(String) seems to throw NPE.
Other getters seem to return default values (0).

Is that the intended behavior?

zaki50 · 2015-12-01T17:24:14Z

Should we document that the dynamic async query and changing schema don't work at the same time?
I'd like to believe no one do that, but users do everything they can...

zaki50 · 2015-12-01T23:45:34Z

realm/realm-library/src/androidTest/java/io/realm/DynamicRealmTest.java

+        });
+
+        TestHelper.awaitOrFail(signalTestFinished);
+    }


Shouldn't we call handlerThread.quitSafely(); handlerThread.join() to quit the thread before returning the test?

the handlerThread is not shared/reused between tests it will be GC'ed

zaki50 · 2015-12-02T01:38:10Z

@nhachicha I reviewed and put some questions.

nhachicha · 2015-12-02T14:13:56Z

@realm/java

cmelchior · 2015-12-02T14:19:56Z

👍

zaki50 · 2015-12-02T15:42:15Z

👍

nhachicha · 2015-12-02T15:45:00Z

squashed commits

Enhancing RealmResults & RealmObject notifications

nhachicha self-assigned this Nov 22, 2015

nhachicha added the WIP label Nov 22, 2015

kneth added the P1 label Nov 23, 2015

nhachicha mentioned this pull request Nov 23, 2015

RealmResults<Foo> change listener not called after adding new Foo objects to Realm. #1802

Closed

nhachicha mentioned this pull request Nov 26, 2015

Support react-native android realm/realm-js#16

Closed

4 tasks

nhachicha added Review and removed WIP labels Nov 29, 2015

cmelchior reviewed Nov 30, 2015
View reviewed changes

zaki50 reviewed Dec 1, 2015
View reviewed changes

nhachicha mentioned this pull request Dec 2, 2015

Use a consistent pattern for test involving Handler #1874

Closed

enhancing notifications, adding Async queries for DynamicRealmObject

d18b97d

nhachicha force-pushed the nh/fine-grained-notif branch from 63648bb to d18b97d Compare December 2, 2015 15:39

nhachicha added a commit that referenced this pull request Dec 2, 2015

Merge pull request #1809 from realm/nh/fine-grained-notif

59e02d1

Enhancing RealmResults & RealmObject notifications

nhachicha merged commit 59e02d1 into master Dec 2, 2015

nhachicha removed P1 labels Dec 2, 2015

nhachicha deleted the nh/fine-grained-notif branch December 2, 2015 15:52

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancing RealmResults & RealmObject notifications #1809

Enhancing RealmResults & RealmObject notifications #1809

nhachicha commented Nov 22, 2015

nhachicha commented Nov 26, 2015

nhachicha commented Nov 29, 2015

cmelchior Nov 30, 2015

cmelchior Nov 30, 2015

nhachicha Dec 1, 2015

cmelchior Dec 1, 2015

cmelchior commented Nov 30, 2015

zaki50 Dec 1, 2015

cmelchior commented Dec 1, 2015

zaki50 Dec 1, 2015

zaki50 commented Dec 1, 2015

zaki50 commented Dec 1, 2015

zaki50 Dec 1, 2015

nhachicha Dec 2, 2015

zaki50 commented Dec 2, 2015

nhachicha commented Dec 2, 2015

cmelchior commented Dec 2, 2015

zaki50 commented Dec 2, 2015

nhachicha commented Dec 2, 2015

		@@ -1,3 +1,6 @@
		0.87.0
		* BREAKING CHANGE: Realm.addChangeListener, RealmObject.addChangeListener and RealmResults.addChangeListener will throw an IllegalStateException when invoked on a non-Looper thread. This is to prevent registering listeners that will never be invoked (since we rely on Handler/Looper).

Enhancing RealmResults & RealmObject notifications #1809

Enhancing RealmResults & RealmObject notifications #1809

Conversation

nhachicha commented Nov 22, 2015

nhachicha commented Nov 26, 2015

nhachicha commented Nov 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior commented Nov 30, 2015

Choose a reason for hiding this comment

cmelchior commented Dec 1, 2015

Choose a reason for hiding this comment

zaki50 commented Dec 1, 2015

zaki50 commented Dec 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaki50 commented Dec 2, 2015

nhachicha commented Dec 2, 2015

cmelchior commented Dec 2, 2015

zaki50 commented Dec 2, 2015

nhachicha commented Dec 2, 2015