Replace flattenParents with directParentName #6314

ericdecanini · 2022-06-15T13:01:56Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Replaces flattenParents field in RoomSummary with directParentName to increase performance

Motivation and context

Previously to have the space parent displayed in the search screen, we added the flattenParents field to RoomSummary, but using this was triggering a double mapping of the room summary and its parents thus affecting performance.

To alleviate this, we decided on simply saving the parents name directly in RoomSummaryUpdater. This has little if any impact on performance and allows us to achieve the same goal without double mapping.

Screenshots / GIFs

Search screen space parent subtitles working as previously

Tests

Preferably before doing this, change the build version so a clean install doesn't happen and you can test the migration too

Run the app and see that the search screen displays with spaces showing their space parent (if any) and performance isn't a problem

Tested devices

Physical
Emulator
OS version(s): Android 11

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

…flatten_with_direct_parent

ericdecanini · 2022-06-15T13:04:29Z

...roid/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo030.kt

+    override fun doMigrate(realm: DynamicRealm) {
+        realm.schema.get("RoomSummaryEntity")
+                ?.addField(RoomSummaryEntityFields.DIRECT_PARENT_NAME, String::class.java)
+                ?.transform { it.setString(RoomSummaryEntityFields.DIRECT_PARENT_NAME, "") }


Even if we migrate it as empty here, it seems like it automatically syncs on launch and updates the value with the corresponding space parent of each room

it's updated after each sync. Is there a need to set a default value.

bmarty · 2022-06-15T19:51:26Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/RoomSummary.kt

         */
-        val flattenParents: List<RoomSummary> = emptyList(),
+        val directParentName: String? = null,


Will it be properly updated, if the parent name changes?

Confirmed this one, it properly updates

BillCarsonFr · 2022-06-28T12:21:01Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/RoomSummary.kt

         */
-        val flattenParents: List<RoomSummary> = emptyList(),
+        val directParentName: String? = null,


@ericdecanini this should be a list, you can have several direct parents.

BillCarsonFr

It should be a list of names, not just one. Or you could format it as a coma separated list, but just one is not correct

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo030.kt

…flatten_with_direct_parent

BillCarsonFr

I don't think directParentNames is properly cleared before re-computing hierarchy, so it looks like you are continously adding the names to it.
I don't think I have seen an update on the migration (? or i missed a commit)
Also why only display the last() parent, I think web displays them all

BillCarsonFr · 2022-07-01T15:43:35Z

Also would be nice to add a new test for that

ericdecanini · 2022-07-04T08:40:36Z

I don't think directParentNames is properly cleared before re-computing hierarchy, so it looks like you are continously adding the names to it. I don't think I have seen an update on the migration (? or i missed a commit) Also why only display the last() parent, I think web displays them all

Pushed a change to make it show all direct parents and clear on sync

See MigrateSessionTo032.kt for the migration
As for unit tests, the real unit testable part of this would be the RoomSummaryUpdater, but it needs to be hugely refactored to be testable. Given the size and importance of this class, I think this should be done separately

bmarty

Please check the migration of the DB.

bmarty · 2022-07-04T12:12:17Z

...roid/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo032.kt

+
+    override fun doMigrate(realm: DynamicRealm) {
+        realm.schema.get("RoomSummaryEntity")
+                ?.addField(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, String::class.java)


I think we need a List and not a String here, no? So use addRealmListField. I cannot test the migration since I have another issue with the Auth DB. Will be fixed once develop is been merged to your branch.

Ahh yes! Did addRealmListField initially but lost it during a dev merge

bmarty · 2022-07-04T12:13:17Z

...dk-android/src/main/java/org/matrix/android/sdk/internal/database/model/RoomSummaryEntity.kt

@@ -235,6 +235,8 @@ internal open class RoomSummaryEntity(
            if (value != field) field = value
        }

+    var directParentNames: RealmList<String> = RealmList()


Not a blocker, but If there is no set fun, this could be moved to the constructor.

BillCarsonFr · 2022-07-06T12:19:00Z

vector/src/main/java/im/vector/app/features/home/room/list/RoomSummaryItemFactory.kt

@@ -207,9 +207,9 @@ class RoomSummaryItemFactory @Inject constructor(

    private fun getSearchResultSubtitle(roomSummary: RoomSummary): String {
        val userId = roomSummary.directUserId
-        val spaceName = roomSummary.flattenParents.lastOrNull()?.name
+        val directParent = roomSummary.directParentNames.takeIf { it.isNotEmpty() }?.joinToString()


parity with web https://github.com/matrix-org/matrix-react-sdk/blob/d4a4eeaf63e27ce8146dfc6ef783c819968cca4c/src/utils/i18n-helpers.ts#L23

BillCarsonFr

please ensure parity with web to display parents in results.
and add a test in SpaceHierarchyTest

…flatten_with_direct_parent

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo032.kt # vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilderGroup.kt # vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilderSpace.kt

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt

fedrunov

Looks good except migration tests, not sure if we really need them

fedrunov · 2022-07-19T09:44:18Z

.../src/test/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo033Test.kt

+import org.matrix.android.sdk.internal.database.model.RoomSummaryEntityFields
+import org.matrix.android.sdk.test.fakes.FakeDynamicRealm
+
+internal class MigrateSessionTo033Test {


I think tests for Realm migrations are excessive. Migration classes never change, so there is no need to retest them, but we put extra work on our CI to run this tests

Tests removed

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo033.kt

BillCarsonFr

I added a quick test, so ok now when CI happy

bmarty · 2022-07-20T12:00:39Z

vector/src/main/java/im/vector/app/features/home/room/list/RoomSummaryItemFactory.kt

+            0 -> null
+            1 -> directParentNames.first()
+            2 -> stringProvider.getQuantityString(R.plurals.search_space_parents, 1, directParentNames[0], directParentNames[1])
+            else -> stringProvider.getQuantityString(R.plurals.search_space_parents, size - 1, directParentNames[0], directParentNames.size - 1)


Nit: Could use size instead of directParentNames.size,

bmarty · 2022-07-20T12:00:42Z

vector/src/main/res/values/strings.xml

+    <plurals name="search_space_parents">
+        <item quantity="one">%1$s and %2$s</item>
+        <item quantity="other">%1$s and %2$d others</item>
+    </plurals>


This is not the correct way to handle plurals, it can lead to errors with translations.
Also the resource name is a bit strange to me
You must have something like:

<strings name="search_space_two parents">%1$s and %2$s</string> <plurals name="search_space_parents"> <item quantity="one">%1$s and %2$d other</item> <item quantity="other">%1$s and %2$d others</item> </plurals>

This is managed the same way for room_displayname_two_members if you want to have a look on it.

bmarty

Thanks for the update.
Please fix the conflict by creating a MigrateSessionTo035.kt.

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo034.kt

bmarty · 2022-07-20T15:28:23Z

...roid/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo035.kt

+    override fun doMigrate(realm: DynamicRealm) {
+        realm.schema.get("RoomSummaryEntity")
+                ?.addRealmListField(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, String::class.java)
+                ?.transform { it.setString(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, "") }


You did not test the migration. The app is crashing.
The correct code is

Suggested change

?.transform { it.setString(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, "") }

?.transform { it.setList(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, RealmList("")) }

Swear I tested this. Maybe the way I was testing it didn't actually cause this migration to happen

sonarqubecloud · 2022-07-20T16:44:04Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

2.3% Coverage
0.0% Duplication

ericdecanini added 5 commits June 15, 2022 11:46

Replaces flatten parents with direct parent name in RoomSummary

385720b

Merge remote-tracking branch 'origin/develop' into task/eric/replace_…

d5c5c57

…flatten_with_direct_parent

Fixes migration error

d40c2f8

Fixes wrong user id in search room summary item

7f3325e

Adds changelog file

b5fc0b5

ericdecanini commented Jun 15, 2022

View reviewed changes

ericdecanini marked this pull request as ready for review June 15, 2022 13:04

ericdecanini requested review from BillCarsonFr and fedrunov June 15, 2022 13:04

Fixes lint error

9766b62

bmarty reviewed Jun 15, 2022

View reviewed changes

BillCarsonFr reviewed Jun 28, 2022

View reviewed changes

BillCarsonFr requested changes Jun 28, 2022

View reviewed changes

ericdecanini added 4 commits June 29, 2022 17:05

Fixes post merge error

cb400b6

Changes directParentName to a list

ed3a201

Merge remote-tracking branch 'origin/develop' into task/eric/replace_…

b437996

…flatten_with_direct_parent

ericdecanini requested review from BillCarsonFr and bmarty July 1, 2022 14:52

BillCarsonFr requested changes Jul 1, 2022

View reviewed changes

Adds clearing to directParentNames

e003bc9

ericdecanini requested a review from BillCarsonFr July 4, 2022 09:06

bmarty requested changes Jul 4, 2022

View reviewed changes

Changes migration to be realm list field

98d195c

ericdecanini requested a review from bmarty July 5, 2022 08:36

BillCarsonFr reviewed Jul 6, 2022

View reviewed changes

BillCarsonFr requested changes Jul 6, 2022

View reviewed changes

Changes string format of space parents to maintain parity with web

c6728dd

ericdecanini added 8 commits July 18, 2022 14:48

Fixes import order

1ae5965

Merge remote-tracking branch 'origin/develop' into task/eric/replace_…

67dd250

…flatten_with_direct_parent

Fixes post merge errors

7d29fbc

Fixes copyright lint error

7bb7a08

Fixes copyright lint error

e6475a3

Fixes copyright lint error

bc5c53b

Merge remote-tracking branch 'origin/develop' into task/eric/replace_…

e926871

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt

ericdecanini requested a review from bmarty July 19, 2022 09:17

fedrunov approved these changes Jul 19, 2022

View reviewed changes

ericdecanini and others added 5 commits July 19, 2022 13:49

Deletes migration test

6f9b5c0

Fixes error in RealmSessionStoreMigration

f9580e6

Merge remote-tracking branch 'origin/develop' into task/eric/replace_…

ee941cf

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo033.kt

Adds migrate session to 034

5f6f16d

add quick test for direct parent name

10b1443

BillCarsonFr approved these changes Jul 20, 2022

View reviewed changes

bmarty requested changes Jul 20, 2022

View reviewed changes

Improves plural used in strings

0ad2051

ericdecanini requested a review from bmarty July 20, 2022 13:26

bmarty requested changes Jul 20, 2022

View reviewed changes

ericdecanini added 2 commits July 20, 2022 16:44

Merge remote-tracking branch 'origin/develop' into task/eric/replace_…

9e1235d

…flatten_with_direct_parent # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo034.kt

Adds MigrateSessionTo035

b89df0b

bmarty requested changes Jul 20, 2022

View reviewed changes

Fixes migration crash

3f637ea

ericdecanini requested a review from bmarty July 20, 2022 15:51

bmarty approved these changes Jul 20, 2022

View reviewed changes

ericdecanini enabled auto-merge July 20, 2022 15:59

ericdecanini merged commit a2cf872 into develop Jul 20, 2022

ericdecanini deleted the task/eric/replace_flatten_with_direct_parent branch July 20, 2022 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace flattenParents with directParentName #6314

Replace flattenParents with directParentName #6314

ericdecanini commented Jun 15, 2022 •

edited

Loading

ericdecanini Jun 15, 2022

BillCarsonFr Jun 28, 2022

bmarty Jun 15, 2022

ericdecanini Jul 1, 2022

BillCarsonFr Jun 28, 2022

BillCarsonFr left a comment

BillCarsonFr left a comment

BillCarsonFr commented Jul 1, 2022

ericdecanini commented Jul 4, 2022

bmarty left a comment

bmarty Jul 4, 2022

ericdecanini Jul 5, 2022

bmarty Jul 4, 2022

BillCarsonFr Jul 6, 2022

BillCarsonFr left a comment

fedrunov left a comment

fedrunov Jul 19, 2022

ericdecanini Jul 19, 2022

BillCarsonFr left a comment

bmarty Jul 20, 2022

bmarty Jul 20, 2022

bmarty left a comment

bmarty Jul 20, 2022

ericdecanini Jul 20, 2022

sonarqubecloud bot commented Jul 20, 2022

	?.transform { it.setString(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, "") }
	?.transform { it.setList(RoomSummaryEntityFields.DIRECT_PARENT_NAMES.`$`, RealmList("")) }

Replace flattenParents with directParentName #6314

Replace flattenParents with directParentName #6314

Conversation

ericdecanini commented Jun 15, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

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

BillCarsonFr left a comment

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

BillCarsonFr commented Jul 1, 2022

ericdecanini commented Jul 4, 2022

bmarty 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

BillCarsonFr left a comment

Choose a reason for hiding this comment

fedrunov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 20, 2022

ericdecanini commented Jun 15, 2022 •

edited

Loading