Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix DM avatars, part 3 #2146

Merged
merged 8 commits into from
Sep 4, 2018
Merged

Fix DM avatars, part 3 #2146

merged 8 commits into from
Sep 4, 2018

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Sep 4, 2018

Requires matrix-org/matrix-js-sdk#719
Fixes element-hq/element-web#7235, element-hq/element-web#7269

The patching up from before wasn't being applied, because the rooms weren't available at the point the account data is received (see c12abab).

That was just as well, because there were some issues with the patching up. I'm a lot more confident this will fix the issue once and for all, as I reproduced the issue locally by corrupting my m.direct account data manually, seeing the wrong avatar and then confirming the patching happens and works.

Let's hope third time's the charm!

as accountData get processed before rooms, during initial sync
or loading sync from cache, accountData gets emitted
before any room is available, hence our patching wasn't doing
anything. Just as well, because it would have failed (see next commits)
}
});
return cpy;
}
Copy link
Member

Choose a reason for hiding this comment

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

Lodash has one of these: https://lodash.com/docs/4.17.10#uniq which we already have a dependency on: I'd suggest using that (also this is a bit O(n^2) because includes on an array will be linear - a Set ought to be faster).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

const myUserId = this.matrixClient.getUserId();
const selfRoomIds = userToRooms[myUserId];
if (selfRoomIds) {
// account data gets emitted before the rooms are available
// so wait for the sync to be ready and then read the rooms.
await this._waitForSyncReady();
Copy link
Member

Choose a reason for hiding this comment

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

populateRoomToUser() is done on access if necessary - could we do the same here, on the assumption that everything will be ready by the time anything comes to query the DMRoomMap? Introducing async-ness to wait for the sync feels like adding more moving parts to go wrong and could introduce races where the DMRoomMap hasn't yet been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a better idea indeed.

@bwindels
Copy link
Contributor Author

bwindels commented Sep 4, 2018

still need to do the lodash uniq thingy

@bwindels bwindels merged commit a6d241c into develop Sep 4, 2018
@bwindels bwindels deleted the bwindels/fixavatars-parttrois branch September 4, 2018 16:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some direct chat rooms without avatar show my avatar
2 participants