Improve rendering of empty topics in the timeline (#29152)

* Improve display of empty topic events in the timeline.

* Use topic parser for topic events.

* Revert changes i18n for the moment

* Use the correct import pattern

* Add tests for topic rendering

src/TextForEvent.tsx

@@ -17,6 +17,7 @@ import {
     MsgType,
     M_POLL_START,
     M_POLL_END,
+    ContentHelpers,
 } from "matrix-js-sdk/src/matrix";
 import { KnownMembership } from "matrix-js-sdk/src/types";
 import { logger } from "matrix-js-sdk/src/logger";
@@ -227,11 +228,16 @@ function textForMemberEvent(
 
 function textForTopicEvent(ev: MatrixEvent): (() => string) | null {
     const senderDisplayName = ev.sender && ev.sender.name ? ev.sender.name : ev.getSender();
+    const topic = ContentHelpers.parseTopicContent(ev.getContent()).text;
     return () =>
-        _t("timeline|m.room.topic", {
-            senderDisplayName,
-            topic: ev.getContent().topic,
-        });
+        topic
+            ? _t("timeline|m.room.topic|changed", {
+                  senderDisplayName,
+                  topic,
+              })
+            : _t("timeline|m.room.topic|removed", {
+                  senderDisplayName,
+              });
 }
 
 function textForRoomAvatarEvent(ev: MatrixEvent): (() => string) | null {

src/components/views/room_settings/RoomProfileSettings.tsx

@@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details.
 
 import React, { createRef } from "react";
 import classNames from "classnames";
-import { EventType } from "matrix-js-sdk/src/matrix";
+import { ContentHelpers, EventType } from "matrix-js-sdk/src/matrix";
 
 import { _t } from "../../../languageHandler";
 import { MatrixClientPeg } from "../../../MatrixClientPeg";
@@ -51,7 +51,7 @@ export default class RoomProfileSettings extends React.Component<IProps, IState>
         const avatarUrl = avatarEvent?.getContent()["url"] ?? null;
 
         const topicEvent = room.currentState.getStateEvents(EventType.RoomTopic, "");
-        const topic = topicEvent && topicEvent.getContent() ? topicEvent.getContent()["topic"] : "";
+        const topic = (topicEvent && ContentHelpers.parseTopicContent(topicEvent.getContent()).text) || "";
 
         const nameEvent = room.currentState.getStateEvents(EventType.RoomName, "");
         const name = nameEvent && nameEvent.getContent() ? nameEvent.getContent()["name"] : "";
@@ -145,6 +145,8 @@ export default class RoomProfileSettings extends React.Component<IProps, IState>
 
         if (this.state.originalTopic !== this.state.topic) {
             const html = htmlSerializeFromMdIfNeeded(this.state.topic, { forceHTML: false });
+            // XXX: Note that we deliberately send an empty string on an empty topic rather
+            // than a clearer `undefined` value. Synapse still requires a string in a topic.
             await client.setRoomTopic(this.props.roomId, this.state.topic, html);
             newState.originalTopic = this.state.topic;
         }

src/i18n/strings/en_EN.json

@@ -3493,7 +3493,10 @@
             "sent": "%(senderName)s sent an invitation to %(targetDisplayName)s to join the room."
         },
         "m.room.tombstone": "%(senderDisplayName)s upgraded this room.",
-        "m.room.topic": "%(senderDisplayName)s changed the topic to \"%(topic)s\".",
+        "m.room.topic": {
+            "changed": "%(senderDisplayName)s changed the topic to \"%(topic)s\".",
+            "removed": "%(senderDisplayName)s removed the topic."
+        },
         "m.sticker": "%(senderDisplayName)s sent a sticker.",
         "m.video": {
             "error_decrypting": "Error decrypting video"

test/unit-tests/TextForEvent-test.ts

@@ -12,6 +12,7 @@ import {
     JoinRule,
     MatrixClient,
     MatrixEvent,
+    MRoomTopicEventContent,
     Room,
     RoomMember,
 } from "matrix-js-sdk/src/matrix";
@@ -613,4 +614,47 @@ describe("TextForEvent", () => {
             },
         );
     });
+
+    describe("textForTopicEvent()", () => {
+        type TestCase = [string, MRoomTopicEventContent, { result: string }];
+        const testCases: TestCase[] = [
+            ["the legacy key", { topic: "My topic" }, { result: '@a changed the topic to "My topic".' }],
+            [
+                "the legacy key with an empty m.topic key",
+                { "topic": "My topic", "m.topic": [] },
+                { result: '@a changed the topic to "My topic".' },
+            ],
+            [
+                "the m.topic key",
+                { "topic": "Ignore this", "m.topic": [{ mimetype: "text/plain", body: "My topic" }] },
+                { result: '@a changed the topic to "My topic".' },
+            ],
+            [
+                "the m.topic key and the legacy key undefined",
+                { "topic": undefined, "m.topic": [{ mimetype: "text/plain", body: "My topic" }] },
+                { result: '@a changed the topic to "My topic".' },
+            ],
+            ["the legacy key undefined", { topic: undefined }, { result: "@a removed the topic." }],
+            ["the legacy key empty string", { topic: "" }, { result: "@a removed the topic." }],
+            [
+                "both the legacy and new keys removed",
+                { "topic": undefined, "m.topic": [] },
+                { result: "@a removed the topic." },
+            ],
+        ];
+
+        it.each(testCases)("returns correct message for topic event with %s", (_caseName, content, { result }) => {
+            expect(
+                textForEvent(
+                    new MatrixEvent({
+                        type: "m.room.topic",
+                        sender: "@a",
+                        content: content,
+                        state_key: "",
+                    }),
+                    mockClient,
+                ),
+            ).toEqual(result);
+        });
+    });
 });