Feature/aris/thread aware #4396

ariskotsomitopoulos · 2021-11-03T09:46:39Z

Make Element Android Thread aware

This feature temporarily until the main threads release and will be deleted. The idea is to make smoother the thread transition release for the clients
Closes #4246

What was the main challenge?
Too old root thread messages, are not fetched in the device so is impossible to show the thread as a reply appropriately. So a custom fetch root thread event mechanism was implemented in order to fetch those root thread events prior the transformation to replies

To treat m.thread the same way that they treat m.in_reply_to
To change the event references from m.in_reply_to to m.thread when replying to an event that has an m.thread relation ( no need for that while it is already supported and displayed correctly in the web clients)

…y old messages that the root thread message is not fetched in the device and initial sync

Add documentation comments

github-actions · 2021-11-03T10:22:22Z

Unit Test Results

  66 files +  4   66 suites +4 58s ⏱️ +10s
135 tests +17 135 ✔️ +17 0 💤 ±0 0 ❌ ±0
418 runs +68 418 ✔️ +68 0 💤 ±0 0 ❌ ±0

Results for commit 8015ffe. ± Comparison against base commit 3760401.

♻️ This comment has been updated with latest results.

bmarty

I have some remarks. I think it would be valuable if @ganfra review this PR too.
I'm wondering why it's mandatory to retrieve the root event during the sync response processing. Can we do it only if the timeline is rendered?

bmarty · 2021-11-03T17:05:41Z

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

