From c3eb517700f35eb501b3557ff2561f249a2e4ef8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Jun 2021 08:22:43 +0200 Subject: [PATCH 1/5] Use fallback avatar only for DMs with 2 people MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- src/Avatar.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Avatar.ts b/src/Avatar.ts index a6499c688e7..5b033feb8f6 100644 --- a/src/Avatar.ts +++ b/src/Avatar.ts @@ -146,15 +146,11 @@ export function avatarUrlForRoom(room: Room, width: number, height: number, resi // space rooms cannot be DMs so skip the rest if (SettingsStore.getValue("feature_spaces") && room.isSpaceRoom()) return null; - let otherMember = null; const otherUserId = DMRoomMap.shared().getUserIdForRoomId(room.roomId); - if (otherUserId) { - otherMember = room.getMember(otherUserId); - } else { - // if the room is not marked as a 1:1, but only has max 2 members - // then still try to show any avatar (pref. other member) - otherMember = room.getAvatarFallbackMember(); - } + if (!otherUserId) return null; + + // If there are only two members in the DM use the avatar of the other member + const otherMember = room.getAvatarFallbackMember(otherUserId); if (otherMember?.getMxcAvatarUrl()) { return mediaFromMxc(otherMember.getMxcAvatarUrl()).getThumbnailOfSourceHttp(width, height, resizeMethod); } From 4b2a9a6bf77471f261f98084fb3b61e8caedf257 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Jun 2021 08:33:14 +0200 Subject: [PATCH 2/5] Remove mistaken param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- src/Avatar.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Avatar.ts b/src/Avatar.ts index 5b033feb8f6..79f9bb699b6 100644 --- a/src/Avatar.ts +++ b/src/Avatar.ts @@ -146,11 +146,10 @@ export function avatarUrlForRoom(room: Room, width: number, height: number, resi // space rooms cannot be DMs so skip the rest if (SettingsStore.getValue("feature_spaces") && room.isSpaceRoom()) return null; - const otherUserId = DMRoomMap.shared().getUserIdForRoomId(room.roomId); - if (!otherUserId) return null; + if (!DMRoomMap.shared().getUserIdForRoomId(room.roomId)) return null; // If there are only two members in the DM use the avatar of the other member - const otherMember = room.getAvatarFallbackMember(otherUserId); + const otherMember = room.getAvatarFallbackMember(); if (otherMember?.getMxcAvatarUrl()) { return mediaFromMxc(otherMember.getMxcAvatarUrl()).getThumbnailOfSourceHttp(width, height, resizeMethod); } From 87622fb8eaccf0ce0b59799b050b095c8e354857 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Jun 2021 08:34:32 +0200 Subject: [PATCH 3/5] Add a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- src/Avatar.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Avatar.ts b/src/Avatar.ts index 79f9bb699b6..e6b47f8a7f6 100644 --- a/src/Avatar.ts +++ b/src/Avatar.ts @@ -146,6 +146,7 @@ export function avatarUrlForRoom(room: Room, width: number, height: number, resi // space rooms cannot be DMs so skip the rest if (SettingsStore.getValue("feature_spaces") && room.isSpaceRoom()) return null; + // If the room is not a DM don't fallback to a member avatar if (!DMRoomMap.shared().getUserIdForRoomId(room.roomId)) return null; // If there are only two members in the DM use the avatar of the other member From 226131409f3854cc3bfbc7eecef42b3bb07ea270 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 1 Oct 2021 14:54:26 +0100 Subject: [PATCH 4/5] Unit tests for room avatars in DM and non-DM rooms Signed-off-by: Andy Balaam --- .../views/rooms/RoomHeader-test.tsx | 252 ++++++++++++++++++ test/test-utils.js | 14 +- 2 files changed, 262 insertions(+), 4 deletions(-) create mode 100644 test/components/views/rooms/RoomHeader-test.tsx diff --git a/test/components/views/rooms/RoomHeader-test.tsx b/test/components/views/rooms/RoomHeader-test.tsx new file mode 100644 index 00000000000..6696c8bd9c5 --- /dev/null +++ b/test/components/views/rooms/RoomHeader-test.tsx @@ -0,0 +1,252 @@ +import React from 'react'; +import ReactDOM from 'react-dom'; + +import "../../../skinned-sdk"; + +import * as TestUtils from '../../../test-utils'; + +import { MatrixClientPeg } from '../../../../src/MatrixClientPeg'; + +import DMRoomMap from '../../../../src/utils/DMRoomMap'; +import RoomHeader from '../../../../src/components/views/rooms/RoomHeader'; + +import { Room, PendingEventOrdering, MatrixEvent, MatrixClient } from 'matrix-js-sdk'; +import { SearchScope } from '../../../../src/components/views/rooms/SearchBar'; +import { E2EStatus } from '../../../../src/utils/ShieldUtils'; +import { PlaceCallType } from '../../../../src/CallHandler'; +import { mkEvent } from '../../../test-utils'; + +describe('RoomHeader', () => { + test('shows the room avatar in a room with only ourselves', () => { + // When we render a non-DM room with 1 person in it + const room = createRoom({ name: "X Room", isDm: false, users: [] }); + const rendered = render(room); + + // Then the room's avatar is the initial of its name + const initial = findSpan(rendered, ".mx_BaseAvatar_initial"); + expect(initial.innerHTML).toEqual("X"); + + // And there is no image avatar (because it's not set on this room) + const image = findImg(rendered, ".mx_BaseAvatar_image"); + expect(image.src).toEqual("data:image/png;base64,00"); + }); + + test('shows the room avatar in a room with 2 people', () => { + // When we render a non-DM room with 2 people in it + const room = createRoom( + { name: "Y Room", isDm: false, users: ["other"] }); + const rendered = render(room); + + // Then the room's avatar is the initial of its name + const initial = findSpan(rendered, ".mx_BaseAvatar_initial"); + expect(initial.innerHTML).toEqual("Y"); + + // And there is no image avatar (because it's not set on this room) + const image = findImg(rendered, ".mx_BaseAvatar_image"); + expect(image.src).toEqual("data:image/png;base64,00"); + }); + + test('shows the room avatar in a room with >2 people', () => { + // When we render a non-DM room with 3 people in it + const room = createRoom( + { name: "Z Room", isDm: false, users: ["other1", "other2"] }); + const rendered = render(room); + + // Then the room's avatar is the initial of its name + const initial = findSpan(rendered, ".mx_BaseAvatar_initial"); + expect(initial.innerHTML).toEqual("Z"); + + // And there is no image avatar (because it's not set on this room) + const image = findImg(rendered, ".mx_BaseAvatar_image"); + expect(image.src).toEqual("data:image/png;base64,00"); + }); + + test('shows the room avatar in a DM with only ourselves', () => { + // When we render a non-DM room with 1 person in it + const room = createRoom({ name: "Z Room", isDm: true, users: [] }); + const rendered = render(room); + + // Then the room's avatar is the initial of its name + const initial = findSpan(rendered, ".mx_BaseAvatar_initial"); + expect(initial.innerHTML).toEqual("Z"); + + // And there is no image avatar (because it's not set on this room) + const image = findImg(rendered, ".mx_BaseAvatar_image"); + expect(image.src).toEqual("data:image/png;base64,00"); + }); + + test('shows the user avatar in a DM with 2 people', () => { + // Note: this is the interesting case - this is the ONLY + // time we should use the user's avatar. + + // When we render a DM room with only 2 people in it + const room = createRoom( + { name: "Y Room", isDm: true, users: ["other"] }); + const rendered = render(room); + + // Then we use the other user's avatar as our room's image avatar + const image = findImg(rendered, ".mx_BaseAvatar_image"); + expect(image.src).toEqual( + "http://this.is.a.url/example.org/other"); + + // And there is no initial avatar + expect( + rendered.querySelectorAll(".mx_BaseAvatar_initial"), + ).toHaveLength(0); + }); + + test('shows the room avatar in a DM with >2 people', () => { + // When we render a DM room with 3 people in it + const room = createRoom({ + name: "Z Room", isDm: true, users: ["other1", "other2"] }); + const rendered = render(room); + + // Then the room's avatar is the initial of its name + const initial = findSpan(rendered, ".mx_BaseAvatar_initial"); + expect(initial.innerHTML).toEqual("Z"); + + // And there is no image avatar (because it's not set on this room) + const image = findImg(rendered, ".mx_BaseAvatar_image"); + expect(image.src).toEqual("data:image/png;base64,00"); + }); +}); + +interface IRoomCreationInfo { + name: string; + isDm: boolean; + users: string[]; +} + +function createRoom(info: IRoomCreationInfo) { + TestUtils.stubClient(); + const client: MatrixClient = MatrixClientPeg.get(); + + const roomId = '!1234567890:domain'; + const userId = client.getUserId(); + if (info.isDm) { + client.getAccountData = (eventType) => { + expect(eventType).toEqual("m.direct"); + return mkDirectEvent(roomId, userId, info.users); + }; + } + + DMRoomMap.makeShared().start(); + + const room = new Room(roomId, client, userId, { + pendingEventOrdering: PendingEventOrdering.Detached, + }); + + const otherJoinEvents = []; + for (const otherUserId of info.users) { + otherJoinEvents.push(mkJoinEvent(roomId, otherUserId)); + } + + room.currentState.setStateEvents([ + mkCreationEvent(roomId, userId), + mkNameEvent(roomId, userId, info.name), + mkJoinEvent(roomId, userId), + ...otherJoinEvents, + ]); + room.recalculate(); + + return room; +} + +function render(room: Room): HTMLDivElement { + const parentDiv = document.createElement('div'); + document.body.appendChild(parentDiv); + ReactDOM.render( + ( + {}} + onSearchClick={() => {}} + onForgetClick={() => {}} + onCallPlaced={(_type: PlaceCallType) => {}} + onAppsClick={() => {}} + e2eStatus={E2EStatus.Normal} + appsShown={true} + searchInfo={{ + searchTerm: "", + searchScope: SearchScope.Room, + searchCount: 0, + }} + /> + ), + parentDiv, + ); + return parentDiv; +} + +function mkCreationEvent(roomId: string, userId: string): MatrixEvent { + return mkEvent({ + event: true, + type: "m.room.create", + room: roomId, + user: userId, + content: { + creator: userId, + room_version: "5", + predecessor: { + room_id: "!prevroom", + event_id: "$someevent", + }, + }, + }); +} + +function mkNameEvent( + roomId: string, userId: string, name: string, +): MatrixEvent { + return mkEvent({ + event: true, + type: "m.room.name", + room: roomId, + user: userId, + content: { name }, + }); +} + +function mkJoinEvent(roomId: string, userId: string) { + const ret = mkEvent({ + event: true, + type: "m.room.member", + room: roomId, + user: userId, + content: { + "membership": "join", + "avatar_url": "mxc://example.org/" + userId, + }, + }); + ret.event.state_key = userId; + return ret; +} + +function mkDirectEvent( + roomId: string, userId: string, otherUsers: string[], +): MatrixEvent { + const content = {}; + for (const otherUserId of otherUsers) { + content[otherUserId] = [roomId]; + } + return mkEvent({ + event: true, + type: "m.direct", + room: roomId, + user: userId, + content, + }); +} + +function findSpan(parent: HTMLElement, selector: string): HTMLSpanElement { + const els = parent.querySelectorAll(selector); + expect(els.length).toEqual(1); + return els[0] as HTMLSpanElement; +} + +function findImg(parent: HTMLElement, selector: string): HTMLImageElement { + const els = parent.querySelectorAll(selector); + expect(els.length).toEqual(1); + return els[0] as HTMLImageElement; +} diff --git a/test/test-utils.js b/test/test-utils.js index c06149991ff..2091a6e0ed7 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -47,6 +47,8 @@ export function createTestClient() { getIdentityServerUrl: jest.fn(), getDomain: jest.fn().mockReturnValue("matrix.rog"), getUserId: jest.fn().mockReturnValue("@userId:matrix.rog"), + getUser: jest.fn().mockReturnValue({ on: jest.fn() }), + credentials: { userId: "@userId:matrix.rog" }, getPushActionsForEvent: jest.fn(), getRoom: jest.fn().mockImplementation(mkStubRoom), @@ -76,7 +78,7 @@ export function createTestClient() { content: {}, }); }, - mxcUrlToHttp: (mxc) => 'http://this.is.a.url/', + mxcUrlToHttp: (mxc) => `http://this.is.a.url/${mxc.substring(6)}`, setAccountData: jest.fn(), setRoomAccountData: jest.fn(), sendTyping: jest.fn().mockResolvedValue({}), @@ -93,12 +95,14 @@ export function createTestClient() { sessionStore: { store: { getItem: jest.fn(), + setItem: jest.fn(), }, }, pushRules: {}, decryptEventIfNeeded: () => Promise.resolve(), isUserIgnored: jest.fn().mockReturnValue(false), getCapabilities: jest.fn().mockResolvedValue({}), + supportsExperimentalThreads: () => false, }; } @@ -130,9 +134,11 @@ export function mkEvent(opts) { }; if (opts.skey) { event.state_key = opts.skey; - } else if (["m.room.name", "m.room.topic", "m.room.create", "m.room.join_rules", - "m.room.power_levels", "m.room.topic", "m.room.history_visibility", "m.room.encryption", - "com.example.state"].indexOf(opts.type) !== -1) { + } else if ([ + "m.room.name", "m.room.topic", "m.room.create", "m.room.join_rules", + "m.room.power_levels", "m.room.topic", "m.room.history_visibility", + "m.room.encryption", "m.room.member", "com.example.state", + ].indexOf(opts.type) !== -1) { event.state_key = ""; } return opts.event ? new MatrixEvent(event) : event; From e8dba59b42ff6cfc55ba6d97d6d1675dc228a964 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 4 Oct 2021 11:41:09 +0100 Subject: [PATCH 5/5] =?UTF-8?q?Fix=20review=20comments=20from=20=C5=A0imon?= =?UTF-8?q?=20Brandner.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename test->it; Rename users->userIds; Un-break a line. Signed-off-by: Andy Balaam --- .../views/rooms/RoomHeader-test.tsx | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/test/components/views/rooms/RoomHeader-test.tsx b/test/components/views/rooms/RoomHeader-test.tsx index 6696c8bd9c5..859107416e1 100644 --- a/test/components/views/rooms/RoomHeader-test.tsx +++ b/test/components/views/rooms/RoomHeader-test.tsx @@ -17,9 +17,9 @@ import { PlaceCallType } from '../../../../src/CallHandler'; import { mkEvent } from '../../../test-utils'; describe('RoomHeader', () => { - test('shows the room avatar in a room with only ourselves', () => { + it('shows the room avatar in a room with only ourselves', () => { // When we render a non-DM room with 1 person in it - const room = createRoom({ name: "X Room", isDm: false, users: [] }); + const room = createRoom({ name: "X Room", isDm: false, userIds: [] }); const rendered = render(room); // Then the room's avatar is the initial of its name @@ -31,10 +31,10 @@ describe('RoomHeader', () => { expect(image.src).toEqual("data:image/png;base64,00"); }); - test('shows the room avatar in a room with 2 people', () => { + it('shows the room avatar in a room with 2 people', () => { // When we render a non-DM room with 2 people in it const room = createRoom( - { name: "Y Room", isDm: false, users: ["other"] }); + { name: "Y Room", isDm: false, userIds: ["other"] }); const rendered = render(room); // Then the room's avatar is the initial of its name @@ -46,10 +46,10 @@ describe('RoomHeader', () => { expect(image.src).toEqual("data:image/png;base64,00"); }); - test('shows the room avatar in a room with >2 people', () => { + it('shows the room avatar in a room with >2 people', () => { // When we render a non-DM room with 3 people in it const room = createRoom( - { name: "Z Room", isDm: false, users: ["other1", "other2"] }); + { name: "Z Room", isDm: false, userIds: ["other1", "other2"] }); const rendered = render(room); // Then the room's avatar is the initial of its name @@ -61,9 +61,9 @@ describe('RoomHeader', () => { expect(image.src).toEqual("data:image/png;base64,00"); }); - test('shows the room avatar in a DM with only ourselves', () => { + it('shows the room avatar in a DM with only ourselves', () => { // When we render a non-DM room with 1 person in it - const room = createRoom({ name: "Z Room", isDm: true, users: [] }); + const room = createRoom({ name: "Z Room", isDm: true, userIds: [] }); const rendered = render(room); // Then the room's avatar is the initial of its name @@ -75,13 +75,12 @@ describe('RoomHeader', () => { expect(image.src).toEqual("data:image/png;base64,00"); }); - test('shows the user avatar in a DM with 2 people', () => { + it('shows the user avatar in a DM with 2 people', () => { // Note: this is the interesting case - this is the ONLY // time we should use the user's avatar. // When we render a DM room with only 2 people in it - const room = createRoom( - { name: "Y Room", isDm: true, users: ["other"] }); + const room = createRoom({ name: "Y Room", isDm: true, userIds: ["other"] }); const rendered = render(room); // Then we use the other user's avatar as our room's image avatar @@ -95,10 +94,10 @@ describe('RoomHeader', () => { ).toHaveLength(0); }); - test('shows the room avatar in a DM with >2 people', () => { + it('shows the room avatar in a DM with >2 people', () => { // When we render a DM room with 3 people in it const room = createRoom({ - name: "Z Room", isDm: true, users: ["other1", "other2"] }); + name: "Z Room", isDm: true, userIds: ["other1", "other2"] }); const rendered = render(room); // Then the room's avatar is the initial of its name @@ -114,7 +113,7 @@ describe('RoomHeader', () => { interface IRoomCreationInfo { name: string; isDm: boolean; - users: string[]; + userIds: string[]; } function createRoom(info: IRoomCreationInfo) { @@ -126,7 +125,7 @@ function createRoom(info: IRoomCreationInfo) { if (info.isDm) { client.getAccountData = (eventType) => { expect(eventType).toEqual("m.direct"); - return mkDirectEvent(roomId, userId, info.users); + return mkDirectEvent(roomId, userId, info.userIds); }; } @@ -137,7 +136,7 @@ function createRoom(info: IRoomCreationInfo) { }); const otherJoinEvents = []; - for (const otherUserId of info.users) { + for (const otherUserId of info.userIds) { otherJoinEvents.push(mkJoinEvent(roomId, otherUserId)); }