Merge pull request #394 from matrix-org/kegan/dm-avatar-only

Only emit avatars when DM room is set

internal/roomname.go

@@ -109,12 +109,6 @@ func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool {
 		sameHeroNames(m.Heroes, other.Heroes))
 }
 
-// SameRoomAvatar checks if the fields relevant for room avatars have changed between the two metadatas.
-// Returns true if there are no changes.
-func (m *RoomMetadata) SameRoomAvatar(other *RoomMetadata) bool {
-	return m.AvatarEvent == other.AvatarEvent && sameHeroAvatars(m.Heroes, other.Heroes)
-}
-
 func (m *RoomMetadata) SameJoinCount(other *RoomMetadata) bool {
 	return m.JoinCount == other.JoinCount
 }
@@ -138,21 +132,6 @@ func sameHeroNames(a, b []Hero) bool {
 	return true
 }
 
-func sameHeroAvatars(a, b []Hero) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	for i := range a {
-		if a[i].ID != b[i].ID {
-			return false
-		}
-		if a[i].Avatar != b[i].Avatar {
-			return false
-		}
-	}
-	return true
-}
-
 func (m *RoomMetadata) RemoveHero(userID string) {
 	for i, h := range m.Heroes {
 		if h.ID == userID {
@@ -264,11 +243,11 @@ const noAvatar = ""
 // CalculateAvatar computes the avatar for the room, based on the global room metadata.
 // Assumption: metadata.RemoveHero has been called to remove the user who is syncing
 // from the list of heroes.
-func CalculateAvatar(metadata *RoomMetadata) string {
+func CalculateAvatar(metadata *RoomMetadata, isDM bool) string {
 	if metadata.AvatarEvent != "" {
 		return metadata.AvatarEvent
 	}
-	if len(metadata.Heroes) == 1 {
+	if len(metadata.Heroes) == 1 && isDM {
 		return metadata.Heroes[0].Avatar
 	}
 	return noAvatar

sync3/caches/user.go

@@ -115,7 +115,7 @@ func NewInviteData(ctx context.Context, userID, roomID string, inviteState []jso
 					Timestamp:     uint64(ts),
 					AlwaysProcess: true,
 				}
-				id.IsDM = j.Get("is_direct").Bool()
+				id.IsDM = j.Get("content.is_direct").Bool()
 			} else if target == j.Get("sender").Str {
 				id.Heroes = append(id.Heroes, internal.Hero{
 					ID:     target,

sync3/handler/connstate.go

@@ -675,7 +675,7 @@ func (s *ConnState) getInitialRoomData(ctx context.Context, roomSub sync3.RoomSu
 		roomName, calculated := internal.CalculateRoomName(metadata, 5) // TODO: customisable?
 		room := sync3.Room{
 			Name:              roomName,
-			AvatarChange:      sync3.NewAvatarChange(internal.CalculateAvatar(metadata)),
+			AvatarChange:      sync3.NewAvatarChange(internal.CalculateAvatar(metadata, userRoomData.IsDM)),
 			NotificationCount: int64(userRoomData.NotificationCount),
 			HighlightCount:    int64(userRoomData.HighlightCount),
 			Timeline:          roomToTimeline[roomID],

sync3/handler/connstate_live.go

@@ -277,7 +277,7 @@ func (s *connStateLive) processLiveUpdate(ctx context.Context, up caches.Update,
 			if delta.RoomAvatarChanged {
 				metadata := roomUpdate.GlobalRoomMetadata()
 				metadata.RemoveHero(s.userID)
-				thisRoom.AvatarChange = sync3.NewAvatarChange(internal.CalculateAvatar(metadata))
+				thisRoom.AvatarChange = sync3.NewAvatarChange(internal.CalculateAvatar(metadata, roomUpdate.UserRoomMetadata().IsDM))
 			}
 			if delta.InviteCountChanged {
 				thisRoom.InvitedCount = &roomUpdate.GlobalRoomMetadata().InviteCount

sync3/lists.go

@@ -85,9 +85,9 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) {
 			//           to conclude.
 			r.CanonicalisedName = existing.CanonicalisedName
 		}
-		delta.RoomAvatarChanged = !existing.SameRoomAvatar(&r.RoomMetadata)
+		delta.RoomAvatarChanged = !existing.SameRoomAvatar(&r)
 		if delta.RoomAvatarChanged {
-			r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata)
+			r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata, r.IsDM)
 		}
 
 		// Interpret the timestamp map on r as the changes we should apply atop the
@@ -114,7 +114,7 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) {
 		r.CanonicalisedName = strings.ToLower(
 			strings.Trim(roomName, "#!():_@"),
 		)
-		r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata)
+		r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata, r.IsDM)
 		// We'll automatically use the LastInterestedEventTimestamps provided by the
 		// caller, so that recency sorts work.
 	}

sync3/room.go

@@ -56,6 +56,37 @@ type RoomConnMetadata struct {
 	LastInterestedEventTimestamps map[string]uint64
 }
 
+// SameRoomAvatar checks if the fields relevant for room avatars have changed between the two metadatas.
+// Returns true if there are no changes.
+func (r *RoomConnMetadata) SameRoomAvatar(next *RoomConnMetadata) bool {
+	sameRoomAvatar := r.AvatarEvent == next.AvatarEvent
+	if next.IsDM {
+		// the avatar is the same IF:
+		// - the m.room.avatar event is the same AND
+		// - the heroes haven't changed AND
+		// - the number of heroes is 1
+		return sameRoomAvatar && sameHeroAvatars(r.Heroes, next.Heroes) && len(next.Heroes) == 1
+	}
+	// the avatar is the same IF:
+	// - the m.room.avatar event is the same
+	return sameRoomAvatar
+}
+
+func sameHeroAvatars(a, b []internal.Hero) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i].ID != b[i].ID {
+			return false
+		}
+		if a[i].Avatar != b[i].Avatar {
+			return false
+		}
+	}
+	return true
+}
+
 func (r *RoomConnMetadata) GetLastInterestedEventTimestamp(listKey string) uint64 {
 	ts, ok := r.LastInterestedEventTimestamps[listKey]
 	if ok {

tests-e2e/lists_test.go

@@ -1376,6 +1376,11 @@ func TestAvatarFieldInRoomResponse(t *testing.T) {
 		"invite":    []string{bob.UserID, chris.UserID},
 	})
 
+	alice.MustSetGlobalAccountData(t, "m.direct", map[string]any{
+		bob.UserID:   []string{dmBob, dmBobChris},
+		chris.UserID: []string{dmChris, dmBobChris},
+	})
+
 	t.Logf("Rooms:\npublic=%s\ndmBob=%s\ndmChris=%s\ndmBobChris=%s", public, dmBob, dmChris, dmBobChris)
 	t.Log("Bob accepts his invites. Chris accepts none.")
 	bob.JoinRoom(t, dmBob, nil)
@@ -1390,14 +1395,14 @@ func TestAvatarFieldInRoomResponse(t *testing.T) {
 		},
 	})
 
-	t.Log("Alice should see each room in the sync response with an appropriate avatar")
+	t.Log("Alice should see each room in the sync response with an appropriate avatar and DM flag")
 	m.MatchResponse(
 		t,
 		res,
-		m.MatchRoomSubscription(public, m.MatchRoomUnsetAvatar()),
-		m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bob.AvatarURL)),
-		m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chris.AvatarURL)),
-		m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar()),
+		m.MatchRoomSubscription(public, m.MatchRoomUnsetAvatar(), m.MatchRoomIsDM(false)),
+		m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bob.AvatarURL), m.MatchRoomIsDM(true)),
+		m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chris.AvatarURL), m.MatchRoomIsDM(true)),
+		m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar(), m.MatchRoomIsDM(true)),
 	)
 
 	t.Run("Avatar not resent on message", func(t *testing.T) {
@@ -1713,7 +1718,6 @@ func TestAvatarFieldInRoomResponse(t *testing.T) {
 			}
 			return m.MatchRoomAvatar(bob.AvatarURL)(r)
 		}))
-
 	})
 
 	t.Run("See avatar when invited", func(t *testing.T) {
@@ -1727,7 +1731,88 @@ func TestAvatarFieldInRoomResponse(t *testing.T) {
 		t.Log("Alice syncs until she sees the invite.")
 		res = alice.SlidingSyncUntilMembership(t, res.Pos, dmInvited, alice, "invite")
 
-		t.Log("The new room should use Chris's avatar.")
-		m.MatchResponse(t, res, m.MatchRoomSubscription(dmInvited, m.MatchRoomAvatar(chris.AvatarURL)))
+		t.Log("The new room should appear as a DM and use Chris's avatar.")
+		m.MatchResponse(t, res, m.MatchRoomSubscription(dmInvited, m.MatchRoomIsDM(true), m.MatchRoomAvatar(chris.AvatarURL)))
+
+		t.Run("Creator of a non-DM never sees an avatar", func(t *testing.T) {
+			t.Log("Alice makes a new room which is not a DM.")
+			privateGroup := alice.MustCreateRoom(t, map[string]interface{}{
+				"preset":    "trusted_private_chat",
+				"is_direct": false,
+			})
+
+			t.Log("Alice sees the group. It has no avatar.")
+			res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(privateGroup, m.MatchRoomUnsetAvatar()))
+			m.MatchResponse(t, res, m.MatchRoomSubscription(privateGroup, m.MatchRoomIsDM(false)))
+
+			t.Log("Alice invites Bob to the group, who accepts.")
+			alice.MustInviteRoom(t, privateGroup, bob.UserID)
+			bob.MustJoinRoom(t, privateGroup, nil)
+
+			t.Log("Alice sees Bob join. The room still has no avatar.")
+			res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error {
+				matchNoAvatarChange := m.MatchRoomSubscription(privateGroup, m.MatchRoomUnchangedAvatar())
+				if err := matchNoAvatarChange(response); err != nil {
+					t.Fatalf("Saw group avatar change: %s", err)
+				}
+				matchJoin := m.MatchRoomSubscription(privateGroup, MatchRoomTimelineMostRecent(1, []Event{
+					{
+						Type:     "m.room.member",
+						Sender:   bob.UserID,
+						StateKey: ptr(bob.UserID),
+					},
+				}))
+				return matchJoin(response)
+			})
+
+			t.Log("Alice invites Chris to the group, who accepts.")
+			alice.MustInviteRoom(t, privateGroup, chris.UserID)
+			chris.MustJoinRoom(t, privateGroup, nil)
+
+			t.Log("Alice sees Chris join. The room still has no avatar.")
+			res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error {
+				matchNoAvatarChange := m.MatchRoomSubscription(privateGroup, m.MatchRoomUnchangedAvatar())
+				if err := matchNoAvatarChange(response); err != nil {
+					t.Fatalf("Saw group avatar change: %s", err)
+				}
+				matchJoin := m.MatchRoomSubscription(privateGroup, MatchRoomTimelineMostRecent(1, []Event{
+					{
+						Type:     "m.room.member",
+						Sender:   chris.UserID,
+						StateKey: ptr(chris.UserID),
+					},
+				}))
+				return matchJoin(response)
+			})
+		})
 	})
 }