@@ -56,10 +57,10 @@ internal open class EventEntity(@Index var eventId: String = "",

    companion object

-    fun setDecryptionResult(result: MXEventDecryptionResult) {
+    fun setDecryptionResult(result: MXEventDecryptionResult, clearEvent: JsonDict? = null) {


This change is strange, let me see the rest of the changes to understand why it is done

bmarty · 2021-11-03T17:07:15Z

...droid/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt

+     * This function is responsible to fetch and store the root event of a thread event
+     * in order to be able to display the event to the user appropriately
+     */
+    private fun fetchRootThreadEventsIfNeeded(offsetResults: RealmResults<TimelineEventEntity>) = runBlocking {


We should not use runBlocking

Correct, but we are already in a thread (and the UI will not freeze) and it's the only way to ensure that the next code block will have the required info, if you can suggest an alternative to that it's welcome. Furthermore keep in mind that this is a temporary fix and will be removed after the thread release

Yes thats not really great but it's ok I guess for now. Note that my rework of the timeline authorise direct usage of suspend methods

...droid/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt

...rc/main/java/org/matrix/android/sdk/internal/session/room/timeline/TimelineEventDecryptor.kt

...in/java/org/matrix/android/sdk/internal/session/sync/handler/room/ThreadsAwarenessHandler.kt

bmarty · 2021-11-03T17:19:31Z

...dk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/SyncResponseHandler.kt

@@ -94,6 +96,10 @@ internal class SyncResponseHandler @Inject constructor(
            Timber.v("Finish handling toDevice in $it ms")
        }
        val aggregator = SyncResponsePostTreatmentAggregator()
+
+        // Prerequisite for thread events handling in RoomSyncHandler
+        threadsAwarenessHandler.fetchRootThreadEventsIfNeeded(syncResponse)


Do we want to do that even for the initial sync? It can take time if there are lots of events to retrieve

Yes, without this it will not be rendered appropriately. I think there will be never more than max 1/2 fetches per room worst case. Ofc this will be removed upon the first thread release.

ariskotsomitopoulos · 2021-11-04T09:37:08Z

I have some remarks. I think it would be valuable if @ganfra review this PR too. I'm wondering why it's mandatory to retrieve the root event during the sync response processing. Can we do it only if the timeline is rendered?

Later in the handler you would need that info, especially for the 10 latest messages.
It's not a big deal while:

User will fetch the root event ONLY once and never again
User will fetch them only if there is a thread response in the last messages

The majority of the fetching will be done when the timeline is rendered and decrypted.

Furthermore, if we go with a different approach we should add loaders on messages and the complexity of that is not worth it while its just a temporarily solution, and we should not spent that much time

ganfra · 2021-11-15T15:25:01Z

...in/java/org/matrix/android/sdk/internal/session/sync/handler/room/ThreadsAwarenessHandler.kt

+        if (eventList.isNullOrEmpty()) return
+
+        val threadsToFetch = emptyMap<String, String>().toMutableMap()
+        monarchy.awaitTransaction { realm ->


Why are u using awaitTransaction here?

ganfra · 2021-11-15T15:26:07Z

...in/java/org/matrix/android/sdk/internal/session/sync/handler/room/ThreadsAwarenessHandler.kt

+        private val permalinkFactory: PermalinkFactory,
+        private val cryptoService: CryptoService,
+        @SessionDatabase private val monarchy: Monarchy,
+        private val session: Session


We shouldn't use session in the SDK this is only intended to be used by SDK user

Updated accordingly

ganfra

Some remarks

bmarty

LGTM, @ganfra ?

...in/java/org/matrix/android/sdk/internal/session/sync/handler/room/ThreadsAwarenessHandler.kt

ganfra · 2021-11-17T17:00:04Z

...in/java/org/matrix/android/sdk/internal/session/sync/handler/room/ThreadsAwarenessHandler.kt

+        return runCatching {
+            Timber.i("------> Fetching event[$eventId]....")
+            getEventTask.execute(GetEventTask.Params(roomId = roomId, eventId = eventId)).apply {
+                unsignedData?.age?.let { System.currentTimeMillis() - it }


Maybe this should be added to the task?

ganfra

Thanks for the changes, ok for me now

ariskotsomitopoulos added 5 commits October 25, 2021 19:00

Thread awareness, map threads events to replies

8f00749

Thread awareness, map threads events to replies

d1f3e3f

Make Android app thread aware. Handling also extreme cases like reall…

45a63b7

…y old messages that the root thread message is not fetched in the device and initial sync

Delete ThreadToReplyMapInterceptor

8ee3f2c

Add documentation comments

Add changelog file

4192c1c

bmarty reviewed Nov 3, 2021

View reviewed changes

bmarty requested a review from ganfra November 3, 2021 17:20

PR Remarks

ec366f1

ganfra reviewed Nov 15, 2021

View reviewed changes

ganfra requested changes Nov 15, 2021

View reviewed changes

ariskotsomitopoulos added 3 commits November 17, 2021 14:14

Use GetEventTask instead of session from the ThreadAwarenessHandler

88656ce

Remove unused import

d463500

Use RealmConfiguration instead of Monarchy

9972dbc

bmarty approved these changes Nov 17, 2021

View reviewed changes

ganfra reviewed Nov 17, 2021

View reviewed changes

...in/java/org/matrix/android/sdk/internal/session/sync/handler/room/ThreadsAwarenessHandler.kt Show resolved Hide resolved

ganfra reviewed Nov 17, 2021

View reviewed changes

PR remrarks

8015ffe

ganfra approved these changes Nov 17, 2021

View reviewed changes

bmarty merged commit e98dd2e into develop Nov 18, 2021

bmarty deleted the feature/aris/thread_aware branch November 18, 2021 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/aris/thread aware #4396

Feature/aris/thread aware #4396

ariskotsomitopoulos commented Nov 3, 2021

github-actions bot commented Nov 3, 2021 •

edited

Loading

bmarty left a comment

bmarty Nov 3, 2021

bmarty Nov 3, 2021

ariskotsomitopoulos Nov 4, 2021

ganfra Nov 15, 2021

bmarty Nov 3, 2021

ariskotsomitopoulos Nov 4, 2021

ariskotsomitopoulos commented Nov 4, 2021

ganfra Nov 15, 2021

ganfra Nov 15, 2021

ariskotsomitopoulos Nov 17, 2021

ganfra left a comment

bmarty left a comment

ganfra Nov 17, 2021

ganfra left a comment

Feature/aris/thread aware #4396

Feature/aris/thread aware #4396

Conversation

ariskotsomitopoulos commented Nov 3, 2021

Make Element Android Thread aware

github-actions bot commented Nov 3, 2021 • edited Loading

Unit Test Results

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariskotsomitopoulos commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganfra left a 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

ganfra left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2021 •

edited

Loading