-
Notifications
You must be signed in to change notification settings - Fork 38
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
WIP: expose room avatars in room subscriptions #203
Conversation
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.
Very comprehensive tests, I love it.
internal/roomname.go
Outdated
// SameRoomAvatar checks if the fields relevant for room names have changed between the two metadatas. | ||
// Returns true if there are no changes. | ||
func (m *RoomMetadata) SameRoomAvatar(other *RoomMetadata) bool { | ||
return m.AvatarURL == other.AvatarURL |
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.
This probably needs to check the Heroes too like we do with room names?
sync3/caches/user.go
Outdated
@@ -572,6 +584,10 @@ func (c *UserCache) OnInvite(ctx context.Context, roomID string, inviteStateEven | |||
urd.HasLeft = false | |||
urd.HighlightCount = InvitesAreHighlightsValue | |||
urd.IsDM = inviteData.IsDM | |||
urd.ResolvedAvatarURL = inviteData.AvatarURL | |||
if urd.ResolvedAvatarURL == "" && urd.IsDM && len(inviteData.Heroes) == 1 { | |||
urd.ResolvedAvatarURL = inviteData.Heroes[0].Avatar |
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.
Has the Heroes
array dropped the user of the UserCache
by this point? Else len(inviteData.Heroes)
would be 2 not 1.
sync3/room.go
Outdated
Name string `json:"name,omitempty"` | ||
// Avatar is set to the MXC url of the room if it has changed, or to "none" if | ||
// it has been removed. An empty string represents no change to the avatar. | ||
Avatar string `json:"avatar,omitempty"` |
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.
Should be *string
where literal null
means deleted.
tests-e2e/lists_test.go
Outdated
res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "leave") | ||
|
||
t.Log("Alice sees no change in the room's avatar.") | ||
m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(""))) |
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.
This relies on the atomicity of Chris leaving and the avatar being updated in one single response, which I think we do which is good but is a noteworthy comment.
tests-e2e/lists_test.go
Outdated
alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) | ||
|
||
t.Log("Chris changes his global avatar.") | ||
chrisAvatar2 := chris.UploadContent(t, smallPNG, "chris2.png", "image/png") |
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.
Maybe globalChrisAvatar
would be a better name?
SITREP:
|
logger.Warn().Msgf("TODO set avatar on invite") | ||
//urd.ResolvedAvatarURL = inviteData.AvatarEvent | ||
//if urd.ResolvedAvatarURL == "" && urd.IsDM && len(inviteData.Heroes) == 1 { | ||
// urd.ResolvedAvatarURL = inviteData.Heroes[0].Avatar | ||
//} |
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.
Confusingly I didn't seem to have to do this---SetRoom did it for me?
Superseded by #208 |
very wip do not judge me