Give clients avatar URLs for rooms

internal/roomname.go

@@ -13,7 +13,12 @@ type EventMetadata struct {
 	Timestamp uint64
 }
 
-// RoomMetadata holds room-scoped data. It is primarily used in two places:
+// RoomMetadata holds room-scoped data.
+// TODO: This is a lie: we sometimes remove a user U from the list of heroes
+// when calculating the sync response for that user U. Grep for `RemoveHero`.
+//
+// It is primarily used in two places:
+//
 //   - in the caches.GlobalCache, to hold the latest version of data that is consistent
 //     between all users in the room; and
 //   - in the sync3.RoomConnMetadata struct, to hold the version of data last seen by
@@ -25,6 +30,7 @@ type RoomMetadata struct {
 	RoomID         string
 	Heroes         []Hero
 	NameEvent      string // the content of m.room.name, NOT the calculated name
+	AvatarEvent    string // the content of m.room.avatar, NOT the resolved avatar
 	CanonicalAlias string
 	JoinCount      int
 	InviteCount    int
@@ -54,6 +60,32 @@ func NewRoomMetadata(roomID string) *RoomMetadata {
 	}
 }
 
+// CopyHeroes returns a version of the current RoomMetadata whose Heroes field is
+// a brand-new copy of the original Heroes. The return value's Heroes field can be
+// safely modified by the caller, but it is NOT safe for the caller to modify any other
+// fields.
+func (m *RoomMetadata) CopyHeroes() *RoomMetadata {
+	newMetadata := *m
+
+	// XXX: We're doing this because we end up calling RemoveHero() to omit the
+	// currently-sycning user in various places. But this seems smelly. The set of
+	// heroes in the room is a global, room-scoped fact: it is a property of the room
+	// state and nothing else, and all users see the same set of heroes.
+	//
+	// I think the data model would be cleaner if we made the hero-reading functions
+	// aware of the currently syncing user, in order to ignore them without having to
+	// change the underlying data.
+	//
+	// copy the heroes or else we may modify the same slice which would be bad :(
+	newMetadata.Heroes = make([]Hero, len(m.Heroes))
+	copy(newMetadata.Heroes, m.Heroes)
+
+	// ⚠️ NB: there are other pointer fields (e.g. PredecessorRoomID *string) or
+	// and pointer-backed fields (e.g. LatestEventsByType map[string]EventMetadata)
+	// which are not deepcopied here.
+	return &newMetadata
+}
+
 // SameRoomName checks if the fields relevant for room names have changed between the two metadatas.
 // Returns true if there are no changes.
 func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool {
@@ -62,7 +94,13 @@ func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool {
 		m.CanonicalAlias == other.CanonicalAlias &&
 		m.JoinCount == other.JoinCount &&
 		m.InviteCount == other.InviteCount &&
-		sameHeroes(m.Heroes, other.Heroes))
+		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 {
@@ -73,7 +111,7 @@ func (m *RoomMetadata) SameInviteCount(other *RoomMetadata) bool {
 	return m.InviteCount == other.InviteCount
 }
 
-func sameHeroes(a, b []Hero) bool {
+func sameHeroNames(a, b []Hero) bool {
 	if len(a) != len(b) {
 		return false
 	}
@@ -88,6 +126,21 @@ func sameHeroes(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 {
@@ -102,8 +155,9 @@ func (m *RoomMetadata) IsSpace() bool {
 }
 
 type Hero struct {
-	ID   string
-	Name string
+	ID     string
+	Name   string
+	Avatar string
 }
 
 func CalculateRoomName(heroInfo *RoomMetadata, maxNumNamesPerRoom int) string {
@@ -190,3 +244,18 @@ func disambiguate(heroes []Hero) []string {
 	}
 	return disambiguatedNames
 }
+
+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 {
+	if metadata.AvatarEvent != "" {
+		return metadata.AvatarEvent
+	}
+	if len(metadata.Heroes) == 1 {
+		return metadata.Heroes[0].Avatar
+	}
+	return noAvatar
+}

internal/roomname_test.go

@@ -247,3 +247,45 @@ func TestCalculateRoomName(t *testing.T) {
 		}
 	}
 }
+
+func TestCopyHeroes(t *testing.T) {
+	const alice = "@alice:test"
+	const bob = "@bob:test"
+	const chris = "@chris:test"
+	m1 := RoomMetadata{Heroes: []Hero{
+		{ID: alice},
+		{ID: bob},
+		{ID: chris},
+	}}
+
+	m2 := m1.CopyHeroes()
+	// Uncomment this to see why CopyHeroes is necessary!
+	//m2 := m1
+
+	t.Logf("Compare heroes:\n\tm1=%v\n\tm2=%v", m1.Heroes, m2.Heroes)
+
+	t.Log("Remove chris from m1")
+	m1.RemoveHero(chris)
+	t.Logf("Compare heroes:\n\tm1=%v\n\tm2=%v", m1.Heroes, m2.Heroes)
+
+	assertSliceIDs(t, "m1.Heroes", m1.Heroes, []string{alice, bob})
+	assertSliceIDs(t, "m2.Heroes", m2.Heroes, []string{alice, bob, chris})
+
+	t.Log("Remove alice from m1")
+	m1.RemoveHero(alice)
+	t.Logf("Compare heroes:\n\tm1=%v\n\tm2=%v", m1.Heroes, m2.Heroes)
+
+	assertSliceIDs(t, "m1.Heroes", m1.Heroes, []string{bob})
+	assertSliceIDs(t, "m2.Heroes", m2.Heroes, []string{alice, bob, chris})
+}
+
+func assertSliceIDs(t *testing.T, desc string, h []Hero, ids []string) {
+	if len(h) != len(ids) {
+		t.Errorf("%s has length %d, expected %d", desc, len(h), len(ids))
+	}
+	for index, id := range ids {
+		if h[index].ID != id {
+			t.Errorf("%s[%d] ID is %s, expected %s", desc, index, h[index].ID, id)
+		}
+	}
+}

state/storage.go

@@ -232,7 +232,7 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result
 
 	// Select the name / canonical alias for all rooms
 	roomIDToStateEvents, err := s.currentNotMembershipStateEventsInAllRooms(txn, []string{
-		"m.room.name", "m.room.canonical_alias",
+		"m.room.name", "m.room.canonical_alias", "m.room.avatar",
 	})
 	if err != nil {
 		return fmt.Errorf("failed to load state events for all rooms: %s", err)
@@ -244,6 +244,8 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result
 				metadata.NameEvent = gjson.ParseBytes(ev.JSON).Get("content.name").Str
 			} else if ev.Type == "m.room.canonical_alias" && ev.StateKey == "" {
 				metadata.CanonicalAlias = gjson.ParseBytes(ev.JSON).Get("content.alias").Str
+			} else if ev.Type == "m.room.avatar" && ev.StateKey == "" {
+				metadata.AvatarEvent = gjson.ParseBytes(ev.JSON).Get("content.url").Str
 			}
 		}
 		result[roomID] = metadata
@@ -282,8 +284,9 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result
 		seen[key] = true
 		metadata := result[roomID]
 		metadata.Heroes = append(metadata.Heroes, internal.Hero{
-			ID:   targetUser,
-			Name: ev.Get("content.displayname").Str,
+			ID:     targetUser,
+			Name:   ev.Get("content.displayname").Str,
+			Avatar: ev.Get("content.avatar_url").Str,
 		})
 		result[roomID] = metadata
 	}

sync3/avatar.go

@@ -0,0 +1,42 @@
+package sync3
+
+import (
+	"bytes"
+	"encoding/json"
+)
+
+// An AvatarChange represents a change to a room's avatar. There are three cases:
+//   - an empty string represents no change, and should be omitted when JSON-serialised;
+//   - the sentinel `<no-avatar>` represents a room that has never had an avatar,
+//     or a room whose avatar has been removed. It is JSON-serialised as null.
+//   - All other strings represent the current avatar of the room and JSON-serialise as
+//     normal.
+type AvatarChange string
+
+const DeletedAvatar = AvatarChange("<no-avatar>")
+const UnchangedAvatar AvatarChange = ""
+
+// NewAvatarChange interprets an optional avatar string as an AvatarChange.
+func NewAvatarChange(avatar string) AvatarChange {
+	if avatar == "" {
+		return DeletedAvatar
+	}
+	return AvatarChange(avatar)
+}
+
+func (a AvatarChange) MarshalJSON() ([]byte, error) {
+	if a == DeletedAvatar {
+		return []byte(`null`), nil
+	} else {
+		return json.Marshal(string(a))
+	}
+}
+
+// Note: the unmarshalling is only used in tests.
+func (a *AvatarChange) UnmarshalJSON(data []byte) error {
+	if bytes.Equal(data, []byte("null")) {
+		*a = DeletedAvatar
+		return nil
+	}
+	return json.Unmarshal(data, (*string)(a))
+}

sync3/caches/global.go

@@ -118,13 +118,7 @@ func (c *GlobalCache) copyRoom(roomID string) *internal.RoomMetadata {
 		logger.Warn().Str("room", roomID).Msg("GlobalCache.LoadRoom: no metadata for this room, returning stub")
 		return internal.NewRoomMetadata(roomID)
 	}
-	srCopy := *sr
-	// copy the heroes or else we may modify the same slice which would be bad :(
-	srCopy.Heroes = make([]internal.Hero, len(sr.Heroes))
-	for i := range sr.Heroes {
-		srCopy.Heroes[i] = sr.Heroes[i]
-	}
-	return &srCopy
+	return sr.CopyHeroes()
 }
 
 // LoadJoinedRooms loads all current joined room metadata for the user given, together
@@ -285,6 +279,10 @@ func (c *GlobalCache) OnNewEvent(
 		if ed.StateKey != nil && *ed.StateKey == "" {
 			metadata.NameEvent = ed.Content.Get("name").Str
 		}
+	case "m.room.avatar":
+		if ed.StateKey != nil && *ed.StateKey == "" {
+			metadata.AvatarEvent = ed.Content.Get("url").Str
+		}
 	case "m.room.encryption":
 		if ed.StateKey != nil && *ed.StateKey == "" {
 			metadata.Encrypted = true
@@ -349,14 +347,16 @@ func (c *GlobalCache) OnNewEvent(
 				for i := range metadata.Heroes {
 					if metadata.Heroes[i].ID == *ed.StateKey {
 						metadata.Heroes[i].Name = ed.Content.Get("displayname").Str
+						metadata.Heroes[i].Avatar = ed.Content.Get("avatar_url").Str
 						found = true
 						break
 					}
 				}
 				if !found {
 					metadata.Heroes = append(metadata.Heroes, internal.Hero{
-						ID:   *ed.StateKey,
-						Name: ed.Content.Get("displayname").Str,
+						ID:     *ed.StateKey,
+						Name:   ed.Content.Get("displayname").Str,
+						Avatar: ed.Content.Get("avatar_url").Str,
 					})
 				}
 			}

sync3/caches/user.go

@@ -46,10 +46,18 @@ type UserRoomData struct {
 	// The zero value of this safe to use (0 latest nid, no prev batch, no timeline).
 	RequestedLatestEvents state.LatestEvents
 
-	// TODO: should Canonicalised really be in RoomConMetadata? It's only set in SetRoom AFAICS
+	// TODO: should CanonicalisedName really be in RoomConMetadata? It's only set in SetRoom AFAICS
 	CanonicalisedName string // stripped leading symbols like #, all in lower case
 	// Set of spaces this room is a part of, from the perspective of this user. This is NOT global room data
 	// as the set of spaces may be different for different users.
+
+	// ResolvedAvatarURL is the avatar that should be displayed to this user to
+	// represent this room. The empty string means that this room has no avatar.
+	// Avatars set in m.room.avatar take precedence; if this is missing and the room is
+	// a DM with one other user joined or invited, we fall back to that user's
+	// avatar (if any) as specified in their membership event in that room.
+	ResolvedAvatarURL string
+
 	Spaces map[string]struct{}
 	// Map of tag to order float.
 	// See https://spec.matrix.org/latest/client-server-api/#room-tagging
@@ -73,6 +81,7 @@ type InviteData struct {
 	Heroes               []internal.Hero
 	InviteEvent          *EventData
 	NameEvent            string // the content of m.room.name, NOT the calculated name
+	AvatarEvent          string // the content of m.room.avatar, NOT the calculated avatar
 	CanonicalAlias       string
 	LastMessageTimestamp uint64
 	Encrypted            bool
@@ -108,12 +117,15 @@ func NewInviteData(ctx context.Context, userID, roomID string, inviteState []jso
 				id.IsDM = j.Get("is_direct").Bool()
 			} else if target == j.Get("sender").Str {
 				id.Heroes = append(id.Heroes, internal.Hero{
-					ID:   target,
-					Name: j.Get("content.displayname").Str,
+					ID:     target,
+					Name:   j.Get("content.displayname").Str,
+					Avatar: j.Get("content.avatar_url").Str,
 				})
 			}
 		case "m.room.name":
 			id.NameEvent = j.Get("content.name").Str
+		case "m.room.avatar":
+			id.AvatarEvent = j.Get("content.avatar_url").Str
 		case "m.room.canonical_alias":
 			id.CanonicalAlias = j.Get("content.alias").Str
 		case "m.room.encryption":
@@ -147,6 +159,7 @@ func (i *InviteData) RoomMetadata() *internal.RoomMetadata {
 	metadata := internal.NewRoomMetadata(i.roomID)
 	metadata.Heroes = i.Heroes
 	metadata.NameEvent = i.NameEvent
+	metadata.AvatarEvent = i.AvatarEvent
 	metadata.CanonicalAlias = i.CanonicalAlias
 	metadata.InviteCount = 1
 	metadata.JoinCount = 1
@@ -212,7 +225,7 @@ func (c *UserCache) Unsubscribe(id int) {
 // OnRegistered is called after the sync3.Dispatcher has successfully registered this
 // cache to receive updates. We use this to run some final initialisation logic that
 // is sensitive to race conditions; confusingly, most of the initialisation is driven
-// externally by sync3.SyncLiveHandler.userCache. It's importatn that we don't spend too
+// externally by sync3.SyncLiveHandler.userCaches. It's important that we don't spend too
 // long inside this function, because it is called within a global lock on the
 // sync3.Dispatcher (see sync3.Dispatcher.Register).
 func (c *UserCache) OnRegistered(ctx context.Context) error {
@@ -328,7 +341,10 @@ func (c *UserCache) LoadRoomData(roomID string) UserRoomData {
 }
 
 type roomUpdateCache struct {
-	roomID         string
+	roomID string
+	// globalRoomData is a snapshot of the global metadata for this room immediately
+	// after this update. It is a copy, specific to the given user whose Heroes
+	// field can be freely modified.
 	globalRoomData *internal.RoomMetadata
 	userRoomData   *UserRoomData
 }

sync3/handler/connstate.go

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