+
+// Regression test for https://github.com/element-hq/element-x-ios/issues/2003
+// Ensure that a group chat with 1 other person has no avatar field set. Only DMs should have this set.
+func TestAvatarUnsetInTwoPersonRoom(t *testing.T) {
+	alice := registerNamedUser(t, "alice")
+	bob := registerNamedUser(t, "bob")
+	bobAvatar := alice.UploadContent(t, smallPNG, "bob.png", "image/png")
+	bob.SetAvatar(t, bobAvatar)
+	roomID := alice.MustCreateRoom(t, map[string]interface{}{
+		"preset": "trusted_private_chat",
+		"name":   "Nice test room",
+		"invite": []string{bob.UserID},
+	})
+
+	res := alice.SlidingSync(t, sync3.Request{
+		Lists: map[string]sync3.RequestList{
+			"a": {
+				Ranges: sync3.SliceRanges{{0, 20}},
+			},
+		},
+	})
+	m.MatchResponse(t, res, m.MatchRoomSubscriptionsStrict(map[string][]m.RoomMatcher{
+		roomID: {
+			m.MatchRoomUnsetAvatar(),
+			m.MatchInviteCount(1),
+			m.MatchRoomName("Nice test room"),
+		},
+	}))
+}

testutils/m/match.go

@@ -73,6 +73,15 @@ func MatchRoomUnchangedAvatar() RoomMatcher {
 	}
 }
 
+func MatchRoomIsDM(wantDM bool) RoomMatcher {
+	return func(r sync3.Room) error {
+		if r.IsDM != wantDM {
+			return fmt.Errorf("MatchRoomIsDM: got %t want %t", r.IsDM, wantDM)
+		}
+		return nil
+	}
+}
+
 func MatchJoinCount(count int) RoomMatcher {
 	return func(r sync3.Room) error {
 		if r.JoinedCount != count {