Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Live Threads #5298

Merged
merged 47 commits into from
Mar 15, 2022
Merged

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Feb 22, 2022

This PR closes #5230 closes #5232 closes #5271 closes #5231 closes #5116

Live Thread Summaries

Currently in threads P0 release we can view all the threads within a room but only for the received messages from the main timeline (it is also not working when there are gaps). This PR uses the enhanced /messages API from MSC3440 to dynamically fetch the latest root threads along with their summary for each room. It will also support edited messages (on root message or thread summary)

Thread Summaries Screenshot_20220222-143424_Element dbg

Live Thread Timeline

Currently in threads P0 release when you open a thread will paginate through /messages api exactly like the main timeline will do. This approach work but there will be a problem with large gaps or large threads, it will try fetch all the messages even when the messages are not from that thread in order to display them. This PR uses the /relations API to fetch and paginate the thread timeline. It will also handle appropriately the already existing events from the main timeline, user will be able to interact smoothly (reactions etc) with them even if the same event has already arrived.

Home Server Capabilities for threads

Some home servers may not support the MSC3440 so we will not be able to use the enhanced /messages api to fetch the thread summaries we described above. This PR will reuse the existing logic like in P0 release in those home servers by using the capabilities api

# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt
# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo025.kt
… MSC 3440

Add capabilities to support local thread list to not supported servers
# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt
@github-actions
Copy link

github-actions bot commented Feb 22, 2022

Unit Test Results

  98 files  ±0    98 suites  ±0   1m 3s ⏱️ -16s
180 tests +1  180 ✔️ +1  0 💤 ±0  0 ±0 
590 runs  +2  590 ✔️ +2  0 💤 ±0  0 ±0 

Results for commit 4d76c0d. ± Comparison against base commit 1a76914.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed=
  • [org.matrix.android.sdk.account]
    passed=
  • [org.matrix.android.sdk.internal]
    passed=
  • [org.matrix.android.sdk.ordering]
    passed=
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed=

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a huge PR. I have only 1 quick remark.
I would defer to @ganfra for a review about the change SDK side.

@@ -199,6 +200,8 @@ fun RoomMemberSummary.toMatrixItem() = MatrixItem.UserItem(userId, displayName,

fun SenderInfo.toMatrixItem() = MatrixItem.UserItem(userId, disambiguatedDisplayName, avatarUrl)

fun SenderInfo.toMatrixItemOrNull() = tryOrNull { MatrixItem.UserItem(userId, disambiguatedDisplayName, avatarUrl) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, what was the problem with SenderInfo.toMatrixItem() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was really rare but sometimes something was wrong with the servers userId (from a local homeserver). Null Check fixed the issue.

    protected fun checkId() {
        if (!id.startsWith(getIdPrefix())) {
            error("Wrong usage of MatrixItem: check the id $id should start with ${getIdPrefix()}")
        }
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really like this change TBH...

Copy link
Contributor

@ouchadam ouchadam Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the change something we could avoid by updating the local synapse script? (assuming live homeservers can't encounter the same problem)

}
}
elementRobot.toggleLabFeature(LabFeature.THREAD_MESSAGES)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a test here!

@bmarty bmarty requested a review from ganfra February 24, 2022 14:00
ganfra
ganfra previously requested changes Mar 3, 2022
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Some remarks/questions

# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/threads/FetchThreadTimelineTask.kt
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/RoomSyncHandler.kt
Enhance thread awareness handler so normal replies with thread disabled will be visible in te appropriate thread
Fix conflicts
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more remarks, else LGTM.
We should probably merge this this week to be able to test a bit before the next release.

@@ -24,4 +24,5 @@ interface RelationContent {
val eventId: String?
val inReplyTo: ReplyToContent?
val option: Int?
val isFallingBack: Boolean? // Thread fallback to differentiate replies within threads
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the form I would use a javadoc. I do not like comment after the code

So something like

/**
 * doc...
 */
val isFallingBack: Boolean?

-> more readable and better integration with the IDE.

Also can you document a bit more what is the purpose of this val? What value true or false mean for this boolean? I have to confess that the name isFallingBack means nothing to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag, was a last minute change that I was not aware about. More details are here. I will add a more precise comment

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome stuff! 👍 for merging and testing on develop

@ariskotsomitopoulos ariskotsomitopoulos removed the request for review from ganfra March 15, 2022 10:28
# Conflicts:
#	vector/src/main/java/im/vector/app/features/home/room/detail/timeline/format/DisplayableEventFormatter.kt
@ariskotsomitopoulos ariskotsomitopoulos dismissed ganfra’s stale review March 15, 2022 13:25

Changes are applied, and we need to merge the feature

@ariskotsomitopoulos ariskotsomitopoulos merged commit e0b93c2 into develop Mar 15, 2022
@ariskotsomitopoulos ariskotsomitopoulos deleted the feature/aris/thread_live_thread_list branch March 15, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment