Avoid calling setter for bound properties for same value #3129

papafe · 2022-12-12T13:48:16Z

Avoids calling the setter for bound properties in case the new value is the same as the old one. This avoids a problem in MAUI in which the UI is calling the setter unnecessarily, and causing an issue in case objects outside of the user's write permissions are modified

Fixes #3128

TODO

Changelog entry
Tests

papafe · 2022-12-12T13:51:37Z

Two things:

I'm not sure if it would make sense to add more tests and verify that this works with all the possible combination of how to call the setter (invoke the setter / call set value/ ....) and write transaction already happening or not. There is only one tests now that seems to be the more general path
There are wildly different ways in which we can get the current value of the property, so I'd be interested to hear if what I've done makes sense.

fealebenpae

Now that I think about it, I wonder if there's a Sync-related reason why we might not want to avoid the redundant set in all cases. @nirinchev, do you know if it matters for the sync history that a set is recorded here anyway?

fealebenpae · 2022-12-12T14:08:39Z

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs

            {
-                return managingRealm.Write(() =>
+                if (_getterMi.Invoke(realmObject, null).Equals(parameters[0]))


I'm not crazy about the reflection here. I would've preferred punching straight to the accessor's GetValue. But this is by definition not a hot path, so I suppose it doesn't really matter. But a comment explaining why we do this is necessary, ideally linking to the issue for the full context.

I'll definitely add a comment.
I was thinking about using GetValue here, but it starts to get a little more complex because we also don't know exactly what is the mapTo name of the property

If the property value is null this will throw, wouldn't it?

nirinchev

Seems reasonable. I have a few minor comments/suggestions.

nirinchev · 2022-12-13T08:14:26Z

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs

            {
-                return managingRealm.Write(() =>
+                if (_getterMi.Invoke(realmObject, null).Equals(parameters[0]))


If the property value is null this will throw, wouldn't it?

nirinchev · 2022-12-13T08:15:53Z

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs

+                    return null;
+                }
+
+                var managingRealm = (obj as IRealmObjectBase)?.Realm;


We already have realmObject - there's no need to do an extra cast here.

nirinchev · 2022-12-13T08:16:52Z

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs


        public override object Invoke(object obj, BindingFlags invokeAttr, Binder binder, object[] parameters, CultureInfo culture)
        {
-            var managingRealm = (obj as IRealmObjectBase)?.Realm;
+            var realmObject = obj as IRealmObjectBase;


Can we add an early return here in case obj is not IRealmObjectBase and remove all the null propagation operators?

We only have one one null propagation line (realmObject?.IsManaged == true), so I suppose we can keep just that?

I think we can still make the checks cleaner by merging the cast with the if-clause (I'm kind of surprised VS isn't hinting this):

if (obj is IRealmObjectBase realmObject && realmObject.IsManaged) { // ... }

nirinchev · 2022-12-13T08:22:21Z

Now that I think about it, I wonder if there's a Sync-related reason why we might not want to avoid the redundant set in all cases. @nirinchev, do you know if it matters for the sync history that a set is recorded here anyway?

Yes, it matters for conflict resolution, but I believe this solution is still the right one considering MAUI's bug. Re-setting all properties every time the UI renders is most likely not what the user intends to do anyway, so this one is the lesser of two evils.

nirinchev · 2022-12-13T11:01:59Z

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs

+                var managingRealm = realmObject.Realm;
+
+                // If managingRealm is not null and not currently in transaction, wrap setting the property in a realm.Write(...)
+                if (realmObject.Realm?.IsInTransaction == false)


We can use managingRealm here.

nirinchev · 2022-12-13T11:04:45Z

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs


        public override object Invoke(object obj, BindingFlags invokeAttr, Binder binder, object[] parameters, CultureInfo culture)
        {
-            var managingRealm = (obj as IRealmObjectBase)?.Realm;
+            var realmObject = obj as IRealmObjectBase;


I think we can still make the checks cleaner by merging the cast with the if-clause (I'm kind of surprised VS isn't hinting this):

if (obj is IRealmObjectBase realmObject && realmObject.IsManaged) { // ... }

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs

Added equality check

3c9eba3

papafe requested review from fealebenpae, nirinchev, LaPeste and JacobOscarGunnarsson December 12, 2022 13:48

cla-bot bot added the cla: yes label Dec 12, 2022

github-actions bot assigned papafe Dec 12, 2022

Added changelog

46a9309

fealebenpae approved these changes Dec 12, 2022

View reviewed changes

nirinchev approved these changes Dec 13, 2022

View reviewed changes

Corrected null checking and added another test

Verified

This commit was signed with the committer’s verified signature.

tecosaur Timothy

SSH Key Fingerprint: eobz41Mnm0/iYWBvWThftS0ElEs1ftBr6jamutnXc/A
Verified
Learn about vigilant mode

b4b3e33

nirinchev approved these changes Dec 13, 2022

View reviewed changes

LaPeste approved these changes Dec 13, 2022

View reviewed changes

Small corrections

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

eb32669

github-advanced-security bot found potential problems Dec 13, 2022

View reviewed changes

Realm/Realm/DataBinding/WovenSetterMethodInfo.cs Fixed Show fixed Hide fixed

Small fix

bfa0e0c

papafe changed the title ~~Added equality check on binding setter~~ Avoid calling setter for bound properties for same value Dec 13, 2022

papafe merged commit 69614d5 into main Dec 13, 2022

papafe deleted the fp/fix-maui-setter branch December 13, 2022 14:17

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid calling setter for bound properties for same value #3129

Avoid calling setter for bound properties for same value #3129

papafe commented Dec 12, 2022 •

edited

Loading

Uh oh!

papafe commented Dec 12, 2022

Uh oh!

fealebenpae left a comment

Uh oh!

fealebenpae Dec 12, 2022

Uh oh!

papafe Dec 12, 2022

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

nirinchev left a comment

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

papafe Dec 13, 2022

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

papafe Dec 13, 2022

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

nirinchev commented Dec 13, 2022

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

nirinchev Dec 13, 2022

Uh oh!

Uh oh!

Avoid calling setter for bound properties for same value #3129

Avoid calling setter for bound properties for same value #3129

Conversation

papafe commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

papafe commented Dec 12, 2022

Uh oh!

fealebenpae left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirinchev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirinchev commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

papafe commented Dec 12, 2022 •

edited

Loading