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

Fixes myroomnick changing Display Name #5618

Merged
merged 24 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bbfefbf
Removes user entity change during room nickname change
ericdecanini Mar 23, 2022
4c1e651
Adds changelog file
ericdecanini Mar 23, 2022
927eeb3
Refactors RoomMemberEventHandler with change to only save the user lo…
ericdecanini Mar 30, 2022
0200f48
Fixes lint error
ericdecanini Mar 30, 2022
6c1f6dc
Uses aggregator to handle room name change events
ericdecanini Mar 31, 2022
7f64c62
Adds missing room member update call during incremental syncs
ericdecanini Mar 31, 2022
25ca684
Improves code reuse and readability
ericdecanini Apr 6, 2022
c650843
Merge remote-tracking branch 'origin/develop' into bugfix/eric/wrong-…
ericdecanini Apr 7, 2022
981d157
Fixes errors in RoomSyncHandler
ericdecanini Apr 7, 2022
c1b1386
Replaces MatrixItem fromJson with User.toMatrixItem
ericdecanini Apr 7, 2022
6c3c309
Combines public initial and incremental handle sync methods into one
ericdecanini Apr 7, 2022
304e2f1
Improves code readability
ericdecanini Apr 7, 2022
669af4d
Removes unneeded method
ericdecanini Apr 7, 2022
4465525
Fixes import error
ericdecanini Apr 7, 2022
de8cc07
Removes unused import
ericdecanini Apr 7, 2022
b88eaf1
Removes usage of MatrixItem.UserItem internally
ericdecanini Apr 8, 2022
e80386b
Removes clearing usersToFetch in aggregator
ericdecanini Apr 13, 2022
f103d46
Adds exception safety to fetch users and makes it a single transaction
ericdecanini Apr 13, 2022
5d5a06b
Changes null check timing when fetching users
ericdecanini Apr 13, 2022
2337b18
Reorders val declaration of eventUserId and roomMember
ericdecanini Apr 13, 2022
8964b55
Adds empty check before saving user entities
ericdecanini Apr 13, 2022
5f949e9
Reformats not empty check
ericdecanini Apr 13, 2022
dd988d0
Simplifies handleIncrementalSync logic
ericdecanini Apr 13, 2022
8b80cf0
Restores debug log
ericdecanini Apr 13, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5618.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes display name being changed when using /myroomnick
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package org.matrix.android.sdk.api.session.user.model

import org.matrix.android.sdk.api.session.profile.ProfileService
import org.matrix.android.sdk.api.util.JsonDict

