thread list loading #7766

fedrunov · 2022-12-12T09:26:49Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Since we have Threads API we should use it to fetch threads list instead of parsing messages locally

Motivation and context

closes #5819

Screenshots / GIFs

Tests

Step 1
Step 2
Step ...

Tested devices

Physical
Emulator
OS version(s):

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()

ElementBot · 2022-12-12T09:28:53Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against ba71291

ElementBot · 2022-12-12T09:29:30Z

	Fails
🚫	`node` failed.

Log

Error:  Error: Could not find the Dangerfile at ./tools/danger/dangerfile-lint.js - does a file exist at danger/dangerfile-lint.js in ./tools?.
    at /usr/src/danger/distribution/platforms/GitHub.js:166:27
    at step (/usr/src/danger/distribution/platforms/GitHub.js:44:23)
    at Object.next (/usr/src/danger/distribution/platforms/GitHub.js:25:53)
    at fulfilled (/usr/src/danger/distribution/platforms/GitHub.js:16:58)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
danger-results://tmp/danger-results-b1e80078.json

Generated by 🚫 dangerJS against e9869ca

ElementBot · 2022-12-12T09:30:45Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against e9869ca

bmarty

Thanks, some remarks.

bmarty · 2022-12-12T09:48:54Z

...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt

-     * Returns a [LiveData] list of all the [ThreadSummary] that exists at the room level.
-     */
-    fun getAllThreadSummariesLive(): LiveData<List<ThreadSummary>>
+    suspend fun getPagedThreadsList(coroutineScope: CoroutineScope, userParticipating: Boolean): LiveData<PagedList<ThreadSummary>>


This is strange to pass a CoroutineScope here. Maybe document the function to add details about the parameters usage?

What is strange is to return a livedata from a suspendable method

bmarty · 2022-12-12T09:56:36Z

.../src/main/java/org/matrix/android/sdk/internal/database/model/threads/ThreadSummaryEntity.kt

