Handle change notifications originating from Core #909

nirinchev · 2016-11-08T13:01:42Z

TODO:

Unregister observed items (event unsubscribe, remove object)
Unregister observed items on deletion (invalidated vector)
Handle changes for same-thread realms (I'm thinking of adding NotifyChanges() at the end of each set_xxx method)
Lookup property index, instead of using the column index
Decide whether we want to support [MapTo]
Moar unit tests 👺
Tests for object deletion

Resolves #886

kristiandupont

Looks good

kristiandupont · 2016-11-21T10:22:31Z

Shared/Realm.Shared/Linq/RealmResults.cs

@@ -271,7 +270,7 @@ private void UnsubscribeFromNotifications()
        {
            Debug.Assert(_notificationToken != null, "_notificationToken must not be null to unsubscribe.");

-            _notificationToken.Dispose();
+            _notificationToken?.Dispose();


We've just asserted that it wasn't null above -- is this defensive programming?

It's a Debug.Assert, meaning it won't run in released apps and just wanted to avoid potential crash in unsubscribing over which users usually have no control.

kristiandupont · 2016-11-21T10:26:38Z

Shared/Realm.Shared/RealmObject.cs

+
+        static RealmObject()
+        {
+            NativeCommon.register_notify_realm_object_changed(NotifyRealmObjectPropertyChanged);


Is there any advantage to keeping this here rather than in the static Realm constructor? I get that it's slightly more related to this, but it seems to me that it would be nice to keep static initialization grouped together, if for no other reason then because it makes the execution order deterministic.

Yeah, I don't have strong feelings about where it should go - as you said, it's slightly more related to object, so I left it here, but I don't mind moving it to Realm.

CSharpBindingContext.will_change is not called

- Use property indices instead of column indices - Respect [MapTo] - Add more tests - Fix deadlock on remove

fealebenpae · 2016-11-21T14:31:43Z

Shared/Realm.Shared/RealmObject.cs

+        {
+            var gch = GCHandle.FromIntPtr(realmObjectHandle);
+            var realmObject = (RealmObject)gch.Target;
+            var propertyName = realmObject.ObjectSchema.ElementAtOrDefault((int)propertyIndex).PropertyInfo.Name;


Realms.Schema.Property.PropertyInfo will be null for dynamic realms.

fealebenpae · 2016-11-21T14:35:49Z

Shared/Realm.Shared/RealmObject.cs

+            Debug.Assert(_notificationsHandle.HasValue, "Notification handle must not be null to unsubscribe");
+
+            _realm.SharedRealmHandle.RemoveObservedObject(GCHandle.ToIntPtr(_notificationsHandle.Value));
+            _notificationsHandle = null;


I think you are going to have to call GCHandle.Free() on both handles you create in SubscribeForNotifications to prevent the handles from being leaked.

fealebenpae · 2016-11-21T15:54:35Z

wrappers/src/shared_realm_cs.cpp

+        auto observer_state = BindingContext::ObserverState();
+        observer_state.row_ndx = object.row().get_index();
+        observer_state.table_ndx = object.row().get_table()->get_index_in_group();
+        observer_state.info = new ObservedObjectDetails(object.get_object_schema(), managed_object_handle);


this needs to be deleted later.

fealebenpae · 2016-11-21T15:55:26Z

wrappers/src/shared_realm_cs.cpp

+        observer_state.row_ndx = object.row().get_index();
+        observer_state.table_ndx = object.row().get_table()->get_index_in_group();
+        observer_state.info = new ObservedObjectDetails(object.get_object_schema(), managed_object_handle);
+        observed_rows.push_back(observer_state);


use std::move to prevent a copy.

fealebenpae · 2016-11-21T15:57:46Z

wrappers/src/shared_realm_cs.cpp

+        }
+    }
+
+    void* CSharpBindingContext::get_managed_realm_handle()


define in header instead so that the getter can be inlined in other translation units.

fealebenpae · 2016-11-21T15:59:24Z

wrappers/src/shared_realm_cs.hpp

+    private:
+        void* m_managed_realm_handle;
+        std::vector<BindingContext::ObserverState> observed_rows;


use the m_ prefix for fields.

fealebenpae · 2016-11-21T16:00:41Z

Shared/Realm.Shared/RealmObject.cs

        #if __IOS__
        [MonoPInvokeCallback(typeof(NativeCommon.NotifyRealmCallback))]
        #endif
-        private static void NotifyRealmObjectPropertyChanged(IntPtr realmObjectHandle, IntPtr propertyIndex)
+        public static void NotifyRealmObjectPropertyChanged(IntPtr realmObjectHandle, IntPtr propertyIndex)


this should be internal

fealebenpae · 2016-11-21T17:02:57Z

wrappers/src/shared_realm_cs.cpp

@@ -65,7 +61,7 @@ namespace binding {

    CSharpBindingContext::CSharpBindingContext(void* managed_realm_handle) : m_managed_realm_handle(managed_realm_handle)
    {
-        observed_rows = std::vector<ObserverState>();
+        m_observed_rows = std::vector<ObserverState>();


You don't need to initialize this - the compiler will have added the necessary code to invoke the std::vector constructor in the CSharpBindingContext constructor.

nirinchev added T-Bug S:In Progress labels Nov 8, 2016

nirinchev added this to the v1.0 milestone Nov 8, 2016

realm-ci assigned nirinchev Nov 8, 2016

nirinchev force-pushed the ni/object-notifications branch 2 times, most recently from 7105d17 to 34b25f2 Compare November 11, 2016 23:15

nirinchev added S:Review and removed S:In Progress labels Nov 15, 2016

kristiandupont approved these changes Nov 21, 2016

View reviewed changes

nirinchev added 13 commits November 21, 2016 15:39

wip

880ec07

wip

44598b4

wip

249e9d9

Add tests for unmanaged NotifyPropertyChanged

d48664e

more wip :/

7c6b5e9

wip

3153ecc

CSharpBindingContext.will_change is not called

Get notifications working for other-thread-realms

8ed45f9

Add support for unsubscribing

9b8bc36

Add logic to call property changed for instances on the same thread

addc3f2

Fix tests

c3e17e6

Fix a few outstanding issues

3334919

- Use property indices instead of column indices - Respect [MapTo] - Add more tests - Fix deadlock on remove

Fix android build

d2793df

Don't use the obsolete ReadOnly field

692aa9d

nirinchev force-pushed the ni/object-notifications branch from 5a08867 to 692aa9d Compare November 21, 2016 15:17

Move register_notify_realm_object_changed to Realm static ctor

9da9034

fealebenpae requested changes Nov 21, 2016

View reviewed changes

address comments

bdf0c5f

nirinchev force-pushed the ni/object-notifications branch from bae4500 to bdf0c5f Compare November 21, 2016 16:56

nirinchev mentioned this pull request Nov 21, 2016

Devise a way to free GCHandles #934

Closed

fealebenpae reviewed Nov 21, 2016

View reviewed changes

move the getter to the .hpp

eeaf3d7

fealebenpae approved these changes Nov 21, 2016

View reviewed changes

nirinchev merged commit f9be43f into master Nov 21, 2016

nirinchev deleted the ni/object-notifications branch November 21, 2016 22:52

nirinchev removed the S:Review label Nov 21, 2016

il-frans mentioned this pull request Nov 30, 2016

Listening to PropertyChanged event on main thread, for changes done in background threads not being raised #957

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle change notifications originating from Core #909

Handle change notifications originating from Core #909

nirinchev commented Nov 8, 2016 •

edited

Loading

kristiandupont left a comment

kristiandupont Nov 21, 2016

nirinchev Nov 21, 2016

kristiandupont Nov 21, 2016

nirinchev Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

fealebenpae Nov 21, 2016

Handle change notifications originating from Core #909

Handle change notifications originating from Core #909

Conversation

nirinchev commented Nov 8, 2016 • edited Loading

kristiandupont left a comment

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

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

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

Choose a reason for hiding this comment

nirinchev commented Nov 8, 2016 •

edited

Loading