/**
* Data class which holds information about a user.
* It can be retrieved with [org.matrix.android.sdk.api.session.user.UserService]
Expand All @@ -27,4 +30,14 @@ data class User(
*/
val displayName: String? = null,
val avatarUrl: String? = null
)
) {

companion object {

fun fromJson(userId: String, json: JsonDict) = User(
userId = userId,
displayName = json[ProfileService.DISPLAY_NAME_KEY] as? String,
avatarUrl = json[ProfileService.AVATAR_URL_KEY] as? String
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ sealed class MatrixItem(
override val displayName: String? = null,
override val avatarUrl: String? = null) :
MatrixItem(id, displayName?.removeSuffix(IRC_PATTERN), avatarUrl) {

init {
if (BuildConfig.DEBUG) checkId()
}
Expand Down Expand Up @@ -200,7 +201,7 @@ fun RoomMemberSummary.toMatrixItem() = MatrixItem.UserItem(userId, displayName,

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

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

fun SpaceChildInfo.toMatrixItem() = if (roomType == RoomType.SPACE) {
MatrixItem.SpaceItem(childRoomId, name ?: canonicalAlias, avatarUrl)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ internal class DefaultLoadRoomMembersTask @Inject constructor(
eventId = roomMemberEvent.eventId
root = eventEntity
}
roomMemberEventHandler.handle(realm, roomId, roomMemberEvent)
roomMemberEventHandler.handle(realm, roomId, roomMemberEvent, false)
}
roomEntity.membersLoadStatus = RoomMembersLoadStatusType.LOADED
roomSummaryUpdater.update(realm, roomId, updateMembers = true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,49 @@ internal class RoomMemberEventHandler @Inject constructor(
@UserId private val myUserId: String
) {

fun handle(realm: Realm, roomId: String, event: Event, aggregator: SyncResponsePostTreatmentAggregator? = null): Boolean {
fun handle(realm: Realm,
roomId: String,
event: Event,
isInitialSync: Boolean,
aggregator: SyncResponsePostTreatmentAggregator? = null): Boolean {
if (event.type != EventType.STATE_ROOM_MEMBER) {
return false
}
val userId = event.stateKey ?: return false
val roomMember = event.getFixedRoomMemberContent()
return handle(realm, roomId, userId, roomMember, aggregator)
val eventUserId = event.stateKey ?: return false
Copy link
Member

Choose a reason for hiding this comment

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

Why those 2 lines of code have been reordered? I think it's better to restore the original order since it's faster to first check for the stateKey.

val roomMember = event.getFixedRoomMemberContent() ?: return false

return if (isInitialSync) {
handleInitialSync(realm, roomId, myUserId, eventUserId, roomMember, aggregator)
} else {
handleIncrementalSync(
realm,
roomId,
eventUserId,
roomMember,
event.resolvedPrevContent(),
aggregator
)
}
}

fun handle(realm: Realm,
roomId: String,
userId: String,
roomMember: RoomMemberContent?,
aggregator: SyncResponsePostTreatmentAggregator? = null): Boolean {
if (roomMember == null) {
return false
private fun handleInitialSync(realm: Realm,
Copy link
Member

Choose a reason for hiding this comment

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

Having two methods with the same name, one public, one private is not ideal to me.
That's also a reason to keep only one public handle method.

roomId: String,
currentUserId: String,
eventUserId: String,
roomMember: RoomMemberContent,
aggregator: SyncResponsePostTreatmentAggregator?): Boolean {
if (currentUserId != eventUserId) {
saveUserEntityLocallyIfNecessary(realm, eventUserId, roomMember)
}
saveRoomMemberEntityLocally(realm, roomId, eventUserId, roomMember)
updateDirectChatsIfNecessary(roomId, roomMember, aggregator)
return true
}

private fun saveRoomMemberEntityLocally(realm: Realm,
roomId: String,
userId: String,
roomMember: RoomMemberContent) {
val roomMemberEntity = RoomMemberEntityFactory.create(
roomId,
userId,
Expand All @@ -58,26 +84,62 @@ internal class RoomMemberEventHandler @Inject constructor(
// but we want to preserve presence record value and not replace it with null
getExistingPresenceState(realm, roomId, userId))
realm.insertOrUpdate(roomMemberEntity)
}

/**
* Get the already existing presence state for a specific user & room in order NOT to be replaced in RoomMemberSummaryEntity
* by NULL value.
*/
private fun getExistingPresenceState(realm: Realm, roomId: String, userId: String): UserPresenceEntity? {
return RoomMemberSummaryEntity.where(realm, roomId, userId).findFirst()?.userPresenceEntity
}

private fun saveUserEntityLocallyIfNecessary(realm: Realm,
userId: String,
roomMember: RoomMemberContent) {
if (roomMember.membership.isActive()) {
val userEntity = UserEntityFactory.create(userId, roomMember)
realm.insertOrUpdate(userEntity)
saveUserLocally(realm, userId, roomMember)
}
}

private fun saveUserLocally(realm: Realm, userId: String, roomMember: RoomMemberContent) {
val userEntity = UserEntityFactory.create(userId, roomMember)
realm.insertOrUpdate(userEntity)
}

private fun updateDirectChatsIfNecessary(roomId: String,
roomMember: RoomMemberContent,
aggregator: SyncResponsePostTreatmentAggregator?) {
// check whether this new room member event may be used to update the directs dictionary in account data
// this is required to handle correctly invite by email in DM
val mxId = roomMember.thirdPartyInvite?.signed?.mxid
if (mxId != null && mxId != myUserId) {
aggregator?.directChatsToCheck?.put(roomId, mxId)
}
return true
}

/**
* Get the already existing presence state for a specific user & room in order NOT to be replaced in RoomMemberSummaryEntity
* by NULL value.
*/
private fun handleIncrementalSync(realm: Realm,
roomId: String,
eventUserId: String,
roomMember: RoomMemberContent,
prevContent: Map<String, Any>?,
Copy link
Member

Choose a reason for hiding this comment

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

Could be of type Content?

aggregator: SyncResponsePostTreatmentAggregator?): Boolean {
val previousDisplayName = prevContent?.get("displayname")
val previousAvatar = prevContent?.get("avatar_url")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add as? String on those 2 lines, so isDifferentFrom can just be removed and replaced by !=, WDYT?


private fun getExistingPresenceState(realm: Realm, roomId: String, userId: String): UserPresenceEntity? {
return RoomMemberSummaryEntity.where(realm, roomId, userId).findFirst()?.userPresenceEntity
if (previousDisplayName.isDifferentFrom(roomMember.displayName) ||
previousAvatar.isDifferentFrom(roomMember.avatarUrl)) {
aggregator?.userIdsToFetch?.add(eventUserId)
Copy link
Member

Choose a reason for hiding this comment

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

If aggregator is null, there is no need to do all this treatment, maybe replace by:

        if (aggregator != null) {
            val previousDisplayName = prevContent?.get("displayname") as? String
            val previousAvatar = prevContent?.get("avatar_url") as? String

            if (previousDisplayName != roomMember.displayName ||
                    previousAvatar != roomMember.avatarUrl) {
                aggregator.userIdsToFetch.add(eventUserId)
            }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this and it does work!

}

saveRoomMemberEntityLocally(realm, roomId, eventUserId, roomMember)
// At the end of the sync, fetch all the profiles from the aggregator
updateDirectChatsIfNecessary(roomId, roomMember, aggregator)
return true
}

private fun Any?.isDifferentFrom(value: Any?) = when {
this == null || this == value -> false
else -> true
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@ internal class SyncResponsePostTreatmentAggregator {

// Map of roomId to directUserId
val directChatsToCheck = mutableMapOf<String, String>()

// List of userIds to fetch and update at the end of incremental syncs
val userIdsToFetch = mutableListOf<String>()
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 the renaming :)

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,31 @@

package org.matrix.android.sdk.internal.session.sync.handler

import com.zhuinden.monarchy.Monarchy
import org.matrix.android.sdk.api.MatrixPatterns
import org.matrix.android.sdk.api.extensions.tryOrNull
import org.matrix.android.sdk.api.session.user.model.User
import org.matrix.android.sdk.internal.di.SessionDatabase
import org.matrix.android.sdk.internal.session.profile.GetProfileInfoTask
import org.matrix.android.sdk.internal.session.sync.RoomSyncEphemeralTemporaryStore
import org.matrix.android.sdk.internal.session.sync.SyncResponsePostTreatmentAggregator
import org.matrix.android.sdk.internal.session.sync.model.accountdata.toMutable
import org.matrix.android.sdk.internal.session.user.UserEntityFactory
import org.matrix.android.sdk.internal.session.user.accountdata.DirectChatsHelper
import org.matrix.android.sdk.internal.session.user.accountdata.UpdateUserAccountDataTask
import javax.inject.Inject

internal class SyncResponsePostTreatmentAggregatorHandler @Inject constructor(
private val directChatsHelper: DirectChatsHelper,
private val ephemeralTemporaryStore: RoomSyncEphemeralTemporaryStore,
private val updateUserAccountDataTask: UpdateUserAccountDataTask
private val updateUserAccountDataTask: UpdateUserAccountDataTask,
private val getProfileInfoTask: GetProfileInfoTask,
@SessionDatabase private val monarchy: Monarchy,
) {
suspend fun handle(synResHaResponsePostTreatmentAggregator: SyncResponsePostTreatmentAggregator) {
cleanupEphemeralFiles(synResHaResponsePostTreatmentAggregator.ephemeralFilesToDelete)
updateDirectUserIds(synResHaResponsePostTreatmentAggregator.directChatsToCheck)
suspend fun handle(aggregator: SyncResponsePostTreatmentAggregator) {
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 having renamed this parameter which was quite typoed :)

cleanupEphemeralFiles(aggregator.ephemeralFilesToDelete)
updateDirectUserIds(aggregator.directChatsToCheck)
fetchAndUpdateUsers(aggregator.userIdsToFetch)
}

private fun cleanupEphemeralFiles(ephemeralFilesToDelete: List<String>) {
Expand Down Expand Up @@ -59,13 +68,33 @@ internal class SyncResponsePostTreatmentAggregatorHandler @Inject constructor(
}

// remove roomId from currentDirectUserId entry
hasUpdate = hasUpdate or(directChats[currentDirectUserId]?.remove(roomId) == true)
hasUpdate = hasUpdate or (directChats[currentDirectUserId]?.remove(roomId) == true)
// remove currentDirectUserId entry if there is no attached room anymore
hasUpdate = hasUpdate or(directChats.takeIf { it[currentDirectUserId].isNullOrEmpty() }?.remove(currentDirectUserId) != null)
hasUpdate = hasUpdate or (directChats.takeIf { it[currentDirectUserId].isNullOrEmpty() }?.remove(currentDirectUserId) != null)
}
}
if (hasUpdate) {
updateUserAccountDataTask.execute(UpdateUserAccountDataTask.DirectChatParams(directMessages = directChats))
}
}

private suspend fun fetchAndUpdateUsers(userIdsToFetch: List<String>) {
fetchUsers(userIdsToFetch)
.takeIf { it.isNotEmpty() }
?.saveLocally()
}

private suspend fun fetchUsers(userIdsToFetch: List<String>) = userIdsToFetch.mapNotNull {
tryOrNull {
val profileJson = getProfileInfoTask.execute(GetProfileInfoTask.Params(it))
Copy link
Member

Choose a reason for hiding this comment

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

I would have put the tryOrNull just on this line to avoid skipping all the treatment in the case of only one request is failing. You can then use mapNotNull to filter out the errors.

User.fromJson(it, profileJson)
}
}

private fun List<User>.saveLocally() {
val userEntities = map { user -> UserEntityFactory.create(user) }
monarchy.doWithRealm {
it.insertOrUpdate(userEntities)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It's good to split code into several smaller methods, but it's maybe harder to read in this particular case.

What about replacing those 3 methods with a single one:

    private suspend fun fetchAndUpdateUsers(usersToFetch: List<String>) {
        usersToFetch
                .map { userId ->
                    val profileJson = profileService.getProfile(userId)
                    User.fromJson(userId, profileJson)
                }
                .map { user -> UserEntityFactory.create(user) }
                .let { userEntities ->
                    monarchy.doWithRealm {
                        it.insertOrUpdate(userEntities)
                    }
                }
    }

Also has the advantage to do on single Realm transaction instead of N transactions with your code.

WDYT?

Copy link
Contributor Author

@ericdecanini ericdecanini Apr 13, 2022

Choose a reason for hiding this comment

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

I personally prefer it split. The advantage is you can get the whole context of the action from the first method, and only if you're concerned about the details of the implementation do you need to read through the bottom methods.

I agree with making it a single Realm transaction though. I'll push changes to make this happen

Copy link
Member

Choose a reason for hiding this comment

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

Your latest implementation is better, thanks. Maybe just skip the transaction if the list is empty, which will be the case nearly all the time, or in case of error when fetching the profile.

}
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
syncLocalTimestampMillis: Long,
aggregator: SyncResponsePostTreatmentAggregator): RoomEntity {
Timber.v("Handle join sync for room $roomId")
Copy link
Member

Choose a reason for hiding this comment

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

This comment is useful when debugging sync request. Can you restore it please?

val isInitialSync = insertType == EventInsertType.INITIAL_SYNC

val ephemeralResult = (roomSync.ephemeral as? LazyRoomSyncEphemeral.Parsed)
?._roomSyncEphemeral
Expand Down Expand Up @@ -241,7 +242,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
}
// Give info to crypto module
cryptoService.onStateEvent(roomId, event)
roomMemberEventHandler.handle(realm, roomId, event, aggregator)
roomMemberEventHandler.handle(realm, roomId, event, isInitialSync, aggregator)
}
}
if (roomSync.timeline?.events?.isNotEmpty() == true) {
Expand Down Expand Up @@ -282,7 +283,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
roomSync: InvitedRoomSync,
insertType: EventInsertType,
syncLocalTimestampMillis: Long): RoomEntity {
Timber.v("Handle invited sync for room $roomId")
Copy link
Member

Choose a reason for hiding this comment

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

Ah I miss that at the previous review. Same remark:
This comment is useful when debugging sync request. Can you restore it please?

val isInitialSync = insertType == EventInsertType.INITIAL_SYNC
val roomEntity = RoomEntity.getOrCreate(realm, roomId)
roomEntity.membership = Membership.INVITE
if (roomSync.inviteState != null && roomSync.inviteState.events.isNotEmpty()) {
Expand All @@ -296,7 +297,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
eventId = eventEntity.eventId
root = eventEntity
}
roomMemberEventHandler.handle(realm, roomId, event)
roomMemberEventHandler.handle(realm, roomId, event, isInitialSync)
}
}
val inviterEvent = roomSync.inviteState?.events?.lastOrNull {
Expand All @@ -312,6 +313,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
roomSync: RoomSync,
insertType: EventInsertType,
syncLocalTimestampMillis: Long): RoomEntity {
val isInitialSync = insertType == EventInsertType.INITIAL_SYNC
val roomEntity = RoomEntity.getOrCreate(realm, roomId)
for (event in roomSync.state?.events.orEmpty()) {
if (event.eventId == null || event.stateKey == null || event.type == null) {
Expand All @@ -323,7 +325,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
eventId = event.eventId
root = eventEntity
}
roomMemberEventHandler.handle(realm, roomId, event)
roomMemberEventHandler.handle(realm, roomId, event, isInitialSync)
}
for (event in roomSync.timeline?.events.orEmpty()) {
if (event.eventId == null || event.senderId == null || event.type == null) {
Expand All @@ -337,7 +339,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
root = eventEntity
}
if (event.type == EventType.STATE_ROOM_MEMBER) {
roomMemberEventHandler.handle(realm, roomEntity.roomId, event)
roomMemberEventHandler.handle(realm, roomEntity.roomId, event, isInitialSync)
}
}
}
Expand Down Expand Up @@ -381,11 +383,11 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
continue
}

eventIds.add(event.eventId)
liveEventService.get().dispatchLiveEventReceived(event, roomId, insertType == EventInsertType.INITIAL_SYNC)

val isInitialSync = insertType == EventInsertType.INITIAL_SYNC

eventIds.add(event.eventId)
liveEventService.get().dispatchLiveEventReceived(event, roomId, isInitialSync)

if (event.isEncrypted() && !isInitialSync) {
runBlocking {
decryptIfNeeded(event, roomId)
Expand All @@ -404,9 +406,8 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
root = eventEntity
}
if (event.type == EventType.STATE_ROOM_MEMBER) {
val fixedContent = event.getFixedRoomMemberContent()
roomMemberContentsByUser[event.stateKey] = fixedContent
roomMemberEventHandler.handle(realm, roomEntity.roomId, event.stateKey, fixedContent, aggregator)
roomMemberContentsByUser[event.stateKey] = event.getFixedRoomMemberContent()
roomMemberEventHandler.handle(realm, roomEntity.roomId, event, isInitialSync, aggregator)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package org.matrix.android.sdk.internal.session.user

import androidx.lifecycle.LiveData
import androidx.paging.PagedList
import org.matrix.android.sdk.api.session.profile.ProfileService
import org.matrix.android.sdk.api.session.user.UserService
import org.matrix.android.sdk.api.session.user.model.User
import org.matrix.android.sdk.api.util.Optional
Expand All @@ -37,16 +36,10 @@ internal class DefaultUserService @Inject constructor(private val userDataSource
}

override suspend fun resolveUser(userId: String): User {
val known = getUser(userId)
if (known != null) {
return known
} else {
return getUser(userId) ?: run {
val params = GetProfileInfoTask.Params(userId)
val data = getProfileInfoTask.execute(params)
return User(
userId,
data[ProfileService.DISPLAY_NAME_KEY] as? String,
data[ProfileService.AVATAR_URL_KEY] as? String)
val json = getProfileInfoTask.execute(params)
User.fromJson(userId, json)
}
}

Expand Down
Loading