Fixes myroomnick changing Display Name #5618
There are no files selected for viewing
| 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 |
|---|---|---|
|
|
@@ -16,22 +16,30 @@ | |
|
|
||
| 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.session.profile.ProfileService | ||
| import org.matrix.android.sdk.api.util.MatrixItem | ||
| import org.matrix.android.sdk.internal.di.SessionDatabase | ||
| 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 profileService: ProfileService, | ||
|
There was a problem hiding this comment. You should not inject Service in SDK classes. Services are meant to be used by the client app. |
||
| @SessionDatabase private val monarchy: Monarchy, | ||
| ) { | ||
| suspend fun handle(aggregator: SyncResponsePostTreatmentAggregator) { | ||
|
There was a problem hiding this comment. Thanks for having renamed this parameter which was quite typoed :) |
||
| cleanupEphemeralFiles(aggregator.ephemeralFilesToDelete) | ||
| updateDirectUserIds(aggregator.directChatsToCheck) | ||
| fetchAndUpdateUsers(aggregator.usersToFetch) | ||
| } | ||
|
|
||
| private fun cleanupEphemeralFiles(ephemeralFilesToDelete: List<String>) { | ||
|
|
@@ -68,4 +76,22 @@ internal class SyncResponsePostTreatmentAggregatorHandler @Inject constructor( | |
| updateUserAccountDataTask.execute(UpdateUserAccountDataTask.DirectChatParams(directMessages = directChats)) | ||
| } | ||
| } | ||
|
|
||
| private suspend fun fetchAndUpdateUsers(usersToFetch: MutableList<String>) { | ||
| val userProfiles = fetchUsers(usersToFetch) | ||
| userProfiles.forEach { saveUserLocally(monarchy, it) } | ||
| usersToFetch.clear() | ||
|
There was a problem hiding this comment. This is probably not necessary to clear the list. There was a problem hiding this comment. If we don't though, wouldn't we keep fetching the same users even when we don't need to fetch them anymore? I think it could increase bandwith and performance usage the longer the app is kept on There was a problem hiding this comment. Nope. A brand new There was a problem hiding this comment.
There was a problem hiding this comment. Ahhh this wasn't clear to me at first. Changes pushed |
||
| } | ||
|
|
||
| private fun saveUserLocally(monarchy: Monarchy, userItem: MatrixItem.UserItem) { | ||
| val userEntity = UserEntityFactory.create(userItem) | ||
| monarchy.doWithRealm { | ||
| it.insertOrUpdate(userEntity) | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. |
||
|
|
||
| private suspend fun fetchUsers(usersToFetch: MutableList<String>) = usersToFetch.map { | ||
| val profileJson = profileService.getProfile(it) | ||
| MatrixItem.UserItem.fromJson(it, profileJson) | ||
|
There was a problem hiding this comment. Better to use |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that this does not exist.
You can use instead
User.fromJson(...).toMatrixItem()But
MatrixItemis more something that the SDK expose to the app, to be used by the UI. The SDK should not useMatrixIteminternally. You should useUserinstead.