@@ -40,5 +40,8 @@ internal open class ThreadSummaryEntity(
    @LinkingObjects("threadSummaries")
    val room: RealmResults<RoomEntity>? = null

+    @LinkingObjects("threadSummaries")
+    val page: RealmResults<ThreadListPageEntity>? = null


Does this addition need to be part of the migration? I do not see it in MigrateSessionTo046. Anyway if this is required, a test will fail.

LinkingObjects are dynamically calculated and so you can't migrate them. I've tested migration by updating app build and it was working ok

bmarty · 2022-12-12T10:04:31Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/threads/DefaultThreadsService.kt

-                    threadSummaryMapper.map(it)
-                }
+    override suspend fun getPagedThreadsList(coroutineScope: CoroutineScope, userParticipating: Boolean): LiveData<PagedList<ThreadSummary>> {
+        return DefaultGetPagedThreadListTask(monarchy, threadSummaryMapper, fetchThreadSummariesTask).execute(


You should inject the task GetPagedThreadListTask in the constructor instead of creating a Default instance here.

That's actually a thing i'd like to discuss. This task is basically a single-use cache, so we need to ensure that if the same task is called again, e.g. when filters are changed, it won't use anything from the previous run. If I'd inject the task it will require to do some cleanup on every execute and could cause problems if anything is not cleared, while running it from the scratch ensure that no previous data remain. Do you think it will be better to inject and do a cleanup?

A Task shouldn't keep data, it's supposed to be a one/off operation

ganfra · 2022-12-12T17:39:47Z

...src/main/java/org/matrix/android/sdk/internal/session/room/threads/GetPagedThreadListTask.kt

+            it.setBoundaryCallback(object : PagedList.BoundaryCallback<ThreadSummary>() {
+                override fun onItemAtEndLoaded(itemAtEnd: ThreadSummary) {
+                    boundaries.postValue(boundaries.value?.copy(endLoaded = true))
+                    coroutineScope?.launch {


We already have some inner coroutineScope from SDK for use cases like that

ElementBot · 2022-12-14T10:10:03Z

	Warnings
⚠️	Please add a changelog. See instructions here
⚠️	Please add a reviewer to your PR.

Generated by 🚫 dangerJS against 7e8859e

ElementBot · 2022-12-14T10:15:07Z

...ndroid/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadLivePageResult.kt

+import org.matrix.android.sdk.api.session.room.ResultBoundaries
+import org.matrix.android.sdk.api.session.room.threads.model.ThreadSummary
+
+data class ThreadLivePageResult (


🚫 Unexpected spacing before "("

ElementBot · 2022-12-14T10:15:07Z

...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt

@@ -17,7 +17,12 @@
 package org.matrix.android.sdk.api.session.room.threads

 import androidx.lifecycle.LiveData


🚫 Unused import

ElementBot · 2022-12-14T10:15:07Z

...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt

@@ -17,7 +17,12 @@
 package org.matrix.android.sdk.api.session.room.threads

 import androidx.lifecycle.LiveData
+import androidx.paging.PagedList
+import kotlinx.coroutines.CoroutineScope


🚫 Unused import

ElementBot · 2022-12-14T10:15:08Z

...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/threads/ThreadsService.kt

@@ -17,7 +17,12 @@
 package org.matrix.android.sdk.api.session.room.threads

 import androidx.lifecycle.LiveData
+import androidx.paging.PagedList
+import kotlinx.coroutines.CoroutineScope
+import org.matrix.android.sdk.api.session.room.ResultBoundaries


🚫 Unused import

ElementBot · 2022-12-14T10:15:08Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/threads/DefaultThreadsService.kt

                }
+            })
+


🚫 Unexpected blank line(s) before "}"

…tyle fixes

ElementBot · 2022-12-14T11:01:54Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against c067213

ElementBot · 2022-12-14T13:03:00Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against fa68956

ElementBot · 2022-12-14T14:50:41Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against f02dd08

ganfra

Please add a changelog and fix the warnings

ElementBot · 2022-12-14T15:34:05Z

	Warnings
⚠️	Please add a reviewer to your PR.

Generated by 🚫 dangerJS against 0c3e238

sonarqubecloud · 2022-12-14T17:33:09Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.4% Coverage
0.0% Duplication

thread list loading

ba71291

fedrunov requested review from bmarty and ganfra December 12, 2022 09:26

page size increased

e9869ca

bmarty reviewed Dec 12, 2022

View reviewed changes

ganfra reviewed Dec 12, 2022

View reviewed changes

ganfra added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Dec 13, 2022

code moved from task to vm

7e8859e

ElementBot reviewed Dec 14, 2022

View reviewed changes

subscribe directly to livedata instead of converting to flow + code s…

c067213

…tyle fixes

fedrunov requested a review from ganfra December 14, 2022 10:58

Merge branch 'develop' into feature/nfe/thread_list_loading

fa68956

changed pagination values

f02dd08

ganfra approved these changes Dec 14, 2022

View reviewed changes

added changelog

0c3e238

fedrunov merged commit cf3abd6 into develop Dec 14, 2022

fedrunov deleted the feature/nfe/thread_list_loading branch December 14, 2022 17:56

		@@ -17,7 +17,12 @@
		package org.matrix.android.sdk.api.session.room.threads

		import androidx.lifecycle.LiveData

thread list loading #7766

thread list loading #7766

Uh oh!

Conversation

fedrunov commented Dec 12, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

ElementBot commented Dec 12, 2022

Uh oh!

ElementBot commented Dec 12, 2022

Log

Uh oh!

ElementBot commented Dec 12, 2022

Uh oh!

bmarty 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElementBot commented Dec 14, 2022

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!

ElementBot commented Dec 14, 2022

Uh oh!

ElementBot commented Dec 14, 2022

Uh oh!

ElementBot commented Dec 14, 2022

Uh oh!

ganfra left a comment

Choose a reason for hiding this comment

Uh oh!

ElementBot commented Dec 14, 2022

Uh oh!

sonarqubecloud bot commented Dec 14, 2022

Uh oh!

Uh oh!