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

Add posibility to request room heroes #304

Merged
merged 8 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions internal/roomname.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,19 +155,21 @@ func (m *RoomMetadata) IsSpace() bool {
}

type Hero struct {
ID string
Name string
Avatar string
ID string `json:"user_id"`
Name string `json:"displayname,omitempty"`
Avatar string `json:"avatar_url,omitempty"`
}

func CalculateRoomName(heroInfo *RoomMetadata, maxNumNamesPerRoom int) string {
// CalculateRoomName calculates the room name. Returns the name and if the name was actually calculated
// based on room heroes.
func CalculateRoomName(heroInfo *RoomMetadata, maxNumNamesPerRoom int) (name string, calculated bool) {
// If the room has an m.room.name state event with a non-empty name field, use the name given by that field.
if heroInfo.NameEvent != "" {
return heroInfo.NameEvent
return heroInfo.NameEvent, false
}
// If the room has an m.room.canonical_alias state event with a valid alias field, use the alias given by that field as the name.
if heroInfo.CanonicalAlias != "" {
return heroInfo.CanonicalAlias
return heroInfo.CanonicalAlias, false
}
// If none of the above conditions are met, a name should be composed based on the members of the room.
disambiguatedNames := disambiguate(heroInfo.Heroes)
Expand All @@ -178,21 +180,21 @@ func CalculateRoomName(heroInfo *RoomMetadata, maxNumNamesPerRoom int) string {
// the client should use the rules BELOW to indicate that the room was empty. For example, "Empty Room (was Alice)",
// "Empty Room (was Alice and 1234 others)", or "Empty Room" if there are no heroes.
if len(heroInfo.Heroes) == 0 && isAlone {
return "Empty Room"
return "Empty Room", false
}

// If the number of m.heroes for the room are greater or equal to m.joined_member_count + m.invited_member_count - 1,
// then use the membership events for the heroes to calculate display names for the users (disambiguating them if required)
// and concatenating them.
if len(heroInfo.Heroes) >= totalNumOtherUsers {
if len(disambiguatedNames) == 1 {
return disambiguatedNames[0]
return disambiguatedNames[0], true
}
calculatedRoomName := strings.Join(disambiguatedNames[:len(disambiguatedNames)-1], ", ") + " and " + disambiguatedNames[len(disambiguatedNames)-1]
if isAlone {
return fmt.Sprintf("Empty Room (was %s)", calculatedRoomName)
return fmt.Sprintf("Empty Room (was %s)", calculatedRoomName), true
}
return calculatedRoomName
return calculatedRoomName, true
}

// if we're here then len(heroes) < (joinedCount + invitedCount - 1)
Expand All @@ -208,13 +210,13 @@ func CalculateRoomName(heroInfo *RoomMetadata, maxNumNamesPerRoom int) string {
// and m.joined_member_count + m.invited_member_count is greater than 1, the client should use the heroes to calculate
// display names for the users (disambiguating them if required) and concatenating them alongside a count of the remaining users.
if (heroInfo.JoinCount + heroInfo.InviteCount) > 1 {
return calculatedRoomName
return calculatedRoomName, true
}

// If m.joined_member_count + m.invited_member_count is less than or equal to 1 (indicating the member is alone),
// the client should use the rules above to indicate that the room was empty. For example, "Empty Room (was Alice)",
// "Empty Room (was Alice and 1234 others)", or "Empty Room" if there are no heroes.
return fmt.Sprintf("Empty Room (was %s)", calculatedRoomName)
return fmt.Sprintf("Empty Room (was %s)", calculatedRoomName), true
}

func disambiguate(heroes []Hero) []string {
Expand Down
38 changes: 26 additions & 12 deletions internal/roomname_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ func TestCalculateRoomName(t *testing.T) {
invitedCount int
maxNumNamesPerRoom int

wantRoomName string
wantRoomName string
wantCalculated bool
}{
// Room name takes precedence
{
Expand Down Expand Up @@ -65,7 +66,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Bob",
},
},
wantRoomName: "Alice, Bob and 3 others",
wantRoomName: "Alice, Bob and 3 others",
wantCalculated: true,
},
// Small group chat
{
Expand All @@ -86,7 +88,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Charlie",
},
},
wantRoomName: "Alice, Bob and Charlie",
wantRoomName: "Alice, Bob and Charlie",
wantCalculated: true,
},
// DM room
{
Expand All @@ -99,7 +102,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Alice",
},
},
wantRoomName: "Alice",
wantRoomName: "Alice",
wantCalculated: true,
},
// 3-way room
{
Expand All @@ -116,7 +120,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Bob",
},
},
wantRoomName: "Alice and Bob",
wantRoomName: "Alice and Bob",
wantCalculated: true,
},
// 3-way room, one person invited with no display name
{
Expand All @@ -132,7 +137,8 @@ func TestCalculateRoomName(t *testing.T) {
ID: "@bob:localhost",
},
},
wantRoomName: "Alice and @bob:localhost",
wantRoomName: "Alice and @bob:localhost",
wantCalculated: true,
},
// 3-way room, no display names
{
Expand All @@ -147,7 +153,8 @@ func TestCalculateRoomName(t *testing.T) {
ID: "@bob:localhost",
},
},
wantRoomName: "@alice:localhost and @bob:localhost",
wantRoomName: "@alice:localhost and @bob:localhost",
wantCalculated: true,
},
// disambiguation all
{
Expand All @@ -168,7 +175,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Alice",
},
},
wantRoomName: "Alice (@alice:localhost), Alice (@bob:localhost), Alice (@charlie:localhost) and 6 others",
wantRoomName: "Alice (@alice:localhost), Alice (@bob:localhost), Alice (@charlie:localhost) and 6 others",
wantCalculated: true,
},
// disambiguation some
{
Expand All @@ -189,7 +197,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Alice",
},
},
wantRoomName: "Alice (@alice:localhost), Bob, Alice (@charlie:localhost) and 6 others",
wantRoomName: "Alice (@alice:localhost), Bob, Alice (@charlie:localhost) and 6 others",
wantCalculated: true,
},
// disambiguation, faking user IDs as display names
{
Expand All @@ -205,7 +214,8 @@ func TestCalculateRoomName(t *testing.T) {
ID: "@alice:localhost",
},
},
wantRoomName: "@alice:localhost (@evil:localhost) and @alice:localhost (@alice:localhost)",
wantRoomName: "@alice:localhost (@evil:localhost) and @alice:localhost (@alice:localhost)",
wantCalculated: true,
},
// left room
{
Expand All @@ -222,7 +232,8 @@ func TestCalculateRoomName(t *testing.T) {
Name: "Bob",
},
},
wantRoomName: "Empty Room (was Alice and Bob)",
wantRoomName: "Empty Room (was Alice and Bob)",
wantCalculated: true,
},
// empty room
{
Expand All @@ -235,7 +246,7 @@ func TestCalculateRoomName(t *testing.T) {
}

for _, tc := range testCases {
gotName := CalculateRoomName(&RoomMetadata{
gotName, gotCalculated := CalculateRoomName(&RoomMetadata{
NameEvent: tc.roomName,
CanonicalAlias: tc.canonicalAlias,
Heroes: tc.heroes,
Expand All @@ -245,6 +256,9 @@ func TestCalculateRoomName(t *testing.T) {
if gotName != tc.wantRoomName {
t.Errorf("got %s want %s for test case: %+v", gotName, tc.wantRoomName, tc)
}
if gotCalculated != tc.wantCalculated {
t.Errorf("got %v want %v for test case: %+v", gotCalculated, tc.wantCalculated, tc)
}
}
}

Expand Down
9 changes: 7 additions & 2 deletions sync3/handler/connstate.go
Original file line number Diff line number Diff line change
Expand Up @@ -664,8 +664,9 @@ func (s *ConnState) getInitialRoomData(ctx context.Context, roomSub sync3.RoomSu
}
}

rooms[roomID] = sync3.Room{
Name: internal.CalculateRoomName(metadata, 5), // TODO: customisable?
roomName, calculated := internal.CalculateRoomName(metadata, 5) // TODO: customisable?
room := sync3.Room{
Name: roomName,
AvatarChange: sync3.NewAvatarChange(internal.CalculateAvatar(metadata)),
NotificationCount: int64(userRoomData.NotificationCount),
HighlightCount: int64(userRoomData.HighlightCount),
Expand All @@ -679,6 +680,10 @@ func (s *ConnState) getInitialRoomData(ctx context.Context, roomSub sync3.RoomSu
PrevBatch: userRoomData.RequestedLatestEvents.PrevBatch,
Timestamp: maxTs,
}
if roomSub.IncludeHeroes() && calculated {
room.Heroes = metadata.Heroes
}
rooms[roomID] = room
}

if rsm.IsLazyLoading() {
Expand Down
25 changes: 24 additions & 1 deletion sync3/handler/connstate_live.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,13 @@ func (s *connStateLive) processLiveUpdate(ctx context.Context, up caches.Update,
if delta.RoomNameChanged {
metadata := roomUpdate.GlobalRoomMetadata()
metadata.RemoveHero(s.userID)
thisRoom.Name = internal.CalculateRoomName(metadata, 5) // TODO: customisable?
roomName, calculated := internal.CalculateRoomName(metadata, 5) // TODO: customisable?

thisRoom.Name = roomName

if calculated && s.shouldIncludeHeroes(roomUpdate.RoomID()) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks better, but where's the tests?! :D

thisRoom.Heroes = metadata.Heroes
}
}
if delta.RoomAvatarChanged {
metadata := roomUpdate.GlobalRoomMetadata()
Expand Down Expand Up @@ -438,3 +444,20 @@ func (s *connStateLive) resort(
}
return ops, hasUpdates
}

// shouldIncludeHeroes returns whether the given roomID is in a list or direct
// subscription which should return heroes.
func (s *connStateLive) shouldIncludeHeroes(roomID string) bool {
if s.roomSubscriptions[roomID].IncludeHeroes() {
return true
}
roomIDsToLists := s.lists.ListsByVisibleRoomIDs(s.muxedReq.Lists)
for _, listKey := range roomIDsToLists[roomID] {
// check if this list should include heroes
if !s.muxedReq.Lists[listKey].IncludeHeroes() {
continue
}
return true
}
return false
}
90 changes: 90 additions & 0 deletions sync3/handler/connstate_live_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package handler

import (
"context"
"testing"

"github.com/matrix-org/sliding-sync/internal"
"github.com/matrix-org/sliding-sync/sync3"
)

func Test_connStateLive_shouldIncludeHeroes(t *testing.T) {
ctx := context.Background()
list := sync3.NewInternalRequestLists()

m1 := sync3.RoomConnMetadata{
RoomMetadata: internal.RoomMetadata{
RoomID: "!abc",
},
}
m2 := sync3.RoomConnMetadata{
RoomMetadata: internal.RoomMetadata{
RoomID: "!def",
},
}
list.SetRoom(m1)
list.SetRoom(m2)

list.AssignList(ctx, "all_rooms", &sync3.RequestFilters{}, []string{sync3.SortByName}, false)
list.AssignList(ctx, "visible_rooms", &sync3.RequestFilters{}, []string{sync3.SortByName}, false)

boolTrue := true
tests := []struct {
name string
ConnState *ConnState
roomID string
want bool
}{
{
name: "neither in subscription nor in list",
roomID: "!abc",
ConnState: &ConnState{
muxedReq: &sync3.Request{},
},
},
{
name: "in room subscription",
want: true,
roomID: "!abc",
ConnState: &ConnState{
muxedReq: &sync3.Request{},
roomSubscriptions: map[string]sync3.RoomSubscription{
"!abc": {
Heroes: &boolTrue,
},
},
},
},
{
name: "in list all_rooms",
roomID: "!def",
want: true,
ConnState: &ConnState{
muxedReq: &sync3.Request{
Lists: map[string]sync3.RequestList{
"all_rooms": {
SlowGetAllRooms: &boolTrue,
RoomSubscription: sync3.RoomSubscription{
Heroes: &boolTrue,
},
},
"visible_rooms": {
SlowGetAllRooms: &boolTrue,
},
},
},
lists: list,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := &connStateLive{
ConnState: tt.ConnState,
}
if got := s.shouldIncludeHeroes(tt.roomID); got != tt.want {
t.Errorf("shouldIncludeHeroes() = %v, want %v", got, tt.want)
}
})
}
}
6 changes: 4 additions & 2 deletions sync3/lists.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) {
delta.RoomNameChanged = !existing.SameRoomName(&r.RoomMetadata)
if delta.RoomNameChanged {
// update the canonical name to allow room name sorting to continue to work
roomName, _ := internal.CalculateRoomName(&r.RoomMetadata, 5)
r.CanonicalisedName = strings.ToLower(
strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"),
strings.Trim(roomName, "#!():_@"),
)
} else {
// XXX: during TestConnectionTimeoutNotReset there is some situation where
Expand Down Expand Up @@ -109,8 +110,9 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) {
}
} else {
// set the canonical name to allow room name sorting to work
roomName, _ := internal.CalculateRoomName(&r.RoomMetadata, 5)
r.CanonicalisedName = strings.ToLower(
strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"),
strings.Trim(roomName, "#!():_@"),
)
r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata)
// We'll automatically use the LastInterestedEventTimestamps provided by the
Expand Down
Loading
Loading