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

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Mar 23, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Removes a user entity update when changing your nickname for a single room using the command /myroomnick

Motivation and context

Closes #5321

When entering the above command to change the nickname, it updates both the Room Member Entity and the User Entity. It's clear why the Room Member Entity update needs to be made (this actually updates the nickname of that room member), but it's very unclear if the User Entity update is needed at all. This being updated is the source of this bug.

Additionally, changing your nickname in the room changed your display name across the app. You would even see this reflected beside your avatar in the navigation drawer.

Screenshots / GIFs

After updating your nickname in a room, you would get these results:

Before After
image image

Tests

  • In any room type the command /myRoomNick [name] to change your nickname for that room
  • Send a message in the room and see that your nickname in that room is successfully changed
  • See that your display name on the navigation drawer is as normal and not your newly set room nickname
  • On another account, create a new room and go to the invite page and search for your account that set the nickname in a room. See that the display name is as normal and not the previously set room nickname

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 10, 12

Checklist

@ericdecanini ericdecanini requested a review from ganfra March 23, 2022 17:45
Comment on lines 61 to 64
if (roomMember.membership.isActive()) {
val userEntity = UserEntityFactory.create(userId, roomMember)
realm.insertOrUpdate(userEntity)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ganfra was there any purpose to this being here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is the way to have local User created from RoomMember state event. If you remove this, we'll loose the avatar/display name of lots of users (which is the same than a RoomMember but outside the context of a room.)
If we remove this, we should take care of fetching profile of users everywhere, which is not that great...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see now

@ericdecanini ericdecanini changed the title Fixes myRoomNick changing Display Name Fixes myroomnick changing Display Name Mar 23, 2022
@ericdecanini ericdecanini added the PR-Small PR with less than 20 updated lines label Mar 23, 2022
@github-actions
Copy link

github-actions bot commented Mar 23, 2022

Unit Test Results

114 files  +  4  114 suites  +4   1m 28s ⏱️ +13s
201 tests +  6  201 ✔️ +  6  0 💤 ±0  0 ±0 
674 runs  +24  674 ✔️ +24  0 💤 ±0  0 ±0 

Results for commit 8b80cf0. ± Comparison against base commit 72bd398.

♻️ This comment has been updated with latest results.

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.

This will fix this case, but will break others, see my comment

Comment on lines 79 to 85
val previousDisplayName = prevContent?.get("displayname")
val shouldLocallySaveUser = roomMember.membership.isActive()
&& (previousDisplayName == null || previousDisplayName == roomMember.displayName)

if (shouldLocallySaveUser) {
saveUserLocally(realm, userId, roomMember)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ganfra I think this should fix the bug whilst keeping previous functionality intact

@ericdecanini ericdecanini requested a review from ganfra March 30, 2022 15:45
@ganfra
Copy link
Member

ganfra commented Mar 31, 2022

So the idea now, is to try fetching profile when having incremental sync using the SyncResponsePostTreatmentAggregator to track userIds to fetch.
In practice, we have 2 cases:

  • InitialSync: just save the RoomMember content into the UserEntity, except for our own userId (as it already have a fetched profile)
  • IncrementalSync: check if avatar/displayName of the RoomMember is different from the avatar/DisplayName of the UserEntity. If yes, add this userId to the SyncResponsePostTreatmentAggregator. Then at the end of the sync, you can fetch all the profiles from the aggregator.

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.

Some remarks

aggregator: SyncResponsePostTreatmentAggregator? = null): Boolean {
fun handleInitialSync(realm: Realm,
roomId: String,
currentUserId: String,
Copy link
Member

Choose a reason for hiding this comment

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

This is already injected on constructor (myUserId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👁️👄👁️

roomMember: RoomMemberContent,
aggregator: SyncResponsePostTreatmentAggregator?): Boolean {
if (currentUserId != eventUserId) {
saveRoomMemberEntityLocally(realm, roomId, eventUserId, roomMember)
Copy link
Member

Choose a reason for hiding this comment

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

Always save RoomMember even if its our userId, but don't save User


MatrixItem.UserItem(
id = it,
displayName = profileJson[ProfileService.DISPLAY_NAME_KEY] as? String,
Copy link
Member

Choose a reason for hiding this comment

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

Create a method to reuse these json keys as it's used in several places

@ericdecanini ericdecanini requested a review from ganfra April 6, 2022 06:48
@ericdecanini ericdecanini removed the PR-Small PR with less than 20 updated lines label Apr 6, 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.

Ok looks like it's ok now, thanks for this! (haven't tested)

@ganfra
Copy link
Member

ganfra commented Apr 6, 2022

Just fix the conflict and build failing

…display-name

# Conflicts:
#	vector/src/main/java/im/vector/app/features/roommemberprofile/RoomMemberProfileViewModel.kt
#	vector/src/main/java/im/vector/app/features/userdirectory/UserListViewModel.kt
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.

I think this PR will fix the issue. Thanks!
Some remarks on the form.

init {
if (BuildConfig.DEBUG) checkId()
}

override fun updateAvatar(newAvatar: String?) = copy(avatarUrl = newAvatar)

companion object {
fun fromJson(userId: String, profileJson: JsonDict) = UserItem(
Copy link
Member

@bmarty bmarty Apr 7, 2022

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 MatrixItem is more something that the SDK expose to the app, to be used by the UI. The SDK should not use MatrixItem internally. You should use User instead.

@@ -123,7 +123,7 @@ internal class DefaultLoadRoomMembersTask @Inject constructor(
eventId = roomMemberEvent.eventId
root = eventEntity
}
roomMemberEventHandler.handle(realm, roomId, roomMemberEvent)
roomMemberEventHandler.handleIncrementalSync(realm, roomId, roomMemberEvent)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that we just add a param isInitialSync: Boolean to the existing handle method instead?
I prefer to have only one handle method in the ...Handler class.
Internally it can be deferred to 2 different methods.

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'm generally not a fan of passing boolean flags as the argument here is if you need to pass a boolean that changes the actions of the functions, you may as well make it another function which should be kept as small as possible (again, from Uncle Bob's Clean Code)

But tbh in this case I'm not too bothered either way. I'll make the changes!

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.

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


private suspend fun fetchUsers(usersToFetch: MutableList<String>) = usersToFetch.map {
val profileJson = profileService.getProfile(it)
MatrixItem.UserItem.fromJson(it, profileJson)
Copy link
Member

Choose a reason for hiding this comment

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

Better to use User.fromJson(it, profileJson) here. (see my other remark)

@@ -206,7 +206,7 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
insertType: EventInsertType,
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?

roomMemberEventHandler.handleInitialSync(realm, roomId, event, aggregator)
} else {
roomMemberEventHandler.handleIncrementalSync(realm, roomId, event, aggregator)
}
Copy link
Member

Choose a reason for hiding this comment

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

This code, and the similar occurence, will be much simpler with an extra parameter (see my other remark)

data[ProfileService.DISPLAY_NAME_KEY] as? String,
data[ProfileService.AVATAR_URL_KEY] as? String)
}
override suspend fun resolveUser(userId: String) = getUser(userId) ?: run {
Copy link
Member

Choose a reason for hiding this comment

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

When reading the code in GitHub, it's far better to keep explicit type : User {
Short fun form with = should be reserved for very small functions.

)
}
session.getProfile(initialState.userId)
.let { MatrixItem.UserItem.fromJson(initialState.userId, it) }
Copy link
Member

Choose a reason for hiding this comment

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

So we will have:

            session.getProfile(initialState.userId)
                    .let { User.fromJson(initialState.userId, it) }
                    .toMatrixItem()

@ericdecanini ericdecanini requested a review from bmarty April 7, 2022 16:13
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.

Thanks for the update. One last remark.

@@ -92,6 +94,6 @@ internal class SyncResponsePostTreatmentAggregatorHandler @Inject constructor(

private suspend fun fetchUsers(usersToFetch: MutableList<String>) = usersToFetch.map {
val profileJson = profileService.getProfile(it)
MatrixItem.UserItem.fromJson(it, profileJson)
User.fromJson(it, profileJson).toMatrixItem()
Copy link
Member

Choose a reason for hiding this comment

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

As I said, we should not use MatrixItem internally.
Can you change to:

User.fromJson(it, profileJson)

update the code to make it compile and remove UserEntityFactory.create(userItem: MatrixItem.UserItem): UserEntity

Thanks!

Copy link
Contributor Author

@ericdecanini ericdecanini Apr 8, 2022

Choose a reason for hiding this comment

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

Ahh I see what you mean! Sorry I misunderstood the first time. Changes pushed

private suspend fun fetchAndUpdateUsers(usersToFetch: MutableList<String>) {
val userProfiles = fetchUsers(usersToFetch)
userProfiles.forEach { saveUserLocally(monarchy, it) }
usersToFetch.clear()
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not necessary to clear the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@bmarty bmarty Apr 12, 2022

Choose a reason for hiding this comment

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

usersToFetch parameter could probably be of type List (instead of MutableList), like for the other methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh this wasn't clear to me at first. Changes pushed

@ericdecanini ericdecanini requested a review from bmarty April 8, 2022 10:16
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.

Sorry, I have further remarks for you.

User.fromJson(it, profileJson)
}

private fun saveUserLocally(monarchy: Monarchy, user: User) {
Copy link
Member

Choose a reason for hiding this comment

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

monarchy is a class member, there is no need to add it as a method parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

istg i'm actually blind

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 profileService: ProfileService,
Copy link
Member

Choose a reason for hiding this comment

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

You should not inject Service in SDK classes. Services are meant to be used by the client app.
Instead you can inject getProfileInfoTask in this case.

monarchy.doWithRealm {
it.insertOrUpdate(userEntity)
}
}
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.

}

private suspend fun fetchUsers(usersToFetch: List<String>) = usersToFetch.map {
val profileJson = profileService.getProfile(it)
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if the request fails for any reason, such as no network? I think it will crash the app. Maybe protect by using tryOrNull. We will just ignore updating user in that case, this is not a big deal I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A very good point

@ericdecanini ericdecanini requested a review from bmarty April 13, 2022 13:27
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.

Thanks for the update, more remarks, we will finish it one day!

val roomMember = event.getFixedRoomMemberContent()
return handle(realm, roomId, userId, roomMember, aggregator)
val roomMember = event.getFixedRoomMemberContent() ?: return false
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.

@@ -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 :)


private suspend fun fetchUsers(userIdsToFetch: List<String>) = tryOrNull {
userIdsToFetch.map {
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.

monarchy.doWithRealm {
it.insertOrUpdate(userEntity)
}
}
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.

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.

Latest remarks?

prevContent: Map<String, Any>?,
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?

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?

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!


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

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.

Latest remark I think. Can you squash all the commits and force push?

@@ -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?

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.

Approved!
Could be nice to squash all the commits to avoid polluting the git history with all the little change / revert. A "squash and merge" can also be done.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong display name in search result for user
3 participants