Create `MailboxLocalValues` class to stop forgetting to pass values in `initLocalValues()` #2058

KevinBoulongne · 2024-09-30T12:28:23Z

Depends on #2059

LunarX · 2024-09-30T13:06:09Z

app/src/main/java/com/infomaniak/mail/data/models/mailbox/Mailbox.kt

+    fun setFeatureFlags(featureFlags: List<String>) {
+        mailboxLocalValues = local.copyFromRealm().also {
+            it.featureFlags.setFeatureFlags(featureFlags)
+        }


Can we change isBimiEnabledLiveData's query so it directly queries the feature flags? This way we might be able to avoid the copyFromRealm in this setFeatureFlags method if it's only used to trigger the live data that observes the realm query

We can't, it's an EmbeddedObject, we need to use its parent (ie. the Mailbox).

LunarX · 2024-09-30T13:19:33Z

app/src/main/java/com/infomaniak/mail/ui/MainViewModel.kt

+                    it.local.externalMailFlagEnabled = externalMailInfo.externalMailFlagEnabled
+                    it.local.trustedDomains = externalMailInfo.trustedDomains.toRealmList()


Is it worth bothering to only update the local values' object rather than updating all of the mailbox object in realm? Because here the only values we updates are inside the local values, not inside the whole mailbox.

In this part of the code we have multiple spot where we modify the realm object the same way we do it for the local.featureFlags where we needed a copyFromRealm(). If we cannot remove the copyFromRealm() will we also need it in these places here?

Updating like it's done here will correctly update data in Realm, but won't trigger any Flow if it's listening to the Mailbox, because the Mailbox itself won't be updated, only it's embeddedObject.

If we want to immediately broadcast this change to any Flow listening to this Mailbox, we need to update the data like we did in the other places, with the copyFromRealm() method.

Maybe it's better to always update it with copyFromRealm(), to be sure we don't miss it.
But I don't really like this idea. It will complicate the code everywhere (remember that we need to do this change for every Realm object that has a initLocalValues() function).

I'm starting to think it's better to drop this PR instead of complicating the code and risking to forget to update correctly.
The issue was that we could forget some parameters in initLocalValues(), and now the issue is that we can forget to correctly broadcast values updates.
The issue is the same, but somewhere else and with more code complexity.

If this doesn't work, it indeed introduces a new equivalent problem. It'd be even worse at that.

But I'd be very surprised if Realm didn't create a way to listen to items in a DB and get notified when they change even in this situation. We should talk about it in person and maybe even talk about it with @sirambd

LunarX · 2024-09-30T14:14:03Z

app/src/main/java/com/infomaniak/mail/data/cache/mailboxInfo/SignatureController.kt

@@ -31,7 +31,7 @@ class SignatureController @Inject constructor(

    //region Get data
    fun getSignaturesAsync(mailboxObjectId: String): Flow<RealmList<Signature>> {
-        return mailboxController.getMailboxAsync(mailboxObjectId).mapNotNull { it.obj?.signatures }
+        return mailboxController.getMailboxAsync(mailboxObjectId).mapNotNull { it.obj?.local?.signatures }


Is it worth it to query the whole mailbox when we only need the local values' object? (This pattern appears in other controllers as well)

This is an EmbeddedObject, we can't query it directly.

What? I asked @sirambd because I feared this exact situation and he told me we could. Maybe he did not understand what I meant

We can query it, but not directly.
There is no PrimaryKey to search for since it's an EmbeddedObject.

As long as we can query it indirectly by checking its parent's primary key, it should be enough. But I don't know to what extent this is feasible

LunarX · 2024-09-30T14:30:52Z

app/src/main/java/com/infomaniak/mail/data/cache/mailboxInfo/MailboxController.kt

@@ -99,19 +101,10 @@ class MailboxController @Inject constructor(
            val remoteMailboxesIds = remoteMailboxes.map { remoteMailbox ->

                SentryLog.d(RealmDatabase.TAG, "Mailboxes: Get current data")
-                val localMailbox = getMailbox(userId, remoteMailbox.mailboxId, realm = this)
+                val previousLocalValues = getMailbox(userId, remoteMailbox.mailboxId, realm = this)?.local?.copyFromRealm()


What's the reason that makes this copyFromRealm() required? How does it fix the bimi issue? Because I'm not sure I understand correctly how copyFromRealm() should properly be used

We are replacing the mailboxLocalValues with another one that's coming from Realm.
We need to copyFromRealm() it if we want it to work.

Yeah I figured we update the value here and apparently copyFromRealm() seems to be needed, but why? How does it work? What does it bring to the table? Why does it work? What's missing if we don't use it? How does it fix the bimi issue?

You need to copy the value from Realm to be able to put it back in Realm later.
If we don't, Realm crashes, that's all.

LunarX · 2024-10-01T06:08:17Z

app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageAiManager.kt

@@ -204,7 +204,7 @@ class NewMessageAiManager @Inject constructor(

    fun observeAiFeatureFlagUpdates() {
        newMessageViewModel.currentMailboxLive.observeNotNull(viewLifecycleOwner) { mailbox ->
-            val isAiEnabled = mailbox.featureFlags.contains(FeatureFlag.AI)
+            val isAiEnabled = mailbox.local.featureFlags.contains(FeatureFlag.AI)


This live data is only ever used to get the current mailbox to now if its local values has the ai flag, would it make sens to replace it with a live data that only returns ai's enabled status? You can still keep the currentMailboxLive as a private live data and map it in a second public isAiEnabled live data or something I guess. Or directly query the local values from realm without querying the whole mailbox if it makes sens

This is an EmbeddedObject, we can't query it directly.

We can expose a LiveData that's named about the AI and not the Mailbox, if you want.
But that's not relevant for this PR.

github-actions · 2024-10-04T07:18:39Z

This PR/issue depends on:

~~Simplify updateMailboxes() code #2059~~
By Dependent Issues (🤖). Happy coding!

…n `initLocalValues()`

…update them

sonarcloud · 2024-10-10T09:10:53Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions bot assigned KevinBoulongne Sep 30, 2024

KevinBoulongne changed the base branch from master to simplify-code September 30, 2024 12:47

github-actions bot added the dependent This MR depends on another PR label Sep 30, 2024

KevinBoulongne force-pushed the stop-forgetting-local-values branch from 8e192fd to 2d9e505 Compare September 30, 2024 12:50

LunarX self-requested a review September 30, 2024 13:00

LunarX marked this pull request as ready for review September 30, 2024 13:01

LunarX requested changes Sep 30, 2024

View reviewed changes

LunarX requested changes Oct 1, 2024

View reviewed changes

KevinBoulongne force-pushed the simplify-code branch from 9f489ce to 4a94b50 Compare October 1, 2024 06:26

KevinBoulongne force-pushed the stop-forgetting-local-values branch from 2d9e505 to a2c0105 Compare October 1, 2024 06:26

KevinBoulongne marked this pull request as draft October 1, 2024 07:31

KevinBoulongne marked this pull request as ready for review October 1, 2024 07:47

KevinBoulongne marked this pull request as draft October 1, 2024 08:52

KevinBoulongne force-pushed the simplify-code branch from 4a94b50 to 537a377 Compare October 1, 2024 08:55

KevinBoulongne force-pushed the stop-forgetting-local-values branch 3 times, most recently from b1657cb to a96b542 Compare October 1, 2024 12:38

KevinBoulongne force-pushed the simplify-code branch 2 times, most recently from 7d716d7 to cde8f48 Compare October 2, 2024 11:45

KevinBoulongne force-pushed the stop-forgetting-local-values branch from a96b542 to 0c838f3 Compare October 2, 2024 11:45

KevinBoulongne force-pushed the simplify-code branch from cde8f48 to 32475d2 Compare October 3, 2024 14:34

Base automatically changed from simplify-code to master October 4, 2024 07:18

github-actions bot removed the dependent This MR depends on another PR label Oct 4, 2024

KevinBoulongne force-pushed the stop-forgetting-local-values branch 4 times, most recently from 98d549c to 13066e0 Compare October 8, 2024 13:47

LunarX and others added 2 commits October 10, 2024 10:33

Create MailboxLocalValues class to stop forgetting to pass values i…

17b2db5

…n `initLocalValues()`

Use MailboxLocalValues class

f307c16

KevinBoulongne added 6 commits October 10, 2024 10:35

Handle Realm migration

b036ec8

Use property name to access mailboxLocalValues

8a89d9b

Remove incorrect code region

290366e

Simplify updateMailboxes() code

f188a93

Correctly copyFromRealm() the mailboxLocalValues when we want to …

44a7a32

…update them

Use copyFromRealm() for all MailboxLocalValues updates

774e409

KevinBoulongne force-pushed the stop-forgetting-local-values branch from 13066e0 to 774e409 Compare October 10, 2024 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `MailboxLocalValues` class to stop forgetting to pass values in `initLocalValues()` #2058

Create `MailboxLocalValues` class to stop forgetting to pass values in `initLocalValues()` #2058

KevinBoulongne commented Sep 30, 2024 •

edited

Loading

LunarX Sep 30, 2024

KevinBoulongne Oct 1, 2024 •

edited

Loading

LunarX Sep 30, 2024

LunarX Sep 30, 2024

KevinBoulongne Oct 1, 2024 •

edited

Loading

LunarX Oct 4, 2024

LunarX Sep 30, 2024

KevinBoulongne Oct 1, 2024

LunarX Oct 1, 2024 •

edited

Loading

KevinBoulongne Oct 1, 2024 •

edited

Loading

LunarX Oct 1, 2024

LunarX Sep 30, 2024

KevinBoulongne Oct 1, 2024

LunarX Oct 1, 2024 •

edited

Loading

KevinBoulongne Oct 1, 2024

LunarX Oct 1, 2024

KevinBoulongne Oct 1, 2024 •

edited

Loading

github-actions bot commented Oct 4, 2024

sonarcloud bot commented Oct 10, 2024

		it.local.externalMailFlagEnabled = externalMailInfo.externalMailFlagEnabled
		it.local.trustedDomains = externalMailInfo.trustedDomains.toRealmList()

Create MailboxLocalValues class to stop forgetting to pass values in initLocalValues() #2058

Are you sure you want to change the base?

Create MailboxLocalValues class to stop forgetting to pass values in initLocalValues() #2058

Conversation

KevinBoulongne commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

KevinBoulongne Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinBoulongne Oct 1, 2024 • edited Loading

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

LunarX Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

KevinBoulongne Oct 1, 2024 • edited Loading

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

LunarX Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinBoulongne Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2024

sonarcloud bot commented Oct 10, 2024

Quality Gate passed

Create `MailboxLocalValues` class to stop forgetting to pass values in `initLocalValues()` #2058

Create `MailboxLocalValues` class to stop forgetting to pass values in `initLocalValues()` #2058

KevinBoulongne commented Sep 30, 2024 •

edited

Loading

KevinBoulongne Oct 1, 2024 •

edited

Loading

KevinBoulongne Oct 1, 2024 •

edited

Loading

LunarX Oct 1, 2024 •

edited

Loading

KevinBoulongne Oct 1, 2024 •

edited

Loading

LunarX Oct 1, 2024 •

edited

Loading

KevinBoulongne Oct 1, 2024 •

edited

